bpo-32228: Reset raw_pos after unwinding the raw stream #4858

nitishch · 2017-12-14T05:46:17Z

This issue was happening because - after write in the given case, write_pos and write_end are ending up to be 0. Nothing wrong here. But when doing truncate, we flush writer changes. During that, even when write_pos == write_end, we've to reset write_end to -1. That was not happening.

https://bugs.python.org/issue32228

…2228

methane · 2017-12-19T09:07:11Z

Would you add test case for it?

…2228 Pulled from latest master

nitishch · 2017-12-20T07:34:52Z

Thanks @methane. While writing a test case, I realised that my fix, although working, doesn't fix the root cause of the issue. Uploaded the new changes.

methane · 2017-12-20T10:59:48Z

Would you add NEWS file?
https://devguide.python.org/committing/?highlight=news%20#what-s-new-and-news-entries

methane · 2017-12-20T11:15:21Z

Hmm, Travis doesn't work... https://travis-ci.org/python/cpython/requests

methane · 2017-12-20T12:42:29Z

@nitishch Could you close this pull request and create new one with clean git log?

methane

LGTM, but I want IO expert review if possible.

pitrou · 2017-12-20T15:32:11Z

Lib/test/test_fileio.py

        self.assertEqual(f.seek(0, io.SEEK_END), 15)
        f.close()

+        f = io.open(TESTFN, 'r+b')


test_fileio is specifically for FileIO tests, but you are not testing FileIO here, but BufferedIORandom. So your test should go into the relevant test class in test_io.

Also, I think the test should exercise several different buffer sizes.

Done for both. See if it is ok now.

pitrou · 2017-12-20T15:35:21Z

Modules/_io/bufferedio.c

-    _bufferedwriter_reset_buf(self);

 end:
+    _bufferedwriter_reset_buf(self);


I'm a bit skeptical here. Shouldn't the fix go in buffered_flush_and_rewind_unlocked instead?

I thought when we are flushing out all dirty writes, we should reset write_end. This function is called in write, seek and in flush_and_rewind. It's usage appears ok to me in all three cases.

I'm just reading through code and trying to understand the behaviour. So I might have very well misunderstood something. Please correct me in that case.

Yes, you may well be right... I have not touched this code in a long time, so I prefer to ask.

A couple of things now:

can you add a comment here pointing to the issue number and explaning why this is necessary?

unrelated, but there's a comment at the beginning of _bufferedwriter_flush_unlocked mentioning restore_pos; that comment is obsolete, so it would be nice to remove it :-)

in _io_BufferedWriter_write_impl, there's a fix for issue #6629 which might have become unnecessary with this PR; could you check it?

pitrou

Thanks for tackling this! Buffered I/O is a delicate area. Please read the comments I left.

bedevere-bot · 2017-12-20T15:37:34Z

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.

nitishch · 2017-12-20T18:06:04Z

@methane Since this PR has the reviews, I'll address the reviews in this branch and then create a new PR if that's ok.

asvetlov · 2017-12-20T18:15:58Z

As an option you can rebase/squash/whatever and git push --force

methane · 2017-12-21T06:46:44Z

@nitishch And could you try log in on Travis-Ci?
Maybe, Travis blocks running build pull request from non Travis users.

…orarily

nitishch · 2017-12-22T11:43:34Z

@pitrou I added the test for 3 buffer sizes. Please see if it is ok now.

nitishch · 2017-12-22T11:56:21Z

Travis build seems to be running now for some reason :)

pitrou

Thank you for the update. See comments below.

pitrou · 2017-12-22T12:25:10Z

Lib/test/test_io.py

            self.assertEqual(f.read(), b"abc")

+    def test_truncate_after_write(self):
+        with self.open(support.TESTFN, "wb") as f:


Thank you. Can you add a comment referring to the bug entry at the beginning at the test?

pitrou · 2017-12-22T12:25:30Z

Lib/test/test_io.py

+                # read operation makes sure that pos != raw_pos
+                f.truncate()
+                self.assertEqual(f.tell(), buffer_size + 2)
+


There's an extraneous empty line here.

pitrou · 2017-12-22T12:29:04Z

Modules/_io/bufferedio.c

-    _bufferedwriter_reset_buf(self);

 end:
+    _bufferedwriter_reset_buf(self);


Yes, you may well be right... I have not touched this code in a long time, so I prefer to ask.

A couple of things now:

can you add a comment here pointing to the issue number and explaning why this is necessary?

unrelated, but there's a comment at the beginning of _bufferedwriter_flush_unlocked mentioning restore_pos; that comment is obsolete, so it would be nice to remove it :-)

in _io_BufferedWriter_write_impl, there's a fix for issue #6629 which might have become unnecessary with this PR; could you check it?

pitrou · 2017-12-22T12:29:32Z

Misc/NEWS.d/next/Library/2017-12-22-16-47-41.bpo-32228.waPx3q.rst

@@ -0,0 +1 @@
+Reset ``write_end`` value after ``truncate()``


Instead of describing the technical solution, can you describe the issue being fixed?

bedevere-bot · 2017-12-22T12:29:55Z

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.

nitishch · 2017-12-22T13:21:03Z

I have made the requested changes; please review again.

@pitrou It seems the fix for #6629 is needed. I just reset write_end to its proper value. The current fix doesn't perform any seeks on the raw stream. #6629 does the seek on the raw stream as per the comment. So I didn't remove it.

bedevere-bot · 2017-12-22T13:21:05Z

Thanks for making the requested changes!

@pitrou, @methane: please review the changes made to this pull request.

pitrou · 2017-12-22T14:03:20Z

Thanks @nitishch for the changes. The holiday period is arriving and I may not review this again before a few days. I hope you don't mind.

nitishch · 2018-01-28T02:07:46Z

Hi. Any updates on this?

pitrou · 2018-01-28T15:58:42Z

@nitishch thank you very much for doing this, and sorry for the delay. I think this PR is good to go now!

bedevere-bot · 2018-01-28T16:00:11Z

@pitrou: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2018-01-28T16:00:11Z

Thanks @nitishch for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington · 2018-01-28T16:01:14Z

Sorry, @nitishch and @pitrou, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 059f58ce938d9c3f0286412a4efb1b9131339421 3.6

…GH-4858) Ensure that ``truncate()`` preserves the file position (as reported by ``tell()``) after writes longer than the buffer size.. (cherry picked from commit 059f58c)

bedevere-bot · 2018-01-28T16:42:59Z

GH-5389 is a backport of this pull request to the 3.6 branch.

… (#5389) Ensure that ``truncate()`` preserves the file position (as reported by ``tell()``) after writes longer than the buffer size.. (cherry picked from commit 059f58c)

nitishch added 2 commits December 14, 2017 09:54

bpo-32228 Reset raw_pos after rewinding the stream

fc94685

Merge branch 'master' of https://github.com/python/cpython into bpo-3…

d7e0d99

…2228

the-knights-who-say-ni added the CLA signed label Dec 14, 2017

bedevere-bot added the awaiting review label Dec 14, 2017

nitishch added 2 commits December 20, 2017 12:26

bpo-32228 Reset write_end value even when write_pos == write_end

024921f

Merge branch 'master' of https://github.com/python/cpython into bpo-3…

0eba307

…2228 Pulled from latest master

methane closed this Dec 20, 2017

methane reopened this Dec 20, 2017

methane requested a review from pitrou December 20, 2017 13:36

methane approved these changes Dec 20, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 20, 2017

pitrou reviewed Dec 20, 2017

View reviewed changes

pitrou requested changes Dec 20, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 20, 2017

nitishch added 3 commits December 22, 2017 14:53

bpo-32228 Removed one unnecessary call and also removed the test temp…

4c9de0e

…orarily

bpo-32228 Added other buffer_sizes to the test

81fc499

bpo-32228 Added NEWS entry

472eefe

pitrou requested changes Dec 22, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Dec 22, 2017

pitrou added needs backport to 2.7 type-bug An unexpected behavior, bug, or error labels Dec 22, 2017

bpo-32228 Added some comments in code and in test files

b83eac0

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 22, 2017

pitrou approved these changes Jan 28, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 28, 2018

pitrou removed the needs backport to 2.7 label Jan 28, 2018

pitrou merged commit 059f58c into python:master Jan 28, 2018

bedevere-bot removed the awaiting merge label Jan 28, 2018

bedevere-bot removed the needs backport to 3.6 label Jan 28, 2018

Uh oh!

bpo-32228: Reset raw_pos after unwinding the raw stream #4858

bpo-32228: Reset raw_pos after unwinding the raw stream #4858

Uh oh!

Conversation

nitishch commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane commented Dec 19, 2017

Uh oh!

nitishch commented Dec 20, 2017

Uh oh!

methane commented Dec 20, 2017

Uh oh!

methane commented Dec 20, 2017

Uh oh!

methane commented Dec 20, 2017

Uh oh!

methane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 20, 2017

Uh oh!

nitishch commented Dec 20, 2017

Uh oh!

asvetlov commented Dec 20, 2017

Uh oh!

methane commented Dec 21, 2017

Uh oh!

nitishch commented Dec 22, 2017

Uh oh!

nitishch commented Dec 22, 2017

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 22, 2017

Uh oh!

nitishch commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Dec 22, 2017

Uh oh!

pitrou commented Dec 22, 2017

Uh oh!

nitishch commented Jan 28, 2018

Uh oh!

pitrou commented Jan 28, 2018

Uh oh!

bedevere-bot commented Jan 28, 2018

Uh oh!

miss-islington commented Jan 28, 2018

Uh oh!

miss-islington commented Jan 28, 2018

Uh oh!

bedevere-bot commented Jan 28, 2018

Uh oh!

nitishch commented Dec 14, 2017 •

edited

Loading

nitishch commented Dec 22, 2017 •

edited

Loading