gh-123321: Make Parser/myreadline.c locking safe in free-threaded build #123690
Conversation
…ed build Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed atomics to avoid the data race on reading _PyOS_ReadlineTState when checking for re-entrant calls.
|
@bharel - here are the modifications for the free-threaded build |
| } | ||
| } | ||
|
|
||
| Py_BEGIN_ALLOW_THREADS |
There was a problem hiding this comment.
What about the GIL though? We don't release it for the regular build?
There was a problem hiding this comment.
Oh, that's right. We don't need it for the PyMutex, which handles it internally when it blocks, but we probably still need to release it for the readline call.
There was a problem hiding this comment.
Does PyMutex take the GIL if it's called outside of the GIL?
Or is it supposed to be called only within the GIL?
Not next to the code atm.
There was a problem hiding this comment.
You can call PyMutex_Lock (and PyMutex_Unlock) with or without the GIL.
If the GIL is currently held AND PyMutex_Lock() needs to wait on the mutex, it will release the GIL while it waits on the mutex and reacquire the GIL before returning.
If the GIL is not currently held, then it's not touched.
| if (PyOS_ReadlineFunctionPointer == NULL) { | ||
| PyOS_ReadlineFunctionPointer = PyOS_StdioReadline; | ||
| } | ||
|
|
||
| if (_PyOS_ReadlineLock == NULL) { | ||
| _PyOS_ReadlineLock = PyThread_allocate_lock(); |
There was a problem hiding this comment.
I understand that this PR removes a race condition on this line by using static PyMutex _PyOS_ReadlineLock; which is initialized statically and so avoids the race condition.
The PyOS_ReadlineFunctionPointer and PyOS_StdioReadline expect to be called with the GIL released.
|
Thanks @colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…ed build (pythonGH-123690) Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed atomics to avoid the data race on reading `_PyOS_ReadlineTState` when checking for re-entrant calls. (cherry picked from commit 0c080d7) Co-authored-by: Sam Gross <[email protected]>
|
GH-123798 is a backport of this pull request to the 3.13 branch. |
|
I set up the backport PR for 3.13, but since 3.13.0rc2 is just around the corner, I expect it will wait until after the 3.13.0 final release. |
…ded build (GH-123690) (#123798) gh-123321: Make Parser/myreadline.c locking safe in free-threaded build (GH-123690) Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed atomics to avoid the data race on reading `_PyOS_ReadlineTState` when checking for re-entrant calls. (cherry picked from commit 0c080d7) Co-authored-by: Sam Gross <[email protected]>
Use a
PyMutexto avoid the race in mutex initialization. Use relaxed atomics to avoid the data race on reading _PyOS_ReadlineTState when checking for re-entrant calls.PyOS_Readlinecrashes in a multi-threaded race #123321