bpo-39877: take_gil() now exits the thread if finalizing #18854
Conversation
take_gil() is now responsible to exit the thread if Python is finalizing. Not only exit before trying to acquire the GIL, but exit also once the GIL is succesfully acquired.
|
@pitrou: My previous change introduced a race condition: https://bugs.python.org/issue39877#msg363667 This change should fix it. |
Python/ceval_gil.h
Outdated
| @@ -243,6 +283,18 @@ take_gil(struct _ceval_runtime_state *ceval, PyThreadState *tstate) | |||
|
|
|||
| MUTEX_UNLOCK(gil->mutex); | |||
|
|
|||
| if (thread_must_exit(tstate)) { | |||
There was a problem hiding this comment.
I'm not sure why you're doing this here. It doesn't seem necessary and, furthermore, it's unlikely to trigger.
There was a problem hiding this comment.
This code path is to prevent https://bugs.python.org/issue39877#msg363667 crash. Are you suggesting to move this code at line 249, after COND_TIMED_WAIT()?
thread_must_exit() at entry is to prevent https://bugs.python.org/issue39877#msg363512 crash.
| { | ||
| int err = errno; | ||
|
|
||
| if (thread_must_exit(tstate)) { |
There was a problem hiding this comment.
Ok, but shouldn't you do this after COND_TIMED_WAIT below as well?
|
When you're done making the requested changes, leave the comment: |
| to run after wait_for_thread_shutdown() and before Py_Finalize() | ||
| completes. For example, when _PyImport_Cleanup() executes Python | ||
| code. */ | ||
| MUTEX_UNLOCK(gil->mutex); |
There was a problem hiding this comment.
I'm not sure if anyone else should be done here.
There was a problem hiding this comment.
For example, is COND_SIGNAL(gil->cond) needed to wake up the next thread waiting on COND_TIMED_WAIT()?
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
|
I merged PR #18890 which checks if the thread must exit at function exit as well. In short, it restores the Python 3.8 behavior. I close this PR for now. We can revisit take_gil() later to try to adjust/optimize it later. I prefer to move step by step. |
take_gil() is now responsible to exit the thread if Python is
finalizing. Not only exit before trying to acquire the GIL, but exit
also once the GIL is succesfully acquired.
https://bugs.python.org/issue39877