Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: eliminate 2nd call to Close() #51

Merged
merged 1 commit into from
Dec 15, 2021
Merged

Bugfix: eliminate 2nd call to Close() #51

merged 1 commit into from
Dec 15, 2021

Conversation

BuckeyeCoder
Copy link
Contributor

While researching your answer in #50 I came across a bug, and am submitting the fix which is simple.

The method qrcode.Save() is calling Close() too many times. In my test code this was causing problems as follows:

drawTo being called
Write: Called with 4096 bytes
Write: Called with 4096 bytes
Write: Called with 4096 bytes
Write: Called with 4096 bytes
Write: Called with 3509 bytes
drawTo returning
STANDARD WRITER.Write() DEFER CLOSE COMING
Close: 19893 bytes in the Buffer
QRCODE.SAVE CLOSE COMING
Close: 0 bytes in the Buffer

In this test qrcode.Save() uses the standard writer's Write() method which calls drawTo() which begins processing 4k chunks normally. When drawTo() completes and returns up the stack to standard.Writer.Write() a defer calls Close() for the first time. This causes my test code to report the size of the Buffer. Control passes up the stack back to the qrcode.Save() method which calls .Close() once again. This causes my test code to again report the size of the Buffer which is now zero.

This is obviously not desirable and potentially causes problems for some custom writers. The question becomes, "Which Close() should be eliminated?" I think the answer to that is easy, the defer in the Write() method makes much more sense to eliminate. Having the Write() and Close() together in qrcode.Save() is much more readable and proper.

This PR eliminates the superfluous deferred call to Close() in the standard writer.

@BuckeyeCoder

@yeqown yeqown merged commit dd74daa into yeqown:main Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants