reecepeg@3c22fb7d6b37 Pulpmill % python3 --version
Python 3.9.1

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.
https://github.com/python/cpython/blob/main/Modules/_io/bufferedio.c#L845

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 urllib3.response.py. Should I still go ahead with writing the test cases in this case? I worry about creating new dependencies for the python repository.

New changeset a450398 by AngstyDuck in branch 'main':
bpo-44687: Ensure BufferedReader objects with unread buffers can peek even when the underlying file is closed (GH-28457)
a450398

Thanks for sticking through it! Sorry about the delays, but I think we ended up with a good fix.

New changeset 2221207 by Miss Islington (bot) in branch '3.10':
bpo-44687: Ensure BufferedReader objects with unread buffers can peek even when the underlying file is closed (GH-28457)
2221207

New changeset 6035d65 by Miss Islington (bot) in branch '3.9':
bpo-44687: Ensure BufferedReader objects with unread buffers can peek even when the underlying file is closed (GH-28457)
6035d65