Skip to content

bpo-33173: Return Underlying fileobj's Seekability in GzipFile.seekable#6303

Closed
waltaskew wants to merge 1 commit intopython:masterfrom
waltaskew:gzipfile-seekable-deference
Closed

bpo-33173: Return Underlying fileobj's Seekability in GzipFile.seekable#6303
waltaskew wants to merge 1 commit intopython:masterfrom
waltaskew:gzipfile-seekable-deference

Conversation

@waltaskew
Copy link
Copy Markdown

@waltaskew waltaskew commented Mar 28, 2018

Does something along these lines seem reasonable?
I ran into an issue where the seekable() method was returning true despite the underlying fileobj not being seekable, which lead to errors when a consumer of the GzipFile used the results of the seekable() method as evidence it was safe to call seek.

https://bugs.python.org/issue33173

@the-knights-who-say-ni
Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@waltaskew waltaskew force-pushed the gzipfile-seekable-deference branch 2 times, most recently from c19e1d4 to 68b117f Compare March 29, 2018 17:33
Comment thread Lib/gzip.py Outdated
# See if we can use the seek and tell methods to seek
# to the current location.
try:
self.fileobj.seek(self.fileobj.tell())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not seek(0, SEEK_CUR)? (typical implementation of tell)

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Would be nice to add tests.

Comment thread Lib/gzip.py Outdated
try:
self.fileobj.seek(self.fileobj.tell())
return True
except (AttributeError, OSError, IOError):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IOError is an alias of OSError in Python 3.

@waltaskew waltaskew force-pushed the gzipfile-seekable-deference branch from 68b117f to eaa6e30 Compare November 8, 2018 16:55
@waltaskew
Copy link
Copy Markdown
Author

Sorry, had nearly forgotten about this!
I checked bz2 and zipfile to see what they do. They simply return the result of seekable() on their underlying buffer rather than trying to do the clever stuff I was doing with checking if the buffer implemented seek & tell:
https://github.com/python/cpython/blob/master/Lib/zipfile.py#L1019
https://github.com/python/cpython/blob/master/Lib/zipfile.py#L825
https://github.com/python/cpython/blob/master/Lib/bz2.py#L149

I simplified this pull request to mimic that behaviour and added a test.

Also, I have signed the CLA, just hadn't before make this pull request.

@brettcannon
Copy link
Copy Markdown
Member

@waltaskew it looks like there was a hiccup with your CLA and it isn't recorded as such on bugs.python.org. Could you email contributors@python.org and try to track down what's happened?

@csabella
Copy link
Copy Markdown
Contributor

@waltaskew, I'm going to close this pending the CLA. Once you get it sorted per @brettcannon's request, feel free to re-open this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants