bpo-32236: Issue RuntimeWarning if buffering=1 for open() in binary mode by izbyshev · Pull Request #4842 · python/cpython

izbyshev · 2017-12-13T16:33:04Z

If buffering=1 is specified for open() in binary mode, it is silently
treated as buffering=-1 (i.e., the default buffer size).
Coupled with the fact that line buffering is always supported in Python 2,
such behavior caused several issues (e.g., bpo-10344, bpo-21332).

Raise ValueError on attempts to use line buffering in binary mode to prevent
possible bugs and expose existing buggy code.

https://bugs.python.org/issue32236

If buffering=1 is specified for open() in binary mode, it is silently treated as buffering=-1 (i.e., the default buffer size). Coupled with the fact that line buffering is always supported in Python 2, such behavior caused several issues (e.g., bpo-10344, bpo-21332). Raise ValueError on attempts to use line buffering in binary mode to prevent possible bugs and expose existing buggy code.

gpshead

The subprocess related changes are correct, others should review the other changes. Though at first glance I believe those to also be correct.

vstinner

Please add a NEWS entry to mention that open() now emits a warning if the file is opened in binary mode with buffering=1. Moreover, the NEWS entry should mention that the change fixes a performance issue in subprocess.Popen if buffering=1: the underlying binary file is no longer opened with inefficient buffering=1, if I understood correctly.

vstinner · 2018-09-11T00:17:14Z

Lib/test/test_file.py

+        try:
+            f = self.open(TESTFN, 'wb', s)
+            f.write(str(s).encode("ascii"))
+            f.close()


what's the point of calling close() twice here? with should be used, no?

This is just a refactor of an existing test.

vstinner · 2018-09-11T00:17:48Z

Lib/test/test_file.py

-            except OSError as msg:
-                self.fail('error setting buffer size %d: %s' % (s, str(msg)))
-            self.assertEqual(d, s)
+        for s in (-1, 0, 512):


Would it be possible to check that these calls don't emit RuntimeWarning?

I've added the check.

bedevere-bot · 2018-09-11T00:20:56Z

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.

pitrou

Thanks. You need to add a NEWS file using the blurb utility. Also, see comments below.

pitrou · 2018-09-11T08:41:13Z

Lib/_pyio.py

        raise ValueError("binary mode doesn't take a newline argument")
+    if binary and buffering == 1:
+        import warnings
+        warnings.warn("line buffering is not supported in binary mode",


It's not necessary obvious why the warning talks about line buffering, so I would be a bit more explicit: "line buffering (buffering = 1) isn't supported in binary mode, file will be fully buffered."

OK, I've clarified the message as you suggest.

pitrou · 2018-09-11T08:45:29Z

Lib/test/test_file.py

+        try:
+            f = self.open(TESTFN, 'wb', s)
+            f.write(str(s).encode("ascii"))
+            f.close()


This is just a refactor of an existing test.

pitrou · 2018-09-11T08:46:15Z

Lib/test/test_io.py

            self.large_file_ops(f)

+    def _checked_open(self, path, mode, bufsize, under_check=False):
+        if not under_check and bufsize == 1:


What is under_check for? It doesn't seem to be ever True.

This function was a conditional context manager, and under_check is set to True in the recursive call. But I've removed that function anyway to have only one place with warning check (in test_file.py).

pitrou · 2018-09-11T08:47:42Z

Lib/test/test_file.py

+        for s in (-1, 0, 512):
+            self._checkBufferSize(s)
+
+        # test that attempts to use line buffering in binary mode cause


No need to test this both here and in test_io.py, IMHO.

OK, I've preserved only one check here. I think that test_with_open in test_io.py wasn't a really good place for that check after all.

izbyshev · 2018-09-11T22:20:25Z

@gpshead, @vstinner, @pitrou Thank you for your reviews! I have made the requested changes; please review again.

Also, should the docs be changed to reflect that buffering=1 is equivalent to buffering=-1 in binary mode? Given current docs, the effect of buffering=1 is unclear.

bedevere-bot · 2018-09-11T22:20:27Z

Thanks for making the requested changes!

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

izbyshev · 2018-09-11T22:22:56Z

@vstinner Regarding the NEWS, there was no performance issue with buffering=1 in binary mode because it was treated as buffering=-1. It was rather a potential correctness problem, especially for code that was originally written for Python 2 and that explicitly depends on line buffering.

izbyshev · 2018-09-11T22:35:08Z

@vstinner Sorry, I misread your comment -- it was about subprocess.Popen. Since the underlying code simply calls io.open, which treats buffering=1 as buffering=-1 in binary mode, there is no performance issue here too.

My previous understanding of behavior of check_no_resource_warning(), which check_no_warnings() is based on, was incorrect.

izbyshev · 2018-10-15T23:14:41Z

@pitrou @vstinner Ping!

vstinner

It is now way better, but I still have a few comments.

vstinner · 2018-10-16T09:08:54Z

Lib/_pyio.py

+    if binary and buffering == 1:
+        import warnings
+        warnings.warn("line buffering (buffering=1) isn't supported in binary "
+                      "mode, file will be fully buffered", RuntimeWarning, 2)


There is no such thing as "fully buffered". I propose to also say "the default size will be used" here.

Extract of the code:

if buffering == 1 or buffering < 0 and raw.isatty(): buffering = -1 line_buffering = True if buffering < 0: buffering = DEFAULT_BUFFER_SIZE

buffering=1 becomes buffering=DEFAULT_BUFFER_SIZE if I understand correctly.

Fixed, thanks!

vstinner · 2018-10-16T09:13:13Z

Lib/subprocess.py

+        if self.text_mode:
+            if bufsize == 1:
+                line_buffering = True
+                # line buffering is not supported in binary mode


But here we are in text mode, no? The comment is misleading, I suggest to remove it.

Or say something like: "# Use the default buffer size for the binary buffered reader/writer".

I was referring to the binary mode used for the underlying streams, and I didn't realize that it can be misleading, but now I see it. Changed to clearer wording, thanks!

vstinner · 2018-10-16T09:17:29Z

Modules/_io/_iomodule.c

+    if (binary && buffering == 1) {
+        if (PyErr_WarnEx(PyExc_RuntimeWarning,
+                         "line buffering (buffering=1) isn't supported in "
+                         "binary mode, file will be fully buffered", 1) < 0)


Ditto: say something like "the default size will be used".

PEP 7: please add { ... } around the "goto error;". By the way, see https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/ :-)

Yes, I'm aware of PEP 7 and goto fail bug, but I usually follow the rule of local consistency, and a similar code above doesn't use brackets. Now I've added brackets here as you suggested.

vstinner · 2018-10-16T09:20:08Z

Misc/NEWS.d/next/Core and Builtins/2018-09-11-23-50-40.bpo-32236.3RupnN.rst

@@ -0,0 +1,2 @@
+Warn that line buffering is not supported if :func:`open` is called with
+binary mode and ``buffering=1``.


I would prefer to be more explicit and list more impact functions: open, io.open, codecs.open. Would it be possible to put functions first in the sentence? Something like: "open(), io.open(), codecs.open() now emit a warning if the file is opened in binary mode with buffering=1." So the reader directly gets the context.

Some other functions are impacted too, like pathlib.Path.open() and tempfile.TemporaryFile() (and some other classes from tempfile). Do you think we should find and list all such functions here? Maybe we should just provide a couple of examples: io.open and the standard library functions relying on it (e.g., codecs.open, pathlib.Path.open) now issue a warning if a file is opened in binary mode with buffering=1.

bedevere-bot · 2018-10-16T09:20: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.

And if you don't make the requested changes, you will be put in the comfy chair!

The PR has been updated

vstinner · 2018-10-20T00:20:12Z

I ran the full test suite on this change using "PYTHONWARNINGS=error ./python -W error -m test -j0 -u all,-gui -r" to check if any stdlib module misuse open(). Good news: "Tests result: SUCCESS".

vstinner · 2018-10-20T00:23:27Z

Thanks @izbyshev for your tenacity :-) Thanks also @gpshead and @pitrou for your reviews.

izbyshev requested a review from gpshead as a code owner December 13, 2017 16:33

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

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

bpo-32236: Issue RuntimeWarning instead of raising ValueError

888ee4d

izbyshev changed the title ~~bpo-32236: Don't allow buffering=1 for open() in binary mode~~ bpo-32236: Issue RuntimeWarning if buffering=1 for open() in binary mode Dec 18, 2017

gpshead approved these changes Sep 10, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Sep 10, 2018

gpshead requested a review from pitrou September 10, 2018 23:58

gpshead assigned pitrou Sep 10, 2018

vstinner requested changes Sep 11, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 11, 2018

pitrou previously requested changes Sep 11, 2018

View reviewed changes

Alexey Izbyshev added 6 commits September 11, 2018 21:07

Clarify warning message

0deefe1

Remove RuntimeWarning test from test_io.py

42a43c1

Fix check for warning message

164470a

Check that no warning is emitted when buffering != 1

54c12b2

Add NEWS entry

3fe49c4

Fix warning message check in test_subprocess.py

aff54cf

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 11, 2018

Alexey Izbyshev added 2 commits September 12, 2018 02:23

Update docs for codecs.open()

cec8cd7

Fix check_no_warnings() docs to reflect what it actually does

64dfd42

My previous understanding of behavior of check_no_resource_warning(), which check_no_warnings() is based on, was incorrect.

izbyshev force-pushed the bpo-32236 branch from 99d1f03 to 64dfd42 Compare September 12, 2018 09:51

vstinner requested changes Oct 16, 2018

View reviewed changes

bedevere-bot removed the awaiting change review label Oct 16, 2018

bedevere-bot added the awaiting changes label Oct 16, 2018

Alexey Izbyshev added 2 commits October 19, 2018 00:28

Clarify warning message

2538bde

Clarify a comment

250adb4

vstinner approved these changes Oct 20, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 20, 2018

vstinner merged commit a267056 into python:master Oct 20, 2018

bedevere-bot removed the awaiting merge label Oct 20, 2018

izbyshev deleted the bpo-32236 branch November 3, 2018 13:59

SantiagoPiccinini mannequin mentioned this pull request Apr 10, 2022

codecs.open() buffering doc needs fix #54553

Closed

		@@ -0,0 +1,2 @@
		Warn that line buffering is not supported if :func:`open` is called with
		binary mode and ``buffering=1``.

Uh oh!

Conversation

izbyshev commented Dec 13, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner 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 Sep 11, 2018

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!

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!

izbyshev commented Sep 11, 2018

Uh oh!

bedevere-bot commented Sep 11, 2018

Uh oh!

izbyshev commented Sep 11, 2018

Uh oh!

izbyshev commented Sep 11, 2018

Uh oh!

izbyshev commented Oct 15, 2018

Uh oh!

vstinner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Oct 16, 2018

Uh oh!

vstinner commented Oct 20, 2018

Uh oh!

vstinner commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

izbyshev commented Dec 13, 2017 •

edited by bedevere-bot

Loading

vstinner left a comment •

edited

Loading