bpo-44687: Fix unintended closing file error when peeking a BufferedReader object #28457
Conversation
…ally passes when the file was previously read
|
What about a read() after the first peek too? I’d expect peek() to allow a read afterward. |
|
Thanks for pointing it out, I'll alter the fix to accommodate the order you've mentioned as well. |
| @@ -0,0 +1 @@ | |||
| Add a new :c:macro:`PEEK_CHECK_CLOSED` macro to check if a file is closed using a method specific to BufferedReader.peek() | |||
There was a problem hiding this comment.
The NEWS entry should be relevant to users, not developers. This would be a better commit message, while the PR title would be better for NEWS.
There was a problem hiding this comment.
thanks for the feedback, i've updated the NEWS entry to incorporate more of the PR title. Hope that works 👍
Modules/_io/bufferedio.c
Outdated
| CHECK_CLOSED(self, "peek of closed file") | ||
|
|
||
| // use a check specific to peek if the previous file operation was not a read | ||
| if (Py_SAFE_DOWNCAST(self->pos, Py_off_t, Py_ssize_t) == 0) { |
There was a problem hiding this comment.
If I'm reading this correctly, this just means we haven't done a read or a seek yet?
Wouldn't it be better to not close the file after a peek, but wait for the read? If the read never comes, it'll be closed when the stream is closed as it normally would.
There was a problem hiding this comment.
If I'm reading this correctly, this just means we haven't done a read or a seek yet?
thanks for your comment, and yes the if condition checks if a read or a seek has been done before.
Wouldn't it be better to not close the file after a peek, but wait for the read? If the read never comes, it'll be closed when the stream is closed as it normally would.
I considered this solution initially as well, but unfortunately based on the example provided, the closing of the file is actually done by one of the methods of the object that requests.get("https://amazon.com", stream=True).raw returns.
I've just pushed an alternate fix, where I've reset some attributes of BufferedReader when its method closed() has been called. I believe this fixes the bug while passing the test case I've previously encountered which prompted me to write this i condition in the first place. Do let me know what you think.
…new check now works regardless of which method was called before peek()
…thub.com:AngstyDuck/cpython into iobuffreader_stop_closing_file_error_during_peek
|
After digging into the issue further, I've realised that the bug can be seen for all methods that use the original check |
Misc/NEWS.d/next/C API/2021-09-19-17-18-25.bpo-44687.3fqDRC.rst
Outdated
Show resolved
Hide resolved
…7.3fqDRC.rst Co-authored-by: Steve Dower <steve.dower@microsoft.com>
zooba
added
needs backport to 3.9
|
Thanks @AngstyDuck for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10. |
|
GH-28685 is a backport of this pull request to the 3.10 branch. |
|
GH-28686 is a backport of this pull request to the 3.9 branch. |
… even when the underlying file is closed (pythonGH-28457) (cherry picked from commit a450398) Co-authored-by: AngstyDuck <solsticedante@gmail.com>
… even when the underlying file is closed (pythonGH-28457) (cherry picked from commit a450398) Co-authored-by: AngstyDuck <solsticedante@gmail.com>
… even when the underlying file is closed (GH-28457) Co-authored-by: AngstyDuck <solsticedante@gmail.com>
When the user runs the
peek()function twice on a BufferedReader object, a closed file error may unintentionally be raised on the second time. This PR applies a check specific to thepeek()function which runs only when the previous file operation is apeek().https://bugs.python.org/issue44687