In threading.Thread.__bootstrap_inner(), the thread sets self.__started before adding itself to the _active collection. If a different thread forks between these two operations, we're left with a thread that believes it's alive although it isn't. _after_fork() doesn't call __stop() on this thread because the thread isn't in _active.

In the attached patch, Thread adds itself to _active before setting self.__started. This way, if it's interrupted by a fork, it may end up stopped without being started, rather than the reverse. In this case isAlive() correctly returns False.

One concern about the patch: my new test in test_threading.py only reveals the bug in Python 2.7.5 very occasionally. I don't know how to force the test to fail more frequently.

Looking at the threading.py on the 3.3 branch, it appears to have the same bug, but I can't reproduce it there, only on the 2.7 branch.

This is just a special-case of the more general problem of forking() in a multi-threaded program (see #16500 and #6721).
Since fork() can occur at any time, even though your data structures are protected by locks, they can end up in an inconsistent state in the child process. There's nothing we can do about it, and it affects *every code that's not atomic*.
For example, even though your patch fixes this specific issue, you can still end up with the thread both in _active and _limbo at the same time (which shouldn't be a problem since both are cleared after fork(), but still).

OTOH, the patch is simple and safe, so it might be interesting (another maybe better approach would be to also iterate over _limbo in _after_fork()).

As for the reproduction in 3.3, you're probably having trouble to reproduce it because of the GIL change. You might want to play with sys.setswitchinterval().

Charles-François, I agree that many guarantees are impossible to enforce in a multithreaded application that calls fork().

But the threading module does try to guarantee, after a fork, that isAlive() is False for all threads but one. I claim that it can more reliably provide this guarantee with my patch applied.

This patch makes sense to me.  I've gone over the code and I cannot spot any adverse effects.  I was wondering in particular if anything would be surprised to find a non-started thread in _active within the short window where that will be true but nothing appears to care about that.

You should add a comment to the code mentioning this issue and why the .set() is done after the _active insertion.  After that, I'd say commit it.

If you want a consistent reproducible test case for this I believe you will need to replace the Thread object's __started with a test wrapper who's set() method blocks waiting for for the fork to have happened before doing the actual set().  That is a bit tricky and may not be worth it.

> If you want a consistent reproducible test case for this I believe you will need to replace the Thread object's __started with a test wrapper who's set() method blocks waiting for for the fork to have happened before doing the actual set().  That is a bit tricky and may not be worth it.

Actually, setting sys.setswitchinterval(1e-6) makes it fairly easy to reproduce.

Patch #2:

* Add comment before .set() as requested.

* setcheckinterval(0) and try 20 times to repro the bug, inspired by test_enumerate_after_join. This reliably repros in 2.7.5.

Bump.

> Bump.

Did you see my review at http://bugs.python.org/review/18418/#ps8668 ?

Oh, no I didn't. I'm learning how this system works. I'll respond there
soon.

Thanks

On Thursday, July 25, 2013, Charles-François Natali wrote:

>
> Charles-François Natali added the comment:
>
> > Bump.
>
> Did you see my review at http://bugs.python.org/review/18418/#ps8668 ?
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org <javascript:;>>
> <http://bugs.python.org/issue18418>
> _______________________________________
>

New patch for 3.3 branch after Charles-François's critique: instead of changing startup sequence in Thread._bootstrap_inner, stop all threads in _limbo after a fork.

(Sorry about the extraneous XML file from my IDE, I made a mistake and allowed the diff to include it.)

By the way, I'm going on vacation through August 4. When I return I'll
check if there's anything else I need to do to help resolve this.

On Thursday, July 25, 2013, A. Jesse Jiryu Davis wrote:

>
> A. Jesse Jiryu Davis added the comment:
>
> (Sorry about the extraneous XML file from my IDE, I made a mistake and
> allowed the diff to include it.)
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org <javascript:;>>
> <http://bugs.python.org/issue18418>
> _______________________________________
>

I've posted another review (not sure you receive notifications).

I'm back from a Zen retreat--no email!--will address your comments
momentarily.

On Fri, Aug 2, 2013 at 3:07 AM, Charles-François Natali <
report@bugs.python.org> wrote:

>
> Charles-François Natali added the comment:
>
> I've posted another review (not sure you receive notifications).
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue18418>
> _______________________________________
>

New patch for 3.3 branch after Charles-François's critique: use _enumerate() where appropriate, and join the test thread before finishing the test.

Bump.

I think my most recent patch (August 4) addresses all of Charles-François's comments.

FYI I'm going on vacation again 8/20 through 8/25. I'll check in again as soon as I return to see if there's anything else I should do.

Well... I can't say anything except that the patch looks harmless.
It's a pity the test failure is hard to trigger, though.

New changeset ba54011aa295 by Charles-François Natali in branch '2.7':
Issue #18418: After fork(), reinit all threads states, not only active ones.
http://hg.python.org/cpython/rev/ba54011aa295

New changeset 29fce7f31539 by Charles-François Natali in branch '3.3':
Issue #18418: After fork(), reinit all threads states, not only active ones.
http://hg.python.org/cpython/rev/29fce7f31539

New changeset 8f39e2f987fb by Charles-François Natali in branch 'default':
Issue #18418: After fork(), reinit all threads states, not only active ones.
http://hg.python.org/cpython/rev/8f39e2f987fb

Thanks for accepting this patch!

Antoine, I initially had trouble writing a test that reliably reproduced the bug, especially in Python 3.3. But I read other threading tests and got smarter. The final patch includes tests that are very reliable at revealing the bug in 2.7 and 3.3.

> The final patch includes tests that are very reliable at
> revealing the bug in 2.7 and 3.3.

Indeed, I could reproduce it systematically without the patch.

> Thanks for accepting this patch!

Well, thanks for the patch!

For reference, this test is successfuly identifying failures on koobs-freebsd and koobs-freebsd10 buildbots:

======================================================================
FAIL: test_is_alive_after_fork (test.test_threading.ThreadTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/koobs-freebsd10/3.x.koobs-freebsd10/build/Lib/test/test_threading.py", line 478, in test_is_alive_after_fork
    self.assertEqual(0, status)
AssertionError: 0 != 256

I've identified a possible cause (and fix) for the sporadic test failures in 74dc664ad699. If the solution holds, it should be backported to 3.3 and 2.7.