garbage collection just after multiprocessing's fork causes exceptions

[sbt]

BPO	14548
Nosy	@pitrou
Files	mp_disable_gc.patch mp_finalize_pid.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 2012-05-25.19:41:08.628>
created_at = <Date 2012-04-11.16:32:31.595>
labels = ['type-bug', 'library']
title = "garbage collection just after multiprocessing's fork causes exceptions"
updated_at = <Date 2014-01-23.00:13:24.968>
user = 'https://bugs.python.org/sbt'

bugs.python.org fields:

activity = <Date 2014-01-23.00:13:24.968>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2012-05-25.19:41:08.628>
closer = 'sbt'
components = ['Library (Lib)']
creation = <Date 2012-04-11.16:32:31.595>
creator = 'sbt'
dependencies = []
files = ['25180', '25195']
hgrepos = []
issue_num = 14548
keywords = ['patch']
message_count = 14.0
messages = ['158049', '158052', '158064', '158071', '158080', '158081', '158087', '158113', '158159', '158164', '158180', '158567', '161580', '208868']
nosy_count = 5.0
nosy_names = ['pitrou', 'nadeem.vawda', 'neologix', 'python-dev', 'sbt']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14548'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']

[sbt]

When running test_multiprocessing on Linux I occasionally see a stream of errors caused by ignored weakref callbacks:

Exception AssertionError: AssertionError() in <Finalize object, dead> ignored

These do not cause the unittests to fail.

Finalizers from the parent process are supposed to be cleared after the fork. But if a garbage collection before that then Finalizer callbacks can be run in the "wrong" process.

Disabling gc during fork seems to prevent the errors. Or maybe the Finalizer should record the pid of the process which created it and only invoke the callback if it matches the current pid.

(Compare Issure 1336 conscerning subprocess.)

[pitrou]

Disabling gc during fork seems to prevent the errors.

Sounds reasonable to me.

[sbt]

Patch to disable gc.

[pitrou]

Shouldn't there be a try..finally, in case os.fork() fails?

[neologix]

Hmm...
I don't really like disabling GC, because it has a process-wide side
effect, and hence isn't thread-safe: if another thread forks() or
creates a subprocess right at the wrong time, it could end up with the
GC disabled for good...

[pitrou]

Hmm...
I don't really like disabling GC, because it has a process-wide side
effect, and hence isn't thread-safe: if another thread forks() or
creates a subprocess right at the wrong time, it could end up with the
GC disabled for good...

That's a problem indeed. Perhaps we need a global "fork lock" shared
between subprocess and multiprocessing?

[sbt]

That's a problem indeed. Perhaps we need a global "fork lock" shared
between subprocess and multiprocessing?

I did an atfork patch which included a (recursive) fork lock. See

http://bugs.python.org/review/6721/show

The patch included changes to multiprocessing and subprocess. (Being able to acquire the lock when doing fd manipulation is quite useful. For instance, the creation of Process.sentinel currently has a race which can mean than another process inherits the write end of the pipe. That would cause Process.join() to wait till both processes terminate.)

Actually, for Finalizers I think it would be easier to just record and check the pid.

[neologix]

> That's a problem indeed. Perhaps we need a global "fork lock" shared
> between subprocess and multiprocessing?

I did an atfork patch which included a (recursive) fork lock.  See

   http://bugs.python.org/review/6721/show

The patch included changes to multiprocessing and subprocess.  (Being able to acquire the lock when doing fd manipulation is quite useful.  For instance, the creation of Process.sentinel currently has a race which can mean than another process inherits the write end of the pipe.  That would cause Process.join() to wait till both processes terminate.)

Indeed, I had a look and it looked good.
I just had a couple minor comments, I'll try to get back to this later
today, or by the end of the week.

Actually, for Finalizers I think it would be easier to just record and check the pid.

I'd prefer this too.

[sbt]

Alternative patch which records pid when Finalize object is created. The callback does nothing if recorded pid does not match os.getpid().

[pitrou]

But what if Finalize is used to cleanup a resource that gets duplicated in children, like a file descriptor?
See e.g. forking.py, line 137 (in Popen.__init__())
or heap.py, line 244 (BufferWrapper.__init__()).

[sbt]

But what if Finalize is used to cleanup a resource that gets
duplicated in children, like a file descriptor?
See e.g. forking.py, line 137 (in Popen.__init__())
or heap.py, line 244 (BufferWrapper.__init__()).

This was how Finalize objects already acted (or were supposed to).

In the case of BufferWrapper this is intended. BufferWrapper objects do not have reference counting semantics. Instead the memory is deallocated when the object is garbage collected in the process that created it. (Garbage collection in a child process should *not* invalidate memory owned by the parent process.) You can prevent the parent process from garbage collecting the object too early by following the advice below from the documentation:

Explicitly pass resources to child processes

On Unix a child process can make use of a shared resource created in a 
parent process using a global resource. However, it is better to pass 
the object as an argument to the constructor for the child process.

Apart from making the code (potentially) compatible with Windows this 
also ensures that as long as the child process is still alive the object 
will not be garbage collected in the parent process. This might be important 
if some resource is freed when the object is garbage collected in the parent process.

In the case of the sentinel in Popen.__init__(), it is harmless if this end of the pipe gets accidentally inherited by another process. Since Process does not have a closefds argument like subprocess.Popen unintended leaking happens all the time. And even without the pid check, I think this finalizer would very rarely be triggered in a child process. (A Process object can only be garbage collected after it has been joined, and it can only be joined by it parent process.)

[neologix]

Alternative patch which records pid when Finalize object is created.

Looks good to me.
Note that it could eventually be rewritten to use an atfork() module, when we finally merge your patch for this other issue :-)

[python-dev]

New changeset 59567c117b0e by Richard Oudkerk in branch 'default':
Issue bpo-14548: Make multiprocessing finalizers check pid before running
http://hg.python.org/cpython/rev/59567c117b0e

[python-dev]

New changeset 751371dd4d1c by Richard Oudkerk in branch '2.7':
Issue bpo-14548: Make multiprocessing finalizers check pid before
http://hg.python.org/cpython/rev/751371dd4d1c