Thread.join returns before PyThreadState is destroyed

[TamasK]

BPO	18808
Nosy	@tim-one, @jcea, @csernazs, @ncoghlan, @pitrou, @vstinner, @koobs
Files	subinterp_threads.patch threadstate_join.patch threadstate_join_2.patch threadstate_join_3.patch threadstate_join_4.patch threadstate_join_5.patch: added more is_alive() testing threadstate_join_6.patch subinterp_test.patch blind.patch: noble attempt ;-) to repair buildbot failures

^{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']

[TamasK]

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]

The bug was found in 2.6.5, but after a quick eyeballing, current HEAD has the same problem.

[pitrou]

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]

(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]

(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]

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]

Here is a patch for 3.3/3.4.

[TamasK]

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]

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]

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]

The first patch looks good, as for the second one, it'll take some time :-)

[python-dev]

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