Repository navigation
Thread.join returns before PyThreadState is destroyed #63008
Description
| BPO | 18808 |
|---|---|
| Nosy | @tim-one, @jcea, @csernazs, @ncoghlan, @pitrou, @vstinner, @koobs |
| Files |
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-09-09.20:08:46.070>
created_at = <Date 2013-08-22.13:56:37.656>
labels = ['interpreter-core', 'library', 'type-crash']
title = 'Thread.join returns before PyThreadState is destroyed'
updated_at = <Date 2019-06-10.23:19:38.831>
user = 'https://bugs.python.org/TamasK'bugs.python.org fields:
activity = <Date 2019-06-10.23:19:38.831>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2013-09-09.20:08:46.070>
closer = 'tim.peters'
components = ['Interpreter Core', 'Library (Lib)']
creation = <Date 2013-08-22.13:56:37.656>
creator = 'Tamas.K'
dependencies = []
files = ['31445', '31455', '31537', '31635', '31640', '31650', '31652', '31653', '31659']
hgrepos = []
issue_num = 18808
keywords = ['patch']
message_count = 60.0
messages = ['195891', '195895', '195906', '195909', '195910', '195911', '195999', '196076', '196077', '196085', '196115', '196154', '196156', '196611', '196627', '196663', '196672', '196923', '196956', '196962', '196977', '196983', '197038', '197052', '197125', '197137', '197177', '197178', '197179', '197181', '197182', '197183', '197184', '197185', '197188', '197190', '197191', '197193', '197195', '197197', '197202', '197207', '197213', '197214', '197215', '197217', '197221', '197247', '197248', '197251', '197253', '197291', '197294', '197302', '197304', '197306', '197307', '197310', '197338', '345152']
nosy_count = 12.0
nosy_names = ['tim.peters', 'jcea', 'csernazs', 'ncoghlan', 'pitrou', 'vstinner', 'grahamd', 'neologix', 'python-dev', 'bkabrda', 'koobs', 'Tamas.K']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue18808'
versions = ['Python 3.4']
Activity
TamasK commentedon Aug 22, 2013
When 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 commentedon Aug 22, 2013
The bug was found in 2.6.5, but after a quick eyeballing, current HEAD has the same problem.
pitrou commentedon Aug 22, 2013
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.
pitrou commentedon Aug 22, 2013
(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.)
pitrou commentedon Aug 22, 2013
(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 commentedon Aug 22, 2013
Removing 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.
pitrou commentedon Aug 23, 2013
Here is a patch for 3.3/3.4.
TamasK commentedon Aug 24, 2013
After 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.
pitrou commentedon Aug 24, 2013
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 :-)
pitrou commentedon Aug 24, 2013
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 commentedon Aug 25, 2013
The first patch looks good, as for the second one, it'll take some time :-)
python-dev commentedon Aug 25, 2013
New 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/becbb65074e1
37 remaining items
pitrou commentedon Sep 8, 2013
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).
pitrou commentedon Sep 8, 2013
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)
----------------------------------------------------------------------
python-dev commentedon Sep 8, 2013
New 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/74dc664ad699
koobs commentedon Sep 8, 2013
Adding reference to failing tests on koobs-freebsd9 and koobs-freebsd10 buildbots:
======================================================================
FAIL: test_is_alive_after_fork (test.test_threading.ThreadTests)
----------------------------------------------------------------------
tim-one commentedon Sep 8, 2013
[Antoine]
Really! In context, the test does:
(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:
which is
which should be the same as
after which _stopped should be set and _tstate_lock should be None. The subsequent is_alive() should then return False, via its
What's going wrong?
pitrou commentedon Sep 8, 2013
Le dimanche 08 septembre 2013 à 17:30 +0000, Tim Peters a écrit :
Ah, no, the failing test did
t.join(something). I removed the timeoutto remove the failure :-)
Yes, old coding style.
tim-one commentedon Sep 8, 2013
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 ;-)".
pitrou commentedon Sep 8, 2013
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?)
tim-one commentedon Sep 8, 2013
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.
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.
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.
pitrou commentedon Sep 8, 2013
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.
tim-one commentedon Sep 8, 2013
I think it will be easier than that, but we'll see ;-)
python-dev commentedon Sep 8, 2013
New changeset aff959a3ba13 by Tim Peters in branch 'default':
bpo-18984: Remove ._stopped Event from Thread internals.
http://hg.python.org/cpython/rev/aff959a3ba13
vstinner commentedon Jun 10, 2019
_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".