gh-133171: Make the executor_list thread-safe #140038
Conversation
aisk
requested review from
ZeroIntensity,
ericsnowcurrently and
markshannon
as code owners
aisk
changed the title
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?
|
In theory, |
|
I am not sure if making executor_list operations thread-safe at this point is good idea. |
|
@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, int is_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); |
|
I will try to re review this PR in this weekend. |
|
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. |
|
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); |
|
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!