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-34563: Fix for invalid assert on big output of multiprocessing.Process #9027
Conversation
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! |
I signed the CLA, can someone check what is the issue with it? |
Hi, it can take at least one business day for it to be reflected on our system. Thanks. |
ah, didn't know that, thank you! |
I get an error on Travis CI Build but I'm not sure how to fix it. It goes as the following.
but I don't have any generated code |
oh, maybe I do have changes in the generated code, hm, probably I need to change the clinic input instead of the function header |
hm, now I'm getting an error that generated files are not up to date
I'm not sure what I did wrong this time |
ok, looks like there was a requirement for a test in the change, I added a python test case for this issue |
seems like non-python test is required here, hm |
ok, I understand what is required but I don't understand how can I write a test for the changes I've made latest commits into _winapi.c don't contain any C test files that would look like the ones I need... |
@vstinner sorry for tagging, but can you help? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -0,0 +1 @@ | |||
Fix for invalid assert on big output of multiprocessing.Process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with serhiy-storchaka
|
||
|
||
class TestMultiprocessingWithBigOutput(unittest.TestCase): | ||
@unittest.skip("skipping test for manual use only. Highly demanding on RAM and time consuming") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the test.support.bigmemtest
decorator.
@@ -0,0 +1 @@ | |||
Fix for invalid assert on big output of multiprocessing.Process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a couple comments. Also, did you make sure the test also passes on Unix, or should someone else test it for you?
@@ -0,0 +1,25 @@ | |||
import unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All multiprocessing tests should go into _test_multiprocessing.py
, so that they get executed under the different execution contexts ("fork", "forkserver", "spawn").
|
||
|
||
def func(): | ||
return 'test' * (10 ** 9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a binary string (b"test"
), and add a comment stating the expected memory consumption.
} | ||
} | ||
|
||
/*[clinic input] | ||
_winapi.ReadFile | ||
|
||
handle: HANDLE | ||
size: int | ||
size: DWORD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... how come you changed this and the generated code didn't change? Did you run make clinic
to update all files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the checks passed this time it seems, can you recheck if all good @pitrou?
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 removed the test because I agree with @vstinner comment about it, it is redundant |
Thanks @ahcub! This looks good to me now. |
Waiting for the CLA to be validated now. |
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. |
Thanks for your prompt review and guidance, guys. |
Done ;-) |
Sorry, @ahcub and @vstinner, I could not cleanly backport this to |
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>
thanks, I will check the backport to 3.6 |
…ing.Process (pythonGH-9027) Fix for invalid assert on big output of multiprocessing.Process. (cherry picked from commit 266f490)
GH-9069 is a backport of this pull request to the 3.6 branch. |
https://bugs.python.org/issue34563