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
io.BufferedReader:peek() closes underlying file, breaking peek/read expectations #88853
Comments
reecepeg@3c22fb7d6b37 Pulpmill % python3 --version When buffering a small file, calling peek() can read in the entire underlying thing and close it, so then following with a read() throws an error about the file being closed already even though peek should not have that effect. Reproducible Steps: >>> r = BufferedReader(requests.get("https://google.com", stream=True).raw)
>>> r.peek(2)[:2]
b'\x1f\x8b'
>>> r.peek()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: peek of closed file
>>> r.read()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: read of closed file However, in the case of a larger stream it appears to work since the underlying stream isn't closed yet: >>> r = BufferedReader(requests.get("https://amazon.com", stream=True).raw)
>>> r.peek(2)[:2]
b'\x1f\x8b'
>>> r.peek(2)[:2]
b'\x1f\x8b' This seems inconsistent at best. Best I can tell, the issue is here and needs to take the current buffer offset into account. |
Seems like it would be better to not check the file open/closed state on peek/read when there is still buffered data? |
Yeap Steve, I was thinking the same thing. I've taken the buffered data into consideration when checking if the file is closed in my fix, feel free to take a look and let me know what you think :) |
That change makes me much happier :) Do you think we need a specific test added for this case? I'd guess we have plenty of coverage for the changed macro already, but clearly nothing that showed the issue before. |
Awesome! To be honest I was very inclined to add test cases for this edge case, but while some existing test cases use native objects like io.BytesIO, the one that our edge case uses is HTTPResponse from |
Thanks for sticking through it! Sorry about the delays, but I think we ended up with a good fix. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: