multiprocessing needs option to eschew fork() under Linux

@gpshead

BPO	8713
Nosy	@gpshead, @pitrou, @tiran, @ned-deily, @ezio-melotti, @dholth
Files	mp_fork_exec.patch mp_fork_exec.patch mp_split_tests.patch 8f08d83264a0.diff d9fe9757ba0c.diff b3620777f54c.diff c7aa0005f231.diff 4fc7c72b1c5d.diff

^{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 2013-08-29.17:51:41.313>
created_at = <Date 2010-05-14.16:13:09.390>
labels = ['type-feature', 'library']
title = 'multiprocessing needs option to eschew fork() under Linux'
updated_at = <Date 2014-03-10.22:11:25.777>
user = 'https://bugs.python.org/brandon-rhodes'

bugs.python.org fields:

activity = <Date 2014-03-10.22:11:25.777>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2013-08-29.17:51:41.313>
closer = 'sbt'
components = ['Library (Lib)']
creation = <Date 2010-05-14.16:13:09.390>
creator = 'brandon-rhodes'
dependencies = []
files = ['23142', '24297', '24298', '28461', '31181', '31186', '31214', '31282']
hgrepos = ['157']
issue_num = 8713
keywords = ['patch']
message_count = 48.0
messages = ['105719', '105724', '105730', '105734', '105936', '126748', '142915', '142918', '143966', '143969', '149996', '149998', '150001', '150004', '150030', '150582', '151818', '151819', '151882', '173646', '173647', '173648', '173670', '173850', '175999', '178213', '178301', '178312', '178314', '178335', '178340', '193924', '194627', '194651', '194656', '194796', '195088', '195141', '195174', '195178', '195181', '195251', '195266', '196167', '196457', '196458', '196470', '213102']
nosy_count = 19.0
nosy_names = ['gregory.p.smith', 'pitrou', 'christian.heimes', 'ned.deily', 'jnoller', 'ezio.melotti', 'rcoyner', 'asksol', 'dholth', 'brandon-rhodes', 'neologix', 'catalin.iacob', 'python-dev', 'sbt', 'numbernine', 'vsekhar', 'piotr.dobrogost', 'mrmekon', 'Stan.Seibert']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue8713'
versions = ['Python 3.4']

The "multiprocessing" module uses a bare fork() to create child processes under Linux, so the children get a copy of the entire state of the parent process. But under Windows, child processes are freshly spun-up Python interpreters with none of the data structures or open connections of the parent process available. This means that code that tests fine under Linux, because it is depending on residual parent state in a way that the programmer has not noticed, can fail spectacularly under Windows.

Therefore, the "multiprocessing" module should offer an option under Linux that ignores the advantage of being able to do a bare fork() and instead spins up a new interpreter instance just like Windows does. Some developers will just use this for testing under Linux, so their test results are valid for Windows too; and some developers might even use this in production, preferring to give up a bit of efficiency under Linux in return for an application that will show the same behavior on both platforms. Either way, an option that lets the developer subvert the simple "sys.platform != 'win32'" check in "forking.py" would go a long way towards helping us write platform-agnostic Python programs.

This is on my wish list; but I have not had time to do it. Patch welcome.

Jesse, it's great to learn it's on your wish list too!

Should I design the patch so that (a) there is some global in the module that needs tweaking to choose the child creation technique, or (b) that an argument to the Process() constructor forces a full interpreter exec to make all platforms match, or (c) that a process object once created has an attribute (like ".daemon") that you set before starting it off? Or (d) should there be a subclass of Process that, if specifically used, has the fork/exec behavior instead of just doing the fork?

My vote would probably be for (b), but you have a much better feel for the library and its style than I do.

I pretty much agree with (b) an argument - your gut instinct is correct - there's a long standing thread in python-dev which pretty much solidified my thinking about whether or not we need this (we do).

Any patch has to be backwards compatible by the way, it can not alter the current default behavior, also, it has to target python3 as 2.7 is nearing final, and this is a behavioral change.

+1 for this issue; I've also wished for this feature in the past.

+1

I have suspected that this may be necessary, not just merely useful, for some time, and bpo-6721 seems to verify that. In addition to adding the keyword arg to Process, it should also be added to Pool and Manager.

Is anyone working on a patch? If not I will work on a patch asap.

No one is currently working on a patch AFAIK

Here is a patch which adds the following functions:

  forking_disable()
  forking_enable()
  forking_is_enabled()
  set_semaphore_prefix()
  get_semaphore_prefix()

To create child processes using fork+exec on Unix, call
forking_disable() at the beginning of the program.

I have tested the patch on Linux (by adding forking_disable() to
test_multiprocessing), and it seems to work. However, the patch does
not modify test_multiprocessing, and I am not sure of the best way to
do so. (See below.)

There are some issues with named semaphores. When forking is
disabled, the name of the semaphore must be left unlinked so that
child processes can use sem_open() on the name. The patch therefore
delays unlinking the name (only when forking is disabled) until the
original SemLock object is garbage collected or the process which
created it exits.

But if a process is killed without exiting cleanly then the name may
be left unlinked. This happens, for instance, if I run
test_multiprocessing and then keep hitting ^C until all the processes
exit. On Linux this leaves files with names like

/dev/shm/sem.mp-fa012c80-4019-2

which represent leaked semaphores. These won't be destroyed until the
computer reboots or the semaphores are manually removed (by using
sem_unlink() or by unlinking the entry from the file system).

If some form of this patch is accepted, then the problem of leaked
semaphores needs to be addressed, otherwise the buildbots are likely
run out of named semaphores. But I am not sure how best to do this in
a platform agnostic way. (Maybe a forked process could collect names
of all semaphores created, via a pipe. Then it could try to
sem_unlink() all those names when all write-ends of the pipe are
closed.)

Small fix to patch.

Thanks for the patch sbt.
I think this is indeed useful, but I'm tempted to go further and say we should make this the default - and only - behavior. This will probably break existing code that accidentaly relied the fact that the implementation uses a bare fork(), but i'd say it's worth it:

it's cleaner
it will make it possible to remove all the ad-hoc handlers called after fork()
it will remove the only place in the whole stdlib where fork() isn't followed by exec(): people who get bitten by issue bpo-6721 will thus only be people calling explicitely fork(), in which case they're the sole responsibles for their misery ;-)

Another - although less common - advantage over the current implementation is that now one can run out of memory pretty easily if the operating system doesn't do overcommitting if you work with a large dataset. If fork() is followed by an exec, no problem.

Thoughts?

Thanks for the patch sbt.
I think this is indeed useful, but I'm tempted to go further and say
we should make this the default - and only - behavior. This will
probably break existing code that accidentaly relied the fact that the
implementation uses a bare fork(),

There is probably lots of such code:

code that passes non-pickleable function object / function args to
execute in the child process
code that executes code with side effects at module top level
code that relies (willingly or not) on other stuff such as fds being
inherited

I think this is indeed useful, but I'm tempted to go further and say we
should make this the default - and only - behavior. This will probably
break existing code that accidentaly relied the fact that the
implementation uses a bare fork(), but i'd say it's worth it:

I'm not convinced about making it the default behaviour, and certainly not the only one.

I have a working patch which ensures that leaked semaphores get cleaned up on exit. However, I think to add proper tests for the patch, test_multiprocessing needs to be refactored. Maybe we could end up with

multiprocessing_common.py
test_multiprocessing_processes_fork.py
test_multiprocessing_processes_nofork.py
test_multiprocessing_manager_fork.py
test_multiprocessing_manager_nofork.py
test_multiprocessing_threads.py
test_multiprocessing_others.py

The actual unittests would be in multiprocessing_common.py and test_multiprocessing_others.py. The other files would run the unittests in multiprocessing_common.py using different configurations.

Thoughts?

The forkserver process is now started using _posixsubprocess.fork_exec(). This should fix the order dependent problem mentioned before.

Also the forkserver tests are now reenabled on OSX.

I have added documentation now so I think it is ready to merge (except for a change to Makefile).

I have added documentation now so I think it is ready to merge
(except for a change to Makefile).

Good for me. This is a very nice addition!

New changeset 3b82e0d83bf9 by Richard Oudkerk in branch 'default':
Issue bpo-8713: Support alternative start methods in multiprocessing on Unix.
http://hg.python.org/cpython/rev/3b82e0d83bf9

Good for me. This is a very nice addition!

Thanks.

I do see a couple of failed assertions on Windows which presumably happen in a child process because they do not cause a failure:

Assertion failed: !collecting, file ..\Modules\gcmodule.c, line 1617

The assertion is in _PyGC_CollectNoFail() and checks that it is not called recursively. See

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/2510/steps/test/logs/stdio

I do see a couple of failed assertions on Windows which presumably
happen in a child process because they do not cause a failure:
Assertion failed: !collecting, file ..\\Modules\\gcmodule.c, line
1617
The assertion is in _PyGC_CollectNoFail() and checks that it is not
called recursively. See
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/2510/steps/test/logs/stdio

That's extremely weird. _PyGC_CollectNoFail() is only called from
PyImport_Cleanup, which itself is only called from Py_Finalize()
and Py_EndInterpreter(). It should be basically impossible for the
GC to be already collecting garbage at that point...

Perhaps you could try to find out in which test this happens?

Using the custom builders, it seems to happen randomly in test_rlock:

test_rlock (test.test_multiprocessing_spawn.WithManagerTestLock) ... Assertion failed: !collecting, file ..\Modules\gcmodule.c, line 1617
ok

http://buildbot.python.org/all/builders/AMD64%20Windows%20Server%202008%20%5BSB%5D%20custom

Ok, I enabled faulthandler in the child process and I got the explanation:
http://buildbot.python.org/all/builders/AMD64%20Windows%20Server%202008%20%5BSB%5D%20custom/builds/5/steps/test/logs/stdio

multiprocessing's manager Server uses daemon threads... Daemon threads are not joined when the interpreter shuts down, they are simply "frozen" at some point. Unfortunately, it may happen that a deamon thread is "frozen" while it was doing a cyclic garbage collection, which later triggers the assert.

I'm gonna replace the assert by a plain "if", then.

The new tests produce a few warnings:
$ ./python -m test -uall test_multiprocessing_spawn
[1/1] test_multiprocessing_spawn
Using start method 'spawn'
Warning -- threading._dangling was modified by test_multiprocessing_spawn
Warning -- multiprocessing.process._dangling was modified by test_multiprocessing_spawn

$ ./python -m test -uall -v -j2 test_multiprocessing_fork
OK (skipped=4)
Warning -- threading._dangling was modified by test_multiprocessing_fork
Warning -- multiprocessing.process._dangling was modified by test_multiprocessing_fork
1 test altered the execution environment:
    test_multiprocessing_fork

I've seen test_multiprocessing_forkserver giving warnings too, while running the whole test suite, but can't reproduce them while running it alone. The warnings seems quite similar though, so a single fix might resolve the problem with all the tests.

The "Using start method '...'" should also be displayed only when the tests are run in verbose mode.

New changeset f6c7ad7d029a by Richard Oudkerk in branch 'default':
Issue bpo-8713: Test should not print message about start method.
http://hg.python.org/cpython/rev/f6c7ad7d029a

New changeset e99832a60e63 by Richard Oudkerk in branch 'default':
Issue bpo-8713: Cleanup before saving process._dangling.
http://hg.python.org/cpython/rev/e99832a60e63

New changeset 6d998a43102b by Richard Oudkerk in branch 'default':
Issue bpo-8713: Print dangling processes/threads, if any.
http://hg.python.org/cpython/rev/6d998a43102b

I've seen test_multiprocessing_forkserver giving warnings too, while
running the whole test suite, but can't reproduce them while running it
alone. The warnings seems quite similar though, so a single fix might
resolve the problem with all the tests.

The "Using start method '...'" should also be displayed only when the
tests are run in verbose mode.

Seems to be fixed now.

New changeset b941a320601a by R David Murray in branch 'default':
whatsnew: multiprocessing start methods and context (bpo-8713 and bpo-18999)
http://hg.python.org/cpython/rev/b941a320601a

Navigation Menu

Repository navigation

multiprocessing needs option to eschew fork() under Linux #52959

22 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

multiprocessing needs option to eschew fork() under Linux #52959

Description

Activity

brandon-rhodes commented on May 14, 2010

jnoller commented on May 14, 2010

brandon-rhodes commented on May 14, 2010

jnoller commented on May 14, 2010

cool-RR commented on May 17, 2010

dholth commented on Jan 21, 2011

asksol commented on Aug 24, 2011

jnoller commented on Aug 24, 2011

sbt commented on Sep 13, 2011

sbt commented on Sep 13, 2011

neologix commented on Dec 21, 2011

pitrou commented on Dec 21, 2011

sbt commented on Dec 21, 2011

22 remaining items

sbt commented on Aug 10, 2013

sbt commented on Aug 13, 2013

pitrou commented on Aug 14, 2013

python-dev commented on Aug 14, 2013

sbt commented on Aug 14, 2013

pitrou commented on Aug 14, 2013

pitrou commented on Aug 15, 2013

pitrou commented on Aug 15, 2013

ezio-melotti commented on Aug 25, 2013

python-dev commented on Aug 29, 2013

python-dev commented on Aug 29, 2013

sbt commented on Aug 29, 2013

python-dev commented on Mar 10, 2014

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions