[Python-Dev] Re: threading and forking and 2.0 (patch #101226)

[Tim Peters]

[Guido]
> ...
> (1) BACKGROUND: A Python lock may be released by a different thread
> than who aqcuired it, and it may be acquired by the same thread
> multiple times.  A pthread mutex must always be unlocked by the same
> thread that locked it, and can't be locked more than once.

The business about "multiple times" may be misleading, as it makes Windows
geeks think of reentrant locks.  The Python lock is not reentrant.  Instead,
it's perfectly OK for a thread that has acquired a Python lock to *try* to
acquire it again (but is not OK for a thread that has locked a pthread mutex
to try to lock it again):  the acquire attempt simply blocks until *another*
thread releases the Python lock.  By "Python lock" here I mean at the Python
C API level, and as exposed by the thread module; the threading module
exposes fancier locks (including reentrant locks).

> So, a Python lock can't be built out of a simple pthread mutex; instead,
> a Python lock is built out of a "locked" flag and a <condition variable,
> mutex> pair.  The mutex is locked for at most a few cycles, to protect
> the flag.  This design is Tim's (while still at KSR).

At that time, a pthread mutex was generally implemented as a pure spin lock,
so it was important to hold a pthread mutex for as short a span as possible
(and, indeed, the code never holds a pthread mutex for longer than across 2
simple C stmts).

> ...
> (3) BRAINWAVE: If we use a single mutex shared by all locks, instead
> of a mutex per lock, we can lock this mutex around the fork and thus
> prevent any other thread from locking it.  This is okay because, while
> a condition variable always needs a mutex to go with it, there's no
> rule that the same mutex can't be shared by many condition variables.
> The code below implements this.

Before people panic <wink>, note that this is "an issue" only for those
thread_xxx.h implementations such that fork() is supported *and* the child
process nukes threads in the child, leaving its mutexes and the data they
protect in an insane state.  They're the ones creating problems, so they're
the ones that pay.

> (4) MORE WORK: (a) The PyThread API also defines semaphores, which may
> have a similar problem.  But I'm not aware of any use of these (I'm
> not quite sure why semaphore support was added), so I haven't patched
> these.

I'm almost certain we all agreed (spurred by David Ascher) to get rid of the
semaphore implementations a while back.

> (b) The thread_pth.h file define locks in the same way; there
> may be others too.  I haven't touched these.

(c) While the scheme protects mutexes from going nuts in the child, that
doesn't necessarily imply that the data mutexes *protect* won't go nuts.
For example, this *may* not be enough to prevent insanity in import.c:  if
another thread is doing imports at the time a fork() occurs,
import_lock_level could be left at an arbitrarily high value in import.c.
But the thread doing the import has gone away in the child, so can't restore
import_lock_level to a sane value there.  I'm not convinced that matters in
this specific case, just saying we've got some tedious headwork to review
all the cases.

> (5) TESTING: Charles Waldman posted this code to reproduce the
> problem.  Unfortunately I haven't had much success with it; it seems
> to hang even when I apply Charles' patch.

What about when you apply *your* patch?

>     import thread
>     import os, sys
>     import time
>
>     def doit(name):
> 	while 1:
> 	    if os.fork()==0:
> 		print name, 'forked', os.getpid()
> 		os._exit(0)
> 	    r = os.wait()
>
>     for x in range(50):
> 	name = 't%s'%x
> 	print 'starting', name
> 	thread.start_new_thread(doit, (name,))
>
>     time.sleep(300)
>
> Here's the patch:

> ...
> + static pthread_mutex_t locking_mutex = PTHREAD_MUTEX_INITIALIZER;

Anyone know whether this gimmick is supported by all pthreads
implementations?

> ...
> + 	/* XXX Is the following supported here? */
> + 	pthread_atfork(&prefork_callback, &parent_callback,
> &child_callback);

I expect we need some autoconf stuff for that, right?

Thanks for writing this up!  Even more thanks for thinking of it <wink>.