gh-133171: Make the executor_list thread-safe#140038
Uh oh!
There was an error while loading. Please reload this page.
Conversation
aisk requested review from ZeroIntensity, ericsnowcurrently and markshannon as code owners
aisk changed the title 
Uh oh!
There was an error while loading. Please reload this page.
ZeroIntensity left a comment
ZeroIntensity left a comment There was a problem hiding this comment.
I'm not entirely sure that a mutex is safe here, because we're locking code paths that call Py_DECREF. @Fidget-Spinner, can these paths escape?
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner commented Oct 14, 2025
In theory, |
I am not sure if making executor_list operations thread-safe at this point is good idea. |
Fidget-Spinner commented Nov 7, 2025
@corona10 we need the executor list to be thread safe for it to work with the plan here #133171 (comment). @aisk could you resume the PR please? |
| void | ||
| _Py_Executors_InvalidateAll(PyInterpreterState*interp, intis_invalidation) | ||
| { | ||
| EXECUTOR_LIST_LOCK(interp); |
There was a problem hiding this comment.
This should be a stop the world event.
| } | ||
| /* Clearing an executor can deallocate others, so we need to make a list of | ||
| * executors to invalidate first */ | ||
| EXECUTOR_LIST_LOCK(interp); |
Uh oh!
There was an error while loading. Please reload this page.
corona10 commented Nov 7, 2025
I will try to re review this PR in this weekend. |
corona10 commented Nov 7, 2025
Share the DM context between @Fidget-Spinner and I. Before merging this PR, we should investigate the current blocker #133171 (comment) to verify this PR in practice. |
aisk commented Nov 9, 2025
Hi @ZeroIntensity@corona10@Fidget-Spinner , thanks for the review and feedback. I'll bring this PR up to date while waiting for #133171 (comment) to be resolved. |
| /* Release the lock before calling _Py_ClearExecutorDeletionList | ||
| * to avoid deadlock, since it also tries to acquire the same lock */ | ||
| EXECUTOR_LIST_UNLOCK(interp); | ||
| _Py_ClearExecutorDeletionList(interp); | ||
| EXECUTOR_LIST_LOCK(interp); |
Fidget-Spinner commented Nov 10, 2025
Sorry, now that I think about it, locking isn't the right choice here. We should instead move the executor list to tstate. Really sorry for taking up your time! |
4 participants
The idea is simple and naive: just add a
PyMutexto thePyInterpreterStatefor theexecutor_listunder thenogilbuild, and lock or unlock the mutex in all functions that operate on theexecutor_list.Since I'm not very familiar with the related code, there might be a better way to do this, so any feedback would be appreciated!