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

Add statement to close sevenZFile object #5

Closed
wants to merge 2 commits into from
Closed

Add statement to close sevenZFile object #5

wants to merge 2 commits into from

Conversation

sebasblancogonz
Copy link

#4 I just added a line to close the SevenZFile object. A simple call to the close() method of the class.
I've tested it on a local project and it works like a charm!

Hope it fits!

@simonnagl simonnagl self-requested a review November 21, 2019 07:42
Copy link
Member

@simonnagl simonnagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your first pull request. I really appreciate your work done. Can you please use withCloseable instead of calling close directly? This will close the SevenZFile even if an exception is thrown while unzipping.

@sebasblancogonz
Copy link
Author

sebasblancogonz commented Nov 21, 2019

Thank you so much for your advice! For sure will never forget it! Is it correct now?

@simonnagl simonnagl mentioned this pull request Nov 22, 2019
@simonnagl simonnagl closed this in #6 Nov 22, 2019
@simonnagl
Copy link
Member

I just implemented the fix myself. The problem with your fix is that sevenZFile and entry are defined outside the auto-close scope.
This is no problem for now, but the code is now open for bugs. If someone accesses one of the variables after the scope, unexpected things might happen.

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