Skip to content
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

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

Merged
merged 27 commits into from Oct 17, 2018

Conversation

@matrixise
Copy link
Member

@matrixise matrixise commented May 21, 2017

@mention-bot
Copy link

@mention-bot mention-bot commented May 21, 2017

@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.

Copy link
Member

@zware zware left a comment

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.

self.assertEqual(f.tell(), size)

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

This comment has been minimized.

@zware

zware May 22, 2017
Member

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

This comment has been minimized.

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

This comment has been minimized.

@zware

zware May 22, 2017
Member

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.

This comment has been minimized.

@vstinner

vstinner May 22, 2017
Member

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

This comment has been minimized.

@vstinner

vstinner Oct 17, 2018
Member

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

@@ -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 comment has been minimized.

@zware

zware May 22, 2017
Member

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.

This comment has been minimized.

@vstinner

vstinner Oct 17, 2018
Member

fixed: bpo numbers are now clarified

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

This comment has been minimized.

@vstinner

vstinner May 22, 2017
Member

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

@@ -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)

This comment has been minimized.

@vstinner

vstinner May 22, 2017
Member

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.)

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

This comment has been minimized.

@vstinner

vstinner May 22, 2017
Member

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.

This comment has been minimized.

@zware

zware May 22, 2017
Member

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

This comment has been minimized.

@vstinner

vstinner Oct 17, 2018
Member

note for myself: the issue has been fixed

@matrixise matrixise force-pushed the matrixise:bpo-24658 branch from 5c333ac to 26e0e87 May 22, 2017
@matrixise
Copy link
Member Author

@matrixise matrixise commented May 22, 2017

  1. Change the comment about the support of OSX for _PY_READ_MAX and _PY_WRITE_MAX
  2. Add the clean up of the temporary file
@@ -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

This comment has been minimized.

@zware

zware May 22, 2017
Member

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

@matrixise matrixise closed this May 22, 2017
@matrixise matrixise reopened this May 22, 2017
.travis.yml Outdated
@@ -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"

This comment has been minimized.

@vstinner

vstinner May 23, 2017
Member

Please remove this unrelated change.

This comment has been minimized.

@matrixise

matrixise May 23, 2017
Author Member

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

@@ -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

This comment has been minimized.

@vstinner

vstinner May 23, 2017
Member

bigmmemtest is now unused, please remove it.

This comment has been minimized.

@matrixise

matrixise May 23, 2017
Author Member

yep, thanks for your feedback

This comment has been minimized.

@vstinner

vstinner Oct 17, 2018
Member

note for myself: bigmemtest is now used.

@@ -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 comment has been minimized.

@vstinner

vstinner May 23, 2017
Member

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

@zware
Copy link
Member

@zware zware commented Jun 4, 2017

@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
Copy link
Member

@vstinner vstinner commented Jun 4, 2017

@zware
Copy link
Member

@zware zware commented Jun 4, 2017

Ah, ok.

@brettcannon
Copy link
Member

@brettcannon brettcannon commented Feb 2, 2018

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.

@matrixise matrixise force-pushed the matrixise:bpo-24658 branch from 68b524f to fc08e60 Oct 17, 2018
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Oct 17, 2018

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

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Oct 17, 2018

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
Copy link
Contributor

@miss-islington miss-islington commented Oct 17, 2018

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request Oct 17, 2018
 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
Copy link

@bedevere-bot bedevere-bot commented Oct 17, 2018

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

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Oct 17, 2018

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
Copy link
Contributor

@miss-islington miss-islington commented Oct 17, 2018

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
Copy link
Member

@vstinner vstinner commented Oct 17, 2018

@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?

matrixise added a commit to matrixise/cpython that referenced this pull request Oct 17, 2018
…-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
Copy link

@bedevere-bot bedevere-bot commented Oct 17, 2018

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

vstinner added a commit that referenced this pull request Oct 17, 2018
…GH-9937)

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

(cherry picked from commit 74a8b6e)
matrixise added a commit to matrixise/cpython that referenced this pull request Oct 18, 2018
…-1705)

On macOS, fix reading from and writing into a file with a size larger
than 2 GiB.
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Oct 18, 2018

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

matrixise added a commit to matrixise/cpython that referenced this pull request Oct 18, 2018
…-1705)

On macOS, fix reading from and writing into a file with a size larger
than 2 GiB.
@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 18, 2018

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

miss-islington added a commit that referenced this pull request Oct 18, 2018
 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
Copy link
Member

@vstinner vstinner commented Oct 18, 2018

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 :-)

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request Oct 18, 2018
* 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)
  ...
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request Oct 18, 2018
* 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)
  ...
matrixise added a commit to matrixise/cpython that referenced this pull request Oct 25, 2018
…-1705)

On macOS, fix reading from and writing into a file with a size larger
than 2 GiB.
yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
 On macOS, fix reading from and writing into a file with a size larger than 2 GiB.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants