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

Сlose the old body explicitly after seek for S3. Fix #187 #188

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

inksink
Copy link
Contributor

@inksink inksink commented Apr 10, 2018

Solve issue #187

@inksink inksink changed the title close the old body explicitly after when seek() close the old body explicitly after seek() Apr 10, 2018
smart_open/s3.py Outdated
#
try:
self._body.close()
except Exception as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not be explicit and catch AttributeError?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm +1 too for catching more "restricted", after this fix I think we can merge.

Copy link
Owner

@piskvorky piskvorky Apr 10, 2018

Choose a reason for hiding this comment

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

Is it really only AttributeError? Cannot close() raise something else? (and if it does, should we care?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@piskvorky I don't think we expect close itself to raise anything.

I think the intention here is to mimic the behavior of:

if body is not None:
    body.close()

without paying the price of the conditional every time. I think the author is assuming that most the time, the body will be non-null. I think this assumption is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, if hasattr(self, '_body'), self._body must be io.BytesIO() or object.get()['Body'] (if it really raise something else, we should not care it and just raise it).
And I have changed Exception to AttributeError.

@menshikh-iv menshikh-iv changed the title close the old body explicitly after seek() Сlose the old body explicitly after seek for S3. Fix #187 Apr 10, 2018
@menshikh-iv menshikh-iv merged commit 5b58023 into piskvorky:master Apr 10, 2018
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.

4 participants