set_forkserver_preload() can crash the forkserver if preloaded module instantiate multiprocessing classes

[pitrou]

BPO	28779
Nosy	@pitrou, @applio
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	forkserver_preload.patch

^{Note: 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:

assignee = None
closed_at = <Date 2016-12-10.16:22:38.557>
created_at = <Date 2016-11-23.15:26:41.393>
labels = ['3.7', 'type-bug', 'library']
title = 'set_forkserver_preload() can crash the forkserver if preloaded module instantiate multiprocessing classes'
updated_at = <Date 2017-03-31.16:36:30.370>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:30.370>
actor = 'dstufft'
assignee = 'none'
closed = True
closed_date = <Date 2016-12-10.16:22:38.557>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2016-11-23.15:26:41.393>
creator = 'pitrou'
dependencies = []
files = ['45611']
hgrepos = []
issue_num = 28779
keywords = ['patch']
message_count = 10.0
messages = ['281565', '281566', '281570', '281838', '281840', '281841', '281948', '282663', '282857', '282858']
nosy_count = 4.0
nosy_names = ['pitrou', 'python-dev', 'sbt', 'davin']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue28779'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[pitrou]

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.

[pitrou]

The example above works if you comment out either the "Lock()" line or the "set_forkserver_preload()" line.

[pitrou]

Here is a patch.

[applio]

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?

[pitrou]

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

[pitrou]

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)

[applio]

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 bpo-18999.

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.

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

[pitrou]

I will probably commit this in the coming days.

[python-dev]

New changeset 1a955981b263 by Antoine Pitrou in branch '3.5':
Issue bpo-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 bpo-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 bpo-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

[pitrou]

Now fixed, closing.