-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
set_forkserver_preload() can crash the forkserver if preloaded module instantiate multiprocessing classes #72965
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
Comments
The following script: import multiprocessing
import os
def f():
pass
multiprocessing.Lock()
if __name__ == "__main__":
ctx = multiprocessing.get_context('forkserver')
# modname is the script's importable name (not "__main__")
modname = os.path.basename(__file__).split(".")[0]
ctx.set_forkserver_preload([modname])
proc = ctx.Process(target=f)
proc.start()
proc.join() Fails with the following error: Traceback (most recent call last):
File "/home/antoine/miniconda3/envs/dask35/lib/python3.5/multiprocessing/forkserver.py", line 178, in main
_serve_one(s, listener, alive_r, handler)
File "/home/antoine/miniconda3/envs/dask35/lib/python3.5/multiprocessing/forkserver.py", line 212, in _serve_one
code = spawn._main(child_r)
File "/home/antoine/miniconda3/envs/dask35/lib/python3.5/multiprocessing/spawn.py", line 115, in _main
prepare(preparation_data)
File "/home/antoine/miniconda3/envs/dask35/lib/python3.5/multiprocessing/spawn.py", line 221, in prepare
set_start_method(data['start_method'])
File "/home/antoine/miniconda3/envs/dask35/lib/python3.5/multiprocessing/context.py", line 231, in set_start_method
raise RuntimeError('context has already been set')
RuntimeError: context has already been set This makes set_forkserver_preload() quite fragile if you preload any library that may create multiprocessing resources (such as locks) at the top level. |
The example above works if you comment out either the "Lock()" line or the "set_forkserver_preload()" line. |
Here is a patch. |
Antoine: I'm still unclear on what's going on here but I noticed something by accident when trying your example script. Specifically, when I ran your script from the directory where it exists on disk, I could successfully reproduce what you described but if I ran your script from a different working directory (e.g. 'python3.5 ../tmp/issue_28779_repro.py') then I did not see the misbehavior and it instead ran through to completion without any exceptions reported. I saw this phenomenon (all that I described above) on both OSX and Ubuntu 16.04 systems. Secondly, looking at the file you attached, did everything make it into the patch? I ask because unless I'm missing something it looks like the patch adds arguments to the function signature but does not act upon them in any new way? |
That's due to the way I wrote the script (the use of __file__ to deduce the module name to preload), not anything inherent in the bug described here.
Yes.
It does not. It's only fixing the signature of the method in the base class (which raises NotImplementedError) to match the signature of the method in the derived class (which is the only one actually called). |
Slight mistake in my explanation: the method in the base class raises ValueError, not NotImplementedError. Still, the basic explanation stands. (that said, if you think this is out of scope for this issue, I can revert that part of the patch) |
I don't see any negative consequences for the helpers if the This change may impact users of execution-bundling tools like cx-freeze -- bpo-22255 suggests this could make freezing things easier for some. I admit I don't fully appreciate the details of how these tools are implemented.
Given the nature of BaseContext's implementation, I don't see a problem with keeping your change. In case I was missing something, I spent some time searching for code possibly depending upon BaseContext.set_start_method's calling arguments but turned up nothing (no surprises). It feels cleaner to update it to be in sync with DefaultContext. Overall, LGTM the way you have it. |
I will probably commit this in the coming days. |
New changeset 1a955981b263 by Antoine Pitrou in branch '3.5': New changeset f3b9fd41b5cb by Antoine Pitrou in branch '3.6': New changeset 5456b699788f by Antoine Pitrou in branch 'default': |
Now fixed, closing. |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: