bpo-24658: Fix read/write on file with a size greater than 2GB on OSX #1705

matrixise · 2017-05-21T21:15:25Z

https://bugs.python.org/issue24658

mention-bot · 2017-05-21T21:15:28Z

@matrixise, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @zooba and @benjaminp to be potential reviewers.

zware

Needs a couple of minor changes, but otherwise looks pretty good. I'll test it on my machine once Travis and AppVeyor are happy with it.

zware · 2017-05-22T00:21:13Z

Lib/test/test_largefile.py

+            self.assertEqual(f.tell(), size)
+
+        with self.open(TESTFN, "rb") as f:
+            self.assertEqual(f.read(size), size)


This needs to be len(f.read(size)).

zware · 2017-05-22T00:23:53Z

Lib/test/test_largefile.py

+        with self.open(TESTFN, "wb") as f:
+            b = b'x' * size
+            self.assertEqual(f.write(b), size)
+            self.assertEqual(f.tell(), size)


Since this test is writing the full file, it will need to remove it as well so that the other tests will be set up as expected.

Call self.addCleanup(support.unlink, TESTFN) before creating the file.

Fixed indirectly, the test now reuses LargeFileTest.setUp().

zware · 2017-05-22T00:29:07Z

Include/fileutils.h

@@ -19,6 +19,17 @@ PyAPI_FUNC(char*) Py_EncodeLocale(

 PyAPI_FUNC(PyObject *) _Py_device_encoding(int);

+#if defined(MS_WINDOWS) || defined(__APPLE__)
+/* On Windows, the count parameter of read() is an int
+   See issue #24658


This makes it sound like Windows is dealt with by bpo-24658, but it's actually macOS in that issue. There should be some brief text added about macOS. If you can dig up an issue number about Windows, that could be added as well.

fixed: bpo numbers are now clarified

vstinner · 2017-05-22T05:24:35Z

Lib/test/test_largefile.py

+        with self.open(TESTFN, "wb") as f:
+            b = b'x' * size
+            self.assertEqual(f.write(b), size)
+            self.assertEqual(f.tell(), size)


Call self.addCleanup(support.unlink, TESTFN) before creating the file.

vstinner · 2017-05-22T05:26:45Z

Lib/test/test_largefile.py

@@ -45,6 +45,20 @@ def tearDownClass(cls):
            raise cls.failureException('File was not truncated by opening '
                                       'with mode "wb"')

+    @bigmemtest(size=_2G + 1024 * 1024, memuse=2)


Are you sure about the memuse=2 parameter? I would only expect memuse=1. Try maybe tracemalloc to get the memory peak: run tests using python3 -X tracemalloc ... and then display tracemalloc.get_traced_memory()[1].

https://docs.python.org/dev/library/tracemalloc.html#tracemalloc.get_traced_memory

(You can run such test on Linux.)

vstinner · 2017-05-22T05:34:32Z

Modules/_io/fileio.c

-    if (size > INT_MAX)
-        size = INT_MAX;
-#endif
+    if (size > _PY_READ_MAX) {


I'm not sure that it's ok to use this if on OS other than macOS and Windows, since size type is ssize_t. I expect "condition is always false" warnings on many recent compilers. I would prefer to keep an #ifdef.

The #ifdef is just moved to fileutils.h, where _PY_READ_MAX is defined.

note for myself: the issue has been fixed

matrixise · 2017-05-22T17:13:34Z

Change the comment about the support of OSX for _PY_READ_MAX and _PY_WRITE_MAX
Add the clean up of the temporary file

zware · 2017-05-22T17:17:39Z

Include/fileutils.h

@@ -21,7 +21,7 @@ PyAPI_FUNC(PyObject *) _Py_device_encoding(int);

 #if defined(MS_WINDOWS) || defined(__APPLE__)
 /* On Windows, the count parameter of read() is an int
-   See issue #24658
+   Add the support of MacOS with the issue #24658


"macOS fails when reading or writing more than 2GB, see bpo-24658"

vstinner · 2017-05-23T07:58:02Z

.travis.yml

@@ -81,7 +81,7 @@ before_script:

 script:
  # `-r -w` implicitly provided through `make buildbottest`.
-  - make buildbottest TESTOPTS="-j4"
+  - make buildbottest TESTOPTS="-j4 -v test_largefile"


Please remove this unrelated change.

yep +1, just for some tests, but I will rebase my branch before the final PR.

vstinner · 2017-05-23T07:58:19Z

Lib/test/test_largefile.py

@@ -5,7 +5,7 @@
 import stat
 import sys
 import unittest
-from test.support import TESTFN, requires, unlink
+from test.support import TESTFN, requires, unlink, bigmemtest, _2G


bigmmemtest is now unused, please remove it.

yep, thanks for your feedback

note for myself: bigmemtest is now used.

vstinner · 2017-05-23T07:58:53Z

Lib/test/test_largefile.py

@@ -45,6 +45,25 @@ def tearDownClass(cls):
            raise cls.failureException('File was not truncated by opening '
                                       'with mode "wb"')

+    def test_large_reads_writes(self):
+        # see issue #24658
+        requires('largefile',


This is redundant with what is done in the module "main" code.

zware · 2017-06-04T19:34:58Z

@matrixise Looks like the current Travis failure is due to bigmemtest not being imported; otherwise, have you had any luck tracking down why it had been segfaulting?

vstinner · 2017-06-04T22:17:37Z

I am not aware of segfault, only of process *killed* on Travis which probably means that the test uses too much resources (memory?).

zware · 2017-06-04T22:21:31Z

Ah, ok.

brettcannon · 2018-02-02T22:04:57Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

bpo-24658: Fix read/write on file with a size greater than 2GB on OSX

…X and _PY_WRITE_MAX

miss-islington · 2018-10-17T23:05:45Z

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

miss-islington · 2018-10-17T23:05:45Z

Thanks @matrixise for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2018-10-17T23:05:45Z

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

On macOS, fix reading from and writing into a file with a size larger than 2 GiB. (cherry picked from commit 74a8b6e) Co-authored-by: Stéphane Wirtel <stephane@wirtel.be>

bedevere-bot · 2018-10-17T23:06:00Z

GH-9936 is a backport of this pull request to the 3.7 branch.

miss-islington · 2018-10-17T23:06:01Z

Sorry, @matrixise and @vstinner, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 74a8b6ea7e0a8508b13a1c75ec9b91febd8b5557 2.7

miss-islington · 2018-10-17T23:06:06Z

Sorry, @matrixise and @vstinner, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 74a8b6ea7e0a8508b13a1c75ec9b91febd8b5557 3.6

vstinner · 2018-10-17T23:07:54Z

@matrixise: @miss-islington did the 3.7 backport, but failed to create 2.7 and 3.6 backports. Can you please try to backport your change to these branches?

…-1705) On macOS, fix reading from and writing into a file with a size larger than 2 GiB. (cherry picked from commit 74a8b6e)

bedevere-bot · 2018-10-17T23:27:19Z

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

…GH-9937) On macOS, fix reading from and writing into a file with a size larger than 2 GiB. (cherry picked from commit 74a8b6e)

bedevere-bot · 2018-10-18T00:22:38Z

GH-9938 is a backport of this pull request to the 2.7 branch.

serhiy-storchaka · 2018-10-18T04:28:09Z

Good work! I like to observe how you collaborated, @matrixise and @vstinner!

On macOS, fix reading from and writing into a file with a size larger than 2 GiB. (cherry picked from commit 74a8b6e) Co-authored-by: Stéphane Wirtel <stephane@wirtel.be>

vstinner · 2018-10-18T08:34:16Z

Good work! I like to observe how you collaborated, @matrixise and @vstinner!

FYI you chatted on IRC, that's @matrixise pushed so many small commits quickly :-)

* master: (621 commits) Update opcode.h header comment to mention the source data file (pythonGH-9935) bpo-34936: Fix TclError in tkinter.Spinbox.selection_element(). (pythonGH-9760) Updated documentation on logging.debug(). (pythonGH-9946) bpo-34765: Update the install-sh file (pythonGH-9592) bpo-35008: Fix possible leaks in Element.__setstate__(). (pythonGH-9924) bpo-35011: Restore use of pyexpatns.h in libexpat (pythonGH-9939) bpo-24658: Fix read/write greater than 2 GiB on macOS (pythonGH-1705) Add missing comma to wsgiref doc (pythonGH-9932) bpo-23420: Verify the value of '-s' when execute the CLI of cProfile (pythonGH-9925) Doc: Fix is_prime (pythonGH-9909) In email docs, correct spelling of foregoing (python#9856) In email.parser in message_from_bytes, update `strict` to `policy` (python#9854) bpo-34997: Fix test_logging.ConfigDictTest.test_out_of_order (pythonGH-9913) Added CLI starter example to logging cookbook. (pythonGH-9910) bpo-34783: Fix test_nonexisting_script() (pythonGH-9896) bpo-23554: Change echo server example class name from EchoServerClientProtocol to EchoServerProtocol (pythonGH-9859) bpo-34989: python-gdb.py: fix current_line_num() (pythonGH-9889) Stop using deprecated logging API in Sphinx suspicious checker (pythonGH-9875) fix dangling keyfunc examples in documentation of heapq and sorted (python#1432) bpo-34844: logging.Formatter enhancement - Ensure style and format string matches in logging.Formatter (pythonGH-9703) ...

* master: (1787 commits) Update opcode.h header comment to mention the source data file (pythonGH-9935) bpo-34936: Fix TclError in tkinter.Spinbox.selection_element(). (pythonGH-9760) Updated documentation on logging.debug(). (pythonGH-9946) bpo-34765: Update the install-sh file (pythonGH-9592) bpo-35008: Fix possible leaks in Element.__setstate__(). (pythonGH-9924) bpo-35011: Restore use of pyexpatns.h in libexpat (pythonGH-9939) bpo-24658: Fix read/write greater than 2 GiB on macOS (pythonGH-1705) Add missing comma to wsgiref doc (pythonGH-9932) bpo-23420: Verify the value of '-s' when execute the CLI of cProfile (pythonGH-9925) Doc: Fix is_prime (pythonGH-9909) In email docs, correct spelling of foregoing (python#9856) In email.parser in message_from_bytes, update `strict` to `policy` (python#9854) bpo-34997: Fix test_logging.ConfigDictTest.test_out_of_order (pythonGH-9913) Added CLI starter example to logging cookbook. (pythonGH-9910) bpo-34783: Fix test_nonexisting_script() (pythonGH-9896) bpo-23554: Change echo server example class name from EchoServerClientProtocol to EchoServerProtocol (pythonGH-9859) bpo-34989: python-gdb.py: fix current_line_num() (pythonGH-9889) Stop using deprecated logging API in Sphinx suspicious checker (pythonGH-9875) fix dangling keyfunc examples in documentation of heapq and sorted (python#1432) bpo-34844: logging.Formatter enhancement - Ensure style and format string matches in logging.Formatter (pythonGH-9703) ...

…-1705) On macOS, fix reading from and writing into a file with a size larger than 2 GiB.

the-knights-who-say-ni added the CLA signed label May 21, 2017

zware previously requested changes May 22, 2017

View reviewed changes

vstinner requested changes May 22, 2017

View reviewed changes

matrixise force-pushed the bpo-24658 branch from 5c333ac to 26e0e87 Compare May 22, 2017 16:56

zware reviewed May 22, 2017

View reviewed changes

matrixise closed this May 22, 2017

matrixise reopened this May 22, 2017

vstinner reviewed May 23, 2017

View reviewed changes

brettcannon added the awaiting changes label Feb 2, 2018

veprbl mentioned this pull request Apr 27, 2018

python35, python36: fix reading large files on darwin NixOS/nixpkgs#39627

Merged

8 tasks

matrixise added 12 commits October 17, 2018 14:33

bpo-24658: Fix read/write on file with a size greater than 2GB on OSX

f810129

bpo-24658: Fix read/write on file with a size greater than 2GB on OSX

bpo-24658: Clean the test file for test_large_reads_writes

1b7d412

bpo-24658: Improve the comment for the support of OSX for _PY_READ_MA…

3c447d8

…X and _PY_WRITE_MAX

bpo-24658: Improve the comment about macOS

b5b2754

bpo-24658: Add the bigmemtest decorator for the test

1130c44

bpo-24658: just a test

207f00f

bpo-24658: just a test

7064bd5

bpo-24658: just a test

84b09f9

bpo-24658: remove the requires and fix the comment

a8c0aa1

bpo-24658: rename the test method

3d7fe38

bpo-24658: remove unused imports

bb3e082

bpo-24658: check with bigmemtest

fc08e60

matrixise force-pushed the bpo-24658 branch from 68b524f to fc08e60 Compare October 17, 2018 12:35

Add blurb entry

17d0d04

vstinner merged commit 74a8b6e into python:master Oct 17, 2018

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

vstinner added needs backport to 3.6 labels Oct 17, 2018

bedevere-bot removed the needs backport to 3.7 label Oct 17, 2018

miss-islington assigned vstinner Oct 17, 2018

bedevere-bot removed the needs backport to 3.6 label Oct 17, 2018

vstinner pushed a commit that referenced this pull request Oct 17, 2018

[3.6] bpo-24658: Fix read/write greater than 2 GiB on macOS (GH-1705) (…

a5ebc20

…GH-9937) On macOS, fix reading from and writing into a file with a size larger than 2 GiB. (cherry picked from commit 74a8b6e)

bedevere-bot removed the needs backport to 2.7 label Oct 18, 2018

matrixise mentioned this pull request Oct 19, 2018

ERROR: test_big_arrays (test_io.TestSavezLoad) on OS X + Python 3.3 numpy/numpy#3858

Closed

matrixise added a commit to matrixise/cpython that referenced this pull request Oct 25, 2018

[2.7] bpo-24658: Fix read/write greater than 2 GiB on macOS (pythonGH…

5e6aa97

…-1705) On macOS, fix reading from and writing into a file with a size larger than 2 GiB.

bpo-24658: Fix read/write on file with a size greater than 2GB on OSX #1705

bpo-24658: Fix read/write on file with a size greater than 2GB on OSX #1705

Conversation

matrixise commented May 21, 2017 • edited by bedevere-bot Loading

mention-bot commented May 21, 2017

zware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matrixise commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zware commented Jun 4, 2017

vstinner commented Jun 4, 2017 via email

zware commented Jun 4, 2017

brettcannon commented Feb 2, 2018

miss-islington commented Oct 17, 2018

miss-islington commented Oct 17, 2018

miss-islington commented Oct 17, 2018

bedevere-bot commented Oct 17, 2018

miss-islington commented Oct 17, 2018

miss-islington commented Oct 17, 2018

vstinner commented Oct 17, 2018

bedevere-bot commented Oct 17, 2018

bedevere-bot commented Oct 18, 2018

serhiy-storchaka commented Oct 18, 2018

vstinner commented Oct 18, 2018

matrixise commented May 21, 2017 •

edited by bedevere-bot

Loading