gh-136003: Execute pre-finalization callbacks in a loop #136004
Conversation
| /* Each of these functions can start one another, e.g. a pending call | ||
| * could start a thread or vice versa. To ensure that we properly clean | ||
| * call everything, we run these in a loop until none of them run anything. */ | ||
| for (;;) { |
There was a problem hiding this comment.
Would it make sense to add an arbitrary limit to detect infinite loop? For example, log an error after 16 attemps.
There was a problem hiding this comment.
Well, it would prevent deadlocks in rare cases, but it would cause crashes in other equally rare cases. Maybe it would be better to emit a fatal error when there are too many iterations?
| @threading_helper.requires_working_threading() | ||
| def test_thread_created_in_atexit(self): |
There was a problem hiding this comment.
It would probably make sense to extend this with a subtest to check it in a subinterpreter.
| @threading_helper.requires_working_threading() | ||
| def test_pending_call_creates_thread(self): |
There was a problem hiding this comment.
Likewise, it would probably make sense to extend this with a subtest to check it in a subinterpreter.
Also, it would be good to test adding a pending call to a different interpreter during its finalization.
| int called = 0; | ||
|
|
||
| // Wrap up existing "threading"-module-created, non-daemon threads. | ||
| int threads_joined = wait_for_thread_shutdown(tstate); | ||
| called = Py_MAX(called, threads_joined); | ||
|
|
||
| // Make any remaining pending calls. | ||
| int made_pending_calls = _Py_FinishPendingCalls(tstate); | ||
| called = Py_MAX(called, made_pending_calls); | ||
|
|
||
| /* The interpreter is still entirely intact at this point, and the | ||
| * exit funcs may be relying on that. In particular, if some thread | ||
| * or exit func is still waiting to do an import, the import machinery | ||
| * expects Py_IsInitialized() to return true. So don't say the | ||
| * runtime is uninitialized until after the exit funcs have run. | ||
| * Note that Threading.py uses an exit func to do a join on all the | ||
| * threads created thru it, so this also protects pending imports in | ||
| * the threads created via Threading. | ||
| */ | ||
|
|
||
| int called_atexit = _PyAtExit_Call(tstate->interp); | ||
| called = Py_MAX(called, called_atexit); | ||
|
|
||
| if (called == 0) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a race with this loop. For example, a thread could be started by something other than an atexit handler or a pending call (i.e. from a daemon thread), and that could happen after wait_for_thread_shutdown() is called. Likewise for the others.
I imagine we would need to do a check at the bottom of the loop and wrap it in a lock. That means something like this:
| int called = 0; | |
| // Wrap up existing "threading"-module-created, non-daemon threads. | |
| int threads_joined = wait_for_thread_shutdown(tstate); | |
| called = Py_MAX(called, threads_joined); | |
| // Make any remaining pending calls. | |
| int made_pending_calls = _Py_FinishPendingCalls(tstate); | |
| called = Py_MAX(called, made_pending_calls); | |
| /* The interpreter is still entirely intact at this point, and the | |
| * exit funcs may be relying on that. In particular, if some thread | |
| * or exit func is still waiting to do an import, the import machinery | |
| * expects Py_IsInitialized() to return true. So don't say the | |
| * runtime is uninitialized until after the exit funcs have run. | |
| * Note that Threading.py uses an exit func to do a join on all the | |
| * threads created thru it, so this also protects pending imports in | |
| * the threads created via Threading. | |
| */ | |
| int called_atexit = _PyAtExit_Call(tstate->interp); | |
| called = Py_MAX(called, called_atexit); | |
| if (called == 0) { | |
| break; | |
| } | |
| } | |
| // Wrap up existing "threading"-module-created, non-daemon threads. | |
| wait_for_thread_shutdown(tstate); | |
| // Make any remaining pending calls. | |
| _Py_FinishPendingCalls(tstate); | |
| /* The interpreter is still entirely intact at this point, and the | |
| * exit funcs may be relying on that. In particular, if some thread | |
| * or exit func is still waiting to do an import, the import machinery | |
| * expects Py_IsInitialized() to return true. So don't say the | |
| * runtime is uninitialized until after the exit funcs have run. | |
| * Note that Threading.py uses an exit func to do a join on all the | |
| * threads created thru it, so this also protects pending imports in | |
| * the threads created via Threading. | |
| */ | |
| _PyAtExit_Call(tstate->interp); | |
| // Check if we're done. | |
| PyMutex_Lock(tstate->interp->prefini_mutex); | |
| if (!count_nondaemon_threads(tstate) | |
| && !count_pending_calls(tstate) | |
| && !count_atexit_handlers(tstate)) | |
| { | |
| // There's nothing left to clean up. We keep the lock held | |
| // to prevent adding new cleanup items. We will release it | |
| // after _PyInterpreterState_SetFinalizing() is called. | |
| break; | |
| } | |
| PyMutex_Unlock(tstate->interp->prefini_mutex); |
Then we'd need to make sure the prefini lock keeps new threads from getting started and new pending calls and atexit handlers from getting added.
Note that we would release the prefini lock either right after we call _PyInterpreterState_SetFinalizing() or even in `_PyInterpreterState_SetFinalizing().
There was a problem hiding this comment.
I think we probably can use an RW lock for this. I'll see what I can put together. Thanks for catching this!
|
When you're done making the requested changes, leave the comment: |
Uh oh!
There was an error while loading. Please reload this page.