bpo-36411: Python 3 f.tell() gets out of sync with file pointer in binary append+read mode by websurfer5 · Pull Request #13717 · python/cpython (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

websurfer5

Implement buffered tell() as a wrapper for buffered seek(0, io.SEEK_CUR) to fix a problem with the internally maintained file position getting out of sync with the kernel when using read/write append mode. The fix applies to both binary and text files.

https://bugs.python.org/issue36411

@websurfer5

auvipy

@websurfer5

text file for read/write append. This is required for Windows systems.

ZackerySpytz

@websurfer5

benjaminp

Choose a reason for hiding this comment

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

Won't this break calling tell() on buffered streams wrapping raw io that supports tell() but not seek()?

*/
CHECK_INITIALIZED(self)
PyObject *z_obj = PyLong_FromLong(0);

Choose a reason for hiding this comment

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

This looks like it leaks a reference to z_obj.

@bedevere-bot

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pewscorner

Wouldn't it be possible to use the old buffered_tell() code if the stream doesn't support seek()? As far as I know, the bug only happens after seeking, so it ought to be fine to fall back to the old approach when seeking isn't possible, I would imagine.