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. |
|
I think it's done, and I rebased on head again. Github is showing me it's "processing updates". if this version still need changes, maybe it would be easier if a Python maintainer takes over as they are more familar with the tooling, tests, code, etc. As there is a reproducer, it should already help quite a lot. the test passes after the CL, and fails on the assert |
|
I don't know why, but what I see inthe PR isn't what I have in the branch in my fork: |
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.