-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-141314: Fix TextIOWrapper.tell() assertion failure with standalone carriage return #141331
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
gh-141314: Fix TextIOWrapper.tell() assertion failure with standalone carriage return #141331
Conversation
…dalone carriage return When TextIOWrapper.tell() is called after reading a line that ends with a standalone carriage return (\r), the tell optimization algorithm incorrectly assumes there is buffered data to search through. This causes an assertion failure when skip_back=1 exceeds the empty buffer size. The fix detects when next_input is empty and skips the optimization phase, falling back to the byte-by-byte decoding method which always works correctly. This properly handles the architectural constraint that buffer optimization cannot function without buffered data.
…dalone carriage return Add test case and fix assertion failure in TextIOWrapper.tell() when reading files that end with a standalone carriage return (\r). The optimization algorithm incorrectly assumed buffered data would always be available, causing an assertion failure when next_input is empty.
- Remove trailing whitespace - Add missing newline at end of NEWS file
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cause of this bug is a simple typo.
assert(skip_bytes <= PyBytes_GET_SIZE(next_input));
Lib/test/test_io/test_textio.py
Outdated
| remaining = f.read() | ||
| self.assertEqual(remaining, "") | ||
|
|
||
| def test_tell_after_readline_with_multiple_cr(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test necessary?
Lib/test/test_io/test_textio.py
Outdated
| def test_tell_after_readline_with_cr(self): | ||
| # Test for gh-141314: TextIOWrapper.tell() assertion failure | ||
| # when dealing with standalone carriage returns | ||
| data = b'line1=1\r' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be simply b'line1\r'
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
sergey-miryanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert unnecessary changes in test_textio.py
- Fix assertion to check skip_bytes instead of skip_back - Use simpler test data b'line1\r' instead of b'line1=1\r' - Remove unnecessary multiple CR test case - Clean up workaround code The assertion was checking wrong variable (skip_back vs skip_bytes). skip_back is search step size, skip_bytes is buffer offset needing validation.
b298058 to
e84d686
Compare
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, 👍
sergey-miryanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
Thanks @mohsinm-dev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
|
Thanks @mohsinm-dev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
|
Sorry, @mohsinm-dev and @serhiy-storchaka, I could not cleanly backport this to |
|
Sorry, @mohsinm-dev and @serhiy-storchaka, I could not cleanly backport this to |
|
The cherry picks conflicts are probably because of the |
If I am not wrong Lib/test/test_io/test_textio.py exists in main branch (where our commit adds tests). But it was deleted in 3.13 branch and 3.14 branch because the tests are still in test_general.py |
|
GH-141452 is a backport of this pull request to the 3.13 branch. |
|
GH-141453 is a backport of this pull request to the 3.14 branch. |
…th standalone carriage return (pythonGH-141331) The assertion was checking wrong variable (skip_back vs skip_bytes). (cherry picked from commit af80fac) Co-authored-by: Mohsin Mehmood <55545648+mohsinm-dev@users.noreply.github.com>
|
GH-141452 is a backport of this pull request to the 3.13 branch. |
|
GH-141453 is a backport of this pull request to the 3.14 branch. |
|
IMO, these backports are correct and we can merge them. |
Summary
io.TextIOWrapper.tell()when reading files ending with standalone\rRoot Cause
The tell() optimization algorithm assumed buffered data would always be available when calculating position, but files ending with standalone
\rcreate empty snapshots, causingassert(skip_back <= PyBytes_GET_SIZE(next_input))to fail.Fix
Added check to skip optimization when
next_inputis empty, preventing the assertion failure while maintaining correct position calculation.Test Plan
test_tell_after_readline_with_crintest_textio.py_io_TextIOWrapper_tell_impl: Assertion 'skip_back <= PyBytes_GET_SIZE(next_input)' failed#141314