Repository navigation
Thread.join returns before PyThreadState is destroyed #63008
Description
Activity
TamasK commented
on Aug 22, 2013 TamasKmannequinMannequinAuthorWhen a thread is started in CPython, t_bootstrap [Modules/threadmodule.c] creates a PyThreadState object and then calls Thread.__bootstrap_inner [Lib/threading.py] which calls Thread.run, protected with self.__block, a Condition. Thread.join uses the same __block to block until Thread.run finished.
When Thread.run finished, __bootstrap_inner notifies on __block, so join will return. Here lies a race condition, if a thread switch to Thread.join occures before __bootstrap_inner returns to t_bootstrap. Then join will return before the PyThreadState for the thread is destroyed by t_bootstrap.
It is mostly harmless for general use, as __bootstrap_inner eventually gets scheduled again and PyThreadState is taken care of.
However. Py_EndInterpreter [Python/pythonrun.c] can be called when only the main interpreter thread is running. So when we want to call Py_EndInterpreter, we signal every other thread to stop, and join them. And when Thread.join returns, we call Py_EndInterpreter. Py_EndInterpreter checks if there are any other PyThreadStates still around and does a Py_FatalError.
As a workaround, we now do a sleep after join.
TamasK commented
on Aug 22, 2013 TamasKmannequinMannequinAuthorThe bug was found in 2.6.5, but after a quick eyeballing, current HEAD has the same problem.
I *think* we could remove that limitation in Py_EndInterpreter(). After all, Py_Finalize() is able to join() non-daemon threads, so there's no reason for Py_EndInterpreter() to allow it too. We must keep the fatal error for daemon threads, though.
As for ensuring the thread state is destroyed before Thread.join() returns, I don't know how to do it: the thread state isn't a PyObject, we can't access it from Python code.
(Actually, it would almost be possible to combine weakrefs and thread locals to ensure that join() only returns when the thread state is cleared. However, the thread state is cleared before it is untied from the interpreter, and there would still be a small window for a race condition if some destructor called by clearing the thread state released the GIL.)
(Of course, the latter problem can be solved by having a dedicated sentinel in the thread state that gets DECREF'efed only *after* the thread state is removed from the interpreter...)
TamasK commented
on Aug 22, 2013 TamasKmannequinMannequinAuthorRemoving the last thread check sounds good, what we actually need is a Py_EndInterpreter that does not casually abort() on us, we don't really care about PyThreadStates that much.
Here is a patch for 3.3/3.4.
TamasK commented
on Aug 24, 2013 TamasKmannequinMannequinAuthorAfter a quick glance, I can't see how this patch would fix the problem. It still depends on threading's Thread.join, which is affected by the race condition in __bootstrap_inner.
We already did a Thread.join before calling Py_EndInterpreter and still got bitten by the race.
Well, that's a good point. It does bring in line subinterpreters with the main interpreter when it comes to automatically joining non-daemon threads, but it doesn't solve the race condition you talked about. I forgot a bit too fast about it :-)
Here is a patch to remove the race condition. The patch is sufficiently delicate that I'd rather reserve this for 3.4, though.
neologix commented
on Aug 25, 2013 neologixmannequinMannequinThe first patch looks good, as for the second one, it'll take some time :-)
python-dev commented
on Aug 25, 2013 python-devmannequinMannequinNew changeset becbb65074e1 by Antoine Pitrou in branch 'default':
Issue bpo-18808: Non-daemon threads are now automatically joined when a sub-interpreter is shutdown (it would previously dump a fatal error).
http://hg.python.org/cpython/rev/becbb65074e137 remaining items
Indeed the Ubuntu Shared buildbot started failing again. There's probably a timing-dependent behaviour here (which is why test_is_alive_after_fork() tries several times, after all).
I think I've found the answer: the thread is sometimes already stopped by the time the child is forked, so it doesn't appear in _enumerate() anymore (it left the _active dict). Therefore its locks are not reset in _after_fork().
Oh, I also get the following sporadic failure which is triggered by slight change in semantics with Thread.join(timeout) :-)
======================================================================
FAIL: test_various_ops (test.test_threading.ThreadTests)
----------------------------------------------------------------------Traceback (most recent call last): File "/home/antoine/cpython/default/Lib/test/test_threading.py", line 113, in test_various_ops self.assertTrue(not t.is_alive()) AssertionError: False is not true
python-dev commented
on Sep 8, 2013 python-devmannequinMannequinNew changeset 74dc664ad699 by Antoine Pitrou in branch 'default':
Issue bpo-18808 again: fix the after-fork logic for not-yet-started or already-stopped threads.
http://hg.python.org/cpython/rev/74dc664ad699Adding reference to failing tests on koobs-freebsd9 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
[Antoine]
Oh, I also get the following sporadic failure
which is triggered by slight change in semantics
with Thread.join(timeout) :-)
======================================================================
FAIL: test_various_ops (test.test_threading.ThreadTests)
----------------------------------------------------------------------> Traceback (most recent call last): > File "/home/antoine/cpython/default/Lib/test/test_threading.py", line 113, in test_various_ops > self.assertTrue(not t.is_alive()) > AssertionError: False is not true
Really! In context, the test does:
t.join() self.assertTrue(not t.is_alive())
(BTW, that would be clearer as self.assertFalse(t.is_alive()) ;-) )
It was the intent that this continue to work - the only intended change in Python-visible semantics had to do with join'ing with a timeout.
Without a timeout, I confess I don't see how this can fail. join() is join(timeout=None), which does:
self._stopped.wait(timeout) if self._stopped.is_set(): self._wait_for_tstate_lock(timeout is None)
which is
self._stopped.wait(None) if self._stopped.is_set(): self._wait_for_tstate_lock(True)
which should be the same as
self._stopped.wait() self._wait_for_tstate_lock(True)
after which _stopped should be set and _tstate_lock should be None. The subsequent is_alive() should then return False, via its
return self._tstate_lock is not None
What's going wrong?
Le dimanche 08 septembre 2013 à 17:30 +0000, Tim Peters a écrit :
Really! In context, the test does:
t.join() self.assertTrue(not t.is_alive())Ah, no, the failing test did
t.join(something). I removed the timeout
to remove the failure :-)(BTW, that would be clearer as self.assertFalse(t.is_alive()) ;-) )
Yes, old coding style.
Ah - the test used to do t.join(NUMTASKS)! That's just bizarre ;-)
I believe I can repair that too (well - there was never a _guarantee_ that waiting 10 seconds would be long enough), but I'll wait until this all settles down.
join() and is_alive() are too complicated now, because of the 2-step dance to check whether the thread is done: we have both an Event (_stopped) and a lock (tstate_lock) to check now. The Event doesn't serve a purpose anymore: it's almost always uninteresting to know _just that the Python part of the thread has ended. The only exception I can see is the perverse case of joining the main thread done in some of the tests (in that case we have to claim the main thread is done even though its tstate is still active).
Anyway, after getting rid of the Event it should be dead easy to make join(10) "appear to work the same as before, despite that it never really worked ;-)".
join() and is_alive() are too complicated now, because of the 2-step
dance to check whether the thread is done: we have both an Event
(_stopped) and a lock (tstate_lock) to check now. The Event doesn't
serve a purpose anymore: it's almost always uninteresting to know
_just that the Python part of the thread has ended.Yes, that crossed my mind too. The difficulty is that only plain lock
objects are available from C code, not Events. But if the first join()er
releases the lock just after taking it, it will be enough to make the
code correct?(after all, it's an event that's never reset, which simplifies things)
(also, why is the current Event implementation based on Condition? isn't
an Event actually simpler than a Condition?)Without _stopped, join() can simply wait to acquire _tstate_lock (with or without a timeout, and skipping this if _tstate_lock is already None). Etc ;-) Of course details matter, but it's easy. I did it once, but the tests joining the main thread failed and I put the code on hold. I'll dust it off when the buildbots are all happy with the current changes.
(also, why is the current Event implementation based
on Condition?We'd have to ask Guido ;-) Best guess is that Condition supplied all the machinery to make Event.wait() work correctly, including waking all waiters up when the Event gets set.
isn't an Event actually simpler than a Condition?)
Events are indeed simple :-) There are many ways to implement them, but "ain't broke, don't fix" seems the right approach to me here. In effect, if we get rid of _stopped, the code remaining will be much like an Event implementation built on the plain _tstate_lock lock.
Without _stopped, join() can simply wait to acquire _tstate_lock (with
or without a timeout, and skipping this if _tstate_lock is already
None). Etc ;-) Of course details matter, but it's easy. I did it
once, but the tests joining the main thread failed and I put the code
on hold.Ah, of course. The main thread needs the event, since the thread state
will only be deleted at the end of Py_Finalize().
The MainThread class could override is_alive() and join(), then.The MainThread class could override is_alive() and join(), then.
I think it will be easier than that, but we'll see ;-)
python-dev commented
on Sep 8, 2013 python-devmannequinMannequinNew changeset aff959a3ba13 by Tim Peters in branch 'default':
bpo-18984: Remove ._stopped Event from Thread internals.
http://hg.python.org/cpython/rev/aff959a3ba13_thread._set_sentinel() and threading.Thread._tstate_lock is a great enhancement, as Py_EndInterprter() which now calls threading._shutdown().
FYI I found yet another race condition, this time in threading._shutdown(). See bpo-36402 follow-up: "threading._shutdown() race condition: test_threading test_threads_join_2() fails randomly".
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:
bugs.python.org fields: