Repository navigation
test_threading.test_4_daemon_threads() crash randomly #110052
Description
Activity
The regression was introduced by PR gh-109794: #109794 (comment)
I'm looking into this.
./python -m test test_threading -m test_4_daemon_threads -j50 -F --fail-env-changedI haven't been able to get a crash in over 16 minutes (but I certainly believe you).
I haven't been able to get a crash in over 16 minutes (but I certainly believe you).
What is your operating system? I reproduced the crash on Linux. I didn't try other operating systems.
If you want to make daemon threads crashes more likely, add
sleep(1)at the end of PyInterpreterState_Delete(), after free_interpreter().What is your operating system? I reproduced the crash on Linux. I didn't try other operating systems.
It's an 8-core, 24GB RAM Hyper-V Ubuntu VM running on a Windows laptop.
If you want to make daemon threads crashes more likely, add
sleep(1)at the end of PyInterpreterState_Delete(), after free_interpreter().I'll try it.
gdb traceback:
,,,
gdb debug:
...
I don't understand why the test didn't exit: _PyThreadState_MustExit() should return, no?Yeah, that's the critical question.
Here's the critical part from gh-109794:
diff --git a/Python/pystate.c b/Python/pystate.c index dcc6c112215b3..570b5242600c0 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2964,11 +2964,26 @@ _PyThreadState_MustExit(PyThreadState *tstate) tstate->interp->runtime to support calls from Python daemon threads. After Py_Finalize() has been called, tstate can be a dangling pointer: point to PyThreadState freed memory. */ + unsigned long finalizing_id = _PyRuntimeState_GetFinalizingID(&_PyRuntime); PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime); if (finalizing == NULL) { + // XXX This isn't completely safe from daemon thraeds, + // since tstate might be a dangling pointer. finalizing = _PyInterpreterState_GetFinalizing(tstate->interp); + finalizing_id = _PyInterpreterState_GetFinalizingID(tstate->interp); } - return (finalizing != NULL && finalizing != tstate); + // XXX else check &_PyRuntime._main_interpreter._initial_thread + if (finalizing == NULL) { + return 0; + } + else if (finalizing == tstate) { + return 0; + } + else if (finalizing_id == PyThread_get_thread_ident()) { + /* gh-109793: we must have switched interpreters. */ + return 0; + } + return 1; }The most obvious possibility is that we switched to a different thread state during finalization in the thread where finalization is happening and the thread state belongs to the same interpreter. However, that isn't an expected use case for
PyThread_Swap()and I'm pretty sure we don't have any code that does that.If you want to make daemon threads crashes more likely, add
sleep(1)at the end of PyInterpreterState_Delete(), after free_interpreter().I'll try it.
No failures in 13 minutes.
No failures in 13 minutes.
How do you build Python? Which configure options?
I'm usually using
./configure --with-pydebugto get assertions.Basically:
./configure --with-pydebug CFLAGS=-O0Is this issue tied to any buildbot/CI failures?
6/12 core Win 10, standard PCBuild/build.bat -d
... 0:00:31 load avg: 76.65 [ 87/1] test_threading failed (1 failure) test test_threading failed -- Traceback (most recent call last): File "f:\dev\3x\Lib\test\test_threading.py", line 1172, in test_4_daemon_threads rc, out, err = assert_python_ok('-c', script) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "f:\dev\3x\Lib\test\support\script_helper.py", line 166, in assert_python_ok return _assert_python(True, *args, **env_vars) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "f:\dev\3x\Lib\test\support\script_helper.py", line 151, in _assert_python res.fail(cmd_line) File "f:\dev\3x\Lib\test\support\script_helper.py", line 76, in fail raise AssertionError("Process return code is %d\n" AssertionError: Process return code is 3221225477 command line: ['f:\\dev\\3x\\PCbuild\\amd64\\python_d.exe', '-X', 'faulthandler', '-c', "if True:\n import os\n import random\n import sys\n import time\n import threading\n\n thread_has_run = set()\n\n def random_io():\n '''Loop for a while sleeping random tiny amounts and doing some I/O.'''\n import test.test_threading as mod\n while True:\n with open(mod.__file__, 'rb') as in_f:\n stuff = in_f.read(200)\n with open(os.devnull, 'wb') as null_f:\n null_f.write(stuff)\n time.sleep(random.random() / 1995)\n thread_has_run.add(threading.current_thread())\n\n def main():\n count = 0\n for _ in range(40):\n new_thread = threading.Thread(target=random_io)\n new_thread.daemon = True\n new_thread.start()\n count += 1\n while len(thread_has_run) < count:\n time.sleep(0.001)\n # Trigger process shutdown\n sys.exit(0)\n\n main()\n "] stdout: --- --- stderr: --- Windows fatal exception: access violationRerun showed this warning first.
0:00:29 load avg: 84.65 [ 75] test_threading passed f:\dev\3x\Lib\test\support\os_helper.py:531: RuntimeWarning: tests may fail, unable to create temporary directory 'F:\\dev\\3x\\build\\test_python_3812æ': [WinError 183] Cannot create a file when that file already exists: 'F:\\dev\\3x\\build\\test_python_3812æ' with temp_dir(path=name, quiet=quiet) as temp_path: test_4_daemon_threads (test.test_threading.ThreadJoinOnShutdown.test_4_daemon_threads) ... okThen same failure at 319.
f:\dev\3x\Lib\test\support\os_helper.py:531: RuntimeWarning: tests may fail, unable to create temporary directory 'F:\dev\3x\build\test_python_3812æ': [WinError 183] Cannot create a file when that file already exists: 'F:\dev\3x\build\test_python_3812æ'
Time to time, you may want to run
python -m test --cleanup, to remove test directories which tests left when they crashed. regrtest is supposed to clean them. But well, regrtest itself has also bugs :-)18 remaining items
Since this bug exists since Python 3.9 and nobody (before me) complained, I don't think that there is an emergency to fix it in Python 3.12. We can take time in Python 3.13 to come up with a clean design, as the one described by @colesbury: refcount and states.
I have been thinking about making use of thread-local storage for the problems caused by daemon threads. Something like this:
#Include/internal/pycore_pystate.h typedef struct { PyThreadState *tstate; int finalizing; } _Py_tss_state_t; #Include/cpython/pystate.h struct _ts { ... _Py_tss_state_t *tss; ... } ; # Python/pystate.c // Replaces _Py_tss_tstate. // Guaranteed initialized to {0}? _Py_thread_local _Py_tss_state pystate; // Update tstate_activate() and tstate_deactivate() to set pystate.tstate // (and update pystate.finalizing, if necessary). void _PyThreadState_Bind(PyThreadState *tstate) { ... tstate->tss = &pystate; } // Use this with (or in place of) _PyInterpreterState_SetFinalizing()? void _PyInterpreterState_SetTheadsFinalizing(PyInterpreterState *interp) { HEAD_LOCK(&_PyRuntime); assert(interp->finalizing); // If true, new threads won't be created. PyThreadState *tstate = interp->threads.head; while (tstate != NULL) { if (tstate->tss->tstate == tstate) { tstate->tss->finalizing = 1; } tstate = tstate->next; } HEAD_UNLOCK(&_PyRuntime); } int _PyThreadState_IsFinalizing(void) { return pystate.finalizing; } # Python/ceval_gil.c // Update _PyThreadState_MustExit() to rely on _PyThreadState_IsFinalizing(). // XXX Update take_gil() to use critical data stored in the pystate threadlocal?
Since we only need to worry about races in
take_gil()with daemon threads, we could refcount the GIL instead of the whole thread state, if a refcount is necessary.@ericsnowcurrently - I think that approach opens you up to problems if the daemon thread exits before the main thread. Then the
thread->tsswill point to invalid memory. There's no guarantee that the daemon thread necessarily exits cleanly and cleans up it'sPyThreadState.Yeah, if a thread (not just daemon) exits before we call
PyThreadState_Delete()then that would be a problem, at least without some thread finalizer hook (like C++).That said, this seems like a something that would apply broadly to the whole C community, whereas our current issue of finalization races in
take_gil()with daemon threads is fairly specific to us. Thus it's more likely to have been solved already, no?Hmm,
tss_create()can be used for key-based TSS, a la the PEP 539 API. Unlike our API though,tss_create()takes a destructor arg that gets called when the thread exits (but not with the programexit()). We could use that to clean up the thread-local variable. I'm not sure how that works for Windows. Given that C++ can do it, I'd think we could do it from C on WIndows.It's hard to sequence destruction of thread-local variables. In other words, the tss destructors may be called before or after thread-local storage is destroyed. So unless you create
pystateviatss_create(which would make access slower), then it's really hard to know if thetssdestructor will be called before or afterpystateis destroyed.For an example of this problem, see microsoft/mimalloc#164.
Is there anything wrong with the solution I've outlined above? I've tested it in nogil-3.12 to address this crash, which occurs more frequently once the GIL is disabled.
I didn't have a chance yet.
Metadata
Metadata
Assignees
Projects
- Status
On Linux, when I stress test test_threading.test_4_daemon_threads(), it does crash randomly:
gdb traceback:
gdb debug:
I don't understand why the test didn't exit: _PyThreadState_MustExit() should return, no?
I don't understand why
assert(_PyThreadState_CheckConsistency(tstate));didn't fail.Maybe Py_Finalize() was called between the pre-check:
and the code:
Linked PRs