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-34563: Fix for invalid assert on big output of multiprocessing.Process #9027

Merged
merged 10 commits into from
Sep 4, 2018
25 changes: 25 additions & 0 deletions Lib/test/test_multiprocessing_with_big_output.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import unittest
Copy link
Member

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

from multiprocessing.pool import Pool
from time import sleep


def func():
return 'test' * (10 ** 9)
Copy link
Member

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.



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

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.

def test_big_subprocess_output(self):
with Pool() as pool:
task = pool.apply_async(func)
for i in range(10):
if task.ready():
task.get()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you call get() with a timeout of 5 min instead?

Copy link
Contributor Author

@ahcub ahcub Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are right, it is a better solution here

break
sleep(30)
else:
self.fail('test_big_subprocess_output() timeout.')


if __name__ == '__main__':
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix for invalid assert on big output of multiprocessing.Process
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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

6 changes: 3 additions & 3 deletions Modules/_winapi.c
Original file line number Diff line number Diff line change
Expand Up @@ -1338,7 +1338,7 @@ _winapi_PeekNamedPipe_impl(PyObject *module, HANDLE handle, int size)
}
if (_PyBytes_Resize(&buf, nread))
return NULL;
return Py_BuildValue("Nii", buf, navail, nleft);
return Py_BuildValue("NII", buf, navail, nleft);
}
else {
Py_BEGIN_ALLOW_THREADS
Expand All @@ -1347,15 +1347,15 @@ _winapi_PeekNamedPipe_impl(PyObject *module, HANDLE handle, int size)
if (!ret) {
return PyErr_SetExcFromWindowsErr(PyExc_OSError, 0);
}
return Py_BuildValue("ii", navail, nleft);
return Py_BuildValue("II", navail, nleft);
}
}

/*[clinic input]
_winapi.ReadFile

handle: HANDLE
size: int
size: DWORD
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

overlapped as use_overlapped: bool(accept={int}) = False
[clinic start generated code]*/

Expand Down