classification
Title: set_forkserver_preload() can crash the forkserver if preloaded module instantiate multiprocessing classes
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: davin, pitrou, python-dev, sbt
Priority: normal Keywords: patch

Created on 2016-11-23 15:26 by pitrou, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
forkserver_preload.patch pitrou, 2016-11-23 16:35
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (10)
msg281565 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-11-23 15:26
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.
msg281566 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-11-23 15:27
The example above works if you comment out either the "Lock()" line or the "set_forkserver_preload()" line.
msg281570 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-11-23 16:33
Here is a patch.
msg281838 - (view) Author: Davin Potts (davin) * (Python committer) Date: 2016-11-28 00:25
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?
msg281840 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-11-28 00:52
> I noticed something by accident when trying your example script

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.

>  looking at the file you attached, did everything make it into the patch?

Yes.

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

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).
msg281841 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-11-28 00:56
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)
msg281948 - (view) Author: Davin Potts (davin) * (Python committer) Date: 2016-11-29 05:21
I don't see any negative consequences for the helpers if the `force=True` is made in spawn.prepare's invocation of set_start_method().  In tracing backwards to figure out why this wasn't done already, it seems unchanged since sbt's patch in issue18999.


This change may impact users of execution-bundling tools like cx-freeze -- issue22255 suggests this could make freezing things easier for some.  I admit I don't fully appreciate the details of how these tools are implemented.


> (that said, if you think this is out of scope for this issue, I can revert that part of the patch)

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.
msg282663 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-12-07 20:35
I will probably commit this in the coming days.
msg282857 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-10 16:22
New changeset 1a955981b263 by Antoine Pitrou in branch '3.5':
Issue #28779: multiprocessing.set_forkserver_preload() would crash the forkserver process if a preloaded module instantiated some multiprocessing objects such as locks.
https://hg.python.org/cpython/rev/1a955981b263

New changeset f3b9fd41b5cb by Antoine Pitrou in branch '3.6':
Issue #28779: multiprocessing.set_forkserver_preload() would crash the forkserver process if a preloaded module instantiated some multiprocessing objects such as locks.
https://hg.python.org/cpython/rev/f3b9fd41b5cb

New changeset 5456b699788f by Antoine Pitrou in branch 'default':
Issue #28779: multiprocessing.set_forkserver_preload() would crash the forkserver process if a preloaded module instantiated some multiprocessing objects such as locks.
https://hg.python.org/cpython/rev/5456b699788f
msg282858 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2016-12-10 16:22
Now fixed, closing.
History
Date User Action Args
2017-03-31 16:36:30dstufftsetpull_requests: + pull_request1031
2016-12-10 16:22:38pitrousetstatus: open -> closed
resolution: fixed
messages: + msg282858

stage: patch review -> resolved
2016-12-10 16:22:12python-devsetnosy: + python-dev
messages: + msg282857
2016-12-07 20:35:09pitrousetmessages: + msg282663
2016-11-29 05:21:01davinsetmessages: + msg281948
2016-11-28 00:56:12pitrousetmessages: + msg281841
2016-11-28 00:52:54pitrousetmessages: + msg281840
2016-11-28 00:25:05davinsetmessages: + msg281838
2016-11-23 16:35:08pitrousetfiles: + forkserver_preload.patch
2016-11-23 16:35:01pitrousetfiles: - forkserver_preload.patch
2016-11-23 16:34:27pitrousetstage: needs patch -> patch review
2016-11-23 16:33:41pitrousetfiles: + forkserver_preload.patch
keywords: + patch
messages: + msg281570
2016-11-23 15:27:46pitrousetmessages: + msg281566
2016-11-23 15:26:41pitroucreate