bpo-34563: Fix for invalid assert on big output of multiprocessing.Process by ahcub · Pull Request #9027 · python/cpython

ahcub · 2018-09-01T17:15:10Z

https://bugs.python.org/issue34563

the-knights-who-say-ni · 2018-09-01T17:15:13Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

ahcub · 2018-09-01T17:19:33Z

I signed the CLA, can someone check what is the issue with it?

Mariatta · 2018-09-01T17:56:18Z

Hi, it can take at least one business day for it to be reflected on our system. Thanks.

ahcub · 2018-09-01T17:58:38Z

ah, didn't know that, thank you!

ahcub · 2018-09-01T18:22:37Z

I get an error on Travis CI Build but I'm not sure how to fix it. It goes as the following.

Error in file "./Modules/_winapi.c" on line 1365:
Checksum mismatch!
Expected: 492029ca98161d84
Computed: d3d5b44a8201b944
Suggested fix: remove all generated code including the end marker,
or use the '-f' option.

but I don't have any generated code
Any advice?

ahcub · 2018-09-01T18:25:19Z

oh, maybe I do have changes in the generated code, hm, probably I need to change the clinic input instead of the function header

…nput instead

ahcub · 2018-09-01T18:36:21Z

hm, now I'm getting an error that generated files are not up to date

$ # Check for changes in regenerated files
  if ! test -z "$changes"
  then
    echo "Generated files not up to date"
    echo "$changes"
    exit 1
  fi
  
Generated files not up to date
 M Modules/_winapi.c
 M Modules/clinic/_winapi.c.h

I'm not sure what I did wrong this time
Any advice?

ahcub · 2018-09-01T21:39:01Z

ok, looks like there was a requirement for a test in the change, I added a python test case for this issue

ahcub · 2018-09-01T21:43:30Z

seems like non-python test is required here, hm

ahcub · 2018-09-01T21:59:22Z

ok, I understand what is required but I don't understand how can I write a test for the changes I've made
I tried to search for examples on pythoncore C tests, but I cannot see any, and it should be a generated test also if I read the travis.yml correctly as it must be in git status --porcelain cmd output
is there any guide or example I can take inspiration from?

latest commits into _winapi.c don't contain any C test files that would look like the ones I need...

ahcub · 2018-09-01T22:43:17Z

@vstinner sorry for tagging, but can you help?

vstinner

Honestly, I'm not sure that it's worth it to write an unit test allocating 2 GiB of memory and writing 2 GiB of output into a stdout pipe. I propose to remove the test.

vstinner · 2018-09-03T09:41:54Z

@@ -0,0 +1 @@
+Fix for invalid assert on big output of multiprocessing.Process


You should mention _winapi.PeekNamedPipe() and _winapi.ReadFile(). Maybe:

Fix _winapi.PeekNamedPipe() and _winapi.ReadFile() for read larger than INT_MAX (usually 2^31-1).

I'm not sure that it's worth it to mention multiprocessing in the NEWS entry.

_winapi is a private module. _winapi.PeekNamedPipe() and _winapi.ReadFile() are used only in multiprocessing.connection. It is an implementation detail. The end user uses multiprocessing.

Ok, so maybe mention both: _winapi and multiprocessing.Connection fixes. Example:

On Windows, fix multiprocessing.Connection for very large read: fix _winapi.PeekNamedPipe() and _winapi.ReadFile() for read larger than INT_MAX (usually 2^31-1).

I agree with serhiy-storchaka

serhiy-storchaka · 2018-09-02T21:21:57Z

+
+
+class TestMultiprocessingWithBigOutput(unittest.TestCase):
+    @unittest.skip("skipping test for manual use only. Highly demanding on RAM and time consuming")


Use the test.support.bigmemtest decorator.

serhiy-storchaka · 2018-09-03T09:57:29Z

@@ -0,0 +1 @@
+Fix for invalid assert on big output of multiprocessing.Process


_winapi is a private module. _winapi.PeekNamedPipe() and _winapi.ReadFile() are used only in multiprocessing.connection. It is an implementation detail. The end user uses multiprocessing.

pitrou

Here are a couple comments. Also, did you make sure the test also passes on Unix, or should someone else test it for you?

pitrou · 2018-09-03T10:32:37Z

@@ -0,0 +1,25 @@
+import unittest


All multiprocessing tests should go into _test_multiprocessing.py, so that they get executed under the different execution contexts ("fork", "forkserver", "spawn").

pitrou · 2018-09-03T10:33:03Z

+
+
+def func():
+    return 'test' * (10 ** 9)


Please use a binary string (b"test"), and add a comment stating the expected memory consumption.

pitrou · 2018-09-03T10:34:58Z


    handle: HANDLE
-    size: int
+    size: DWORD


Hmm... how come you changed this and the generated code didn't change? Did you run make clinic to update all files?

no, sorry, I was not aware of the process and I thought that the files are regenerated on the build somehow, I committed the changes to the generated section

the checks passed this time it seems, can you recheck if all good @pitrou?

bedevere-bot · 2018-09-03T10:35:49Z

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.

ahcub · 2018-09-03T10:48:38Z

I removed the test because I agree with @vstinner comment about it, it is redundant

pitrou · 2018-09-03T18:44:31Z

Thanks @ahcub! This looks good to me now.

pitrou · 2018-09-03T18:45:28Z

Waiting for the CLA to be validated now.

vstinner · 2018-09-03T20:26:58Z

FYI @ahcub bugs.python.org account is: https://bugs.python.org/user29366 and I don't see "Contributor Form Received" checked yet. We have to wait for the manual CLA process.

ahcub · 2018-09-03T22:15:34Z

Thanks for your prompt review and guidance, guys.
I hope they will process my CLA soon

ahcub · 2018-09-04T14:25:53Z

The CLA is accepted, is it possible to merge it in today? @pitrou @vstinner

vstinner · 2018-09-04T16:10:35Z

Done ;-)

miss-islington · 2018-09-04T16:10:57Z

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

miss-islington · 2018-09-04T16:10:57Z

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

miss-islington · 2018-09-04T16:11:02Z

Sorry, @ahcub 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 266f4904a222a784080e29aad0916849e507515d 3.6

bedevere-bot · 2018-09-04T16:11:12Z

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

…ocess (pythonGH-9027) Fix for invalid assert on big output of multiprocessing.Process. (cherry picked from commit 266f490) Co-authored-by: Alexander Buchkovsky <olex.buchkovsky@gmail.com>

ahcub · 2018-09-04T17:04:41Z

thanks, I will check the backport to 3.6

…ocess (GH-9027) (GH-9064) Fix for invalid assert on big output of multiprocessing.Process. (cherry picked from commit 266f490) Co-authored-by: Alexander Buchkovsky <olex.buchkovsky@gmail.com>

…ing.Process (pythonGH-9027) Fix for invalid assert on big output of multiprocessing.Process. (cherry picked from commit 266f490)

bedevere-bot · 2018-09-05T01:28:28Z

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

…ing.Process (GH-9027) (GH-9069) Fix for invalid assert on big output of multiprocessing.Process. (cherry picked from commit 266f490)

Fix for invalid assert on big output of multiprocessing.Process

19481ec

the-knights-who-say-ni added the CLA not signed label Sep 1, 2018

bedevere-bot added the awaiting review label Sep 1, 2018

add the news

95bfc95

remove the changes from generated file

c622aae

Remove changes in the generated part and made changes in the clinic i…

5c8ffcb

…nput instead

!U add test for big output multiprocessing check

31c90f9

vstinner reviewed Sep 3, 2018

View reviewed changes

vstinner requested review from pitrou and serhiy-storchaka September 3, 2018 09:43

serhiy-storchaka reviewed Sep 3, 2018

View reviewed changes

pitrou requested changes Sep 3, 2018

View reviewed changes

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

ahcub added 2 commits September 3, 2018 13:45

!U commit changes in the generated files

a1b8f7b

!U remove test for big output because it is only for manual use

26c88c4

!U remove the manual change

4e5026f

vstinner removed the CLA not signed label Sep 3, 2018

the-knights-who-say-ni added the CLA not signed label Sep 3, 2018

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 4, 2018

vstinner merged commit 266f490 into python:master Sep 4, 2018

bedevere-bot removed the awaiting merge label Sep 4, 2018

vstinner added needs backport to 3.6 labels Sep 4, 2018

miss-islington assigned vstinner Sep 4, 2018

bedevere-bot removed the needs backport to 3.7 label Sep 4, 2018

bedevere-bot removed the needs backport to 3.6 label Sep 5, 2018

vstinner pushed a commit that referenced this pull request Sep 7, 2018

[3.6] bpo-34563: Fix for invalid assert on big output of multiprocess…

23f427a

…ing.Process (GH-9027) (GH-9069) Fix for invalid assert on big output of multiprocessing.Process. (cherry picked from commit 266f490)

GreatYYX mentioned this pull request Jul 3, 2019

Limitation of sending more than 2GB data inter process usc-isi-i2/pyrallel#1

Closed

y-he2 mentioned this pull request Jul 21, 2021

Is this fixed: "can't send large data back to main thread"? uqfoundation/pathos#217

Closed

		@@ -0,0 +1 @@
		Fix for invalid assert on big output of multiprocessing.Process



		class TestMultiprocessingWithBigOutput(unittest.TestCase):
		@unittest.skip("skipping test for manual use only. Highly demanding on RAM and time consuming")

Uh oh!

Conversation

ahcub commented Sep 1, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 1, 2018

Uh oh!

ahcub commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mariatta commented Sep 1, 2018

Uh oh!

ahcub commented Sep 1, 2018

Uh oh!

ahcub commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahcub commented Sep 1, 2018

Uh oh!

ahcub commented Sep 1, 2018

Uh oh!

ahcub commented Sep 1, 2018

Uh oh!

ahcub commented Sep 1, 2018

Uh oh!

ahcub commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahcub commented Sep 1, 2018

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!

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!

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 3, 2018

Uh oh!

ahcub commented Sep 3, 2018

Uh oh!

pitrou commented Sep 3, 2018

Uh oh!

pitrou commented Sep 3, 2018

Uh oh!

vstinner commented Sep 3, 2018

Uh oh!

ahcub commented Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahcub commented Sep 4, 2018

Uh oh!

vstinner commented Sep 4, 2018

Uh oh!

miss-islington commented Sep 4, 2018

Uh oh!

miss-islington commented Sep 4, 2018

Uh oh!

miss-islington commented Sep 4, 2018

ahcub commented Sep 1, 2018 •

edited by bedevere-bot

Loading

ahcub commented Sep 1, 2018 •

edited

Loading

ahcub commented Sep 1, 2018 •

edited

Loading

ahcub commented Sep 1, 2018 •

edited

Loading

ahcub commented Sep 3, 2018 •

edited

Loading