Repository navigation
Conversation
…ask lists When a thread exits with pending asyncio Tasks, PyThreadState_Clear() moves them onto the shared interp->asyncio_tasks_head list, but each Task still records the _Py_ThreadId() of the thread that created it. unregister_task() skips the stop-the-world pause whenever that id matches the current thread. That also happens for a later thread that reuses the exited thread's pthread_t, and for destructors that run later in PyThreadState_Clear(). The resulting unlocked llist_remove() calls on the shared list race with each other and can leave an already-removed node (next == NULL) linked into it. asyncio.all_tasks(), which asyncio.run() calls on shutdown, then crashes with SIGSEGV in _asyncio_all_tasks(). Record the unique id (PyThreadState.id) of the creating thread state instead, and only take the lock-free path while that thread state is not being cleared. Also drop the assertion in add_tasks_llist() that every linked Task is pending. A Task's state is set before unregister_task() unlinks it, and that removal can be waiting for the stop-the-world pause during which the list is walked. add_one_task() already filters out Tasks that are done.
|
All commit authors signed the Contributor License Agreement. |
|
Hi, is there anything I could do to help unblock this PR? I don't know what is the expected review time for such bug-fixes. I don't know if they are reviewed in bulk every N-weeks or if we don't hear back after a few days it's a bad sign. |
|
@kumaraditya303 I think this is related to #159073. @jblespiau you're going to need to update this to account for changes made there. |
…ncio-task-unregister-race
|
Updated and rebased on HEAD. |
| // check if we are in the same thread | ||
| // if so, we can avoid locking | ||
| if (task->task_tid == _Py_ThreadId()) { | ||
| if (task->task_tstate_id == tstate->id && !tstate->_status.finalizing) { |
There was a problem hiding this comment.
| if (task->task_tstate_id == tstate->id && !tstate->_status.finalizing) { | |
| if (task->task_tstate_id == tstate->id) { |
the finalizing check is unnecessary with the current design and causes a significant slowdown on teardown
| tls_tasks = make_pending_tasks(loop, 8) | ||
| for task in tls_tasks: | ||
| task.get_coro().close() | ||
| tls.tasks = tls_tasks |
There was a problem hiding this comment.
The loop’s ready queue still holds references to tls.tasks, so those tasks won’t be destroyed when the thread exits. They survive until loop.close(). Also, Task.cancel() only requests cancellation. Since the loop never runs, the tasks stay pending even after their coroutines are closed.
Could this follow the original reproducer more closely? I think that would test the change better.
When a thread exits with pending asyncio Tasks, PyThreadState_Clear() moves them onto the shared interp->asyncio_tasks_head list, but each Task still records the _Py_ThreadId() of the thread that created it. unregister_task() skips the stop-the-world pause whenever that id matches the current thread. That also happens for a later thread that reuses the exited thread's pthread_t, and for destructors that run later in PyThreadState_Clear(). The resulting unlocked llist_remove() calls on the shared list race with each other and can leave an already-removed node (next == NULL) linked into it. asyncio.all_tasks(), which asyncio.run() calls on shutdown, then crashes with SIGSEGV in _asyncio_all_tasks().
Record the unique id (PyThreadState.id) of the creating thread state instead, and only take the lock-free path while that thread state is not being cleared.
Also drop the assertion in add_tasks_llist() that every linked Task is pending. A Task's state is set before unregister_task() unlinks it, and that removal can be waiting for the stop-the-world pause during which the list is walked. add_one_task() already filters out Tasks that are done.
3.14 fix is in #159451 that we can reopen if this one is accepted.