Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-123321: Make Parser/myreadline.c locking safe in free-threaded build #123690

Merged
merged 2 commits into from
Sep 6, 2024
Merged

gh-123321: Make Parser/myreadline.c locking safe in free-threaded build #123690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3976267
gh-123321: Make Parser/myreadline.c locking safe in free-threaded build
colesbury Sep 4, 2024
ebbdf97
Restore removed `Py_BEGIN_ALLOW_THREADS` macro call.
colesbury Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Lib/test/test_readline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import tempfile
import textwrap
import unittest
from test.support import requires_gil_enabled, verbose
from test.support import verbose
from test.support.import_helper import import_module
from test.support.os_helper import unlink, temp_dir, TESTFN
from test.support.pty_helper import run_pty
Expand Down Expand Up @@ -351,7 +351,6 @@ def test_history_size(self):
self.assertEqual(lines[-1].strip(), b"last input")

@requires_working_threading()
@requires_gil_enabled()
def test_gh123321_threadsafe(self):
"""gh-123321: readline should be thread-safe and not crash"""
script = textwrap.dedent(r"""
Expand Down
33 changes: 10 additions & 23 deletions Parser/myreadline.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
PyAPI_DATA(PyThreadState*) _PyOS_ReadlineTState;
PyThreadState *_PyOS_ReadlineTState = NULL;

static PyThread_type_lock _PyOS_ReadlineLock = NULL;
static PyMutex _PyOS_ReadlineLock;

int (*PyOS_InputHook)(void) = NULL;

Expand Down Expand Up @@ -373,34 +373,22 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt)
size_t len;

PyThreadState *tstate = _PyThreadState_GET();
if (_PyOS_ReadlineTState == tstate) {
if (_Py_atomic_load_ptr_relaxed(&_PyOS_ReadlineTState) == tstate) {
PyErr_SetString(PyExc_RuntimeError,
"can't re-enter readline");
return NULL;
}


// GH-123321: We need to acquire the lock before setting
// _PyOS_ReadlineTState, otherwise the variable may be nullified by a
// different thread.
Py_BEGIN_ALLOW_THREADS
PyMutex_Lock(&_PyOS_ReadlineLock);
_Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, tstate);
if (PyOS_ReadlineFunctionPointer == NULL) {
PyOS_ReadlineFunctionPointer = PyOS_StdioReadline;
}

if (_PyOS_ReadlineLock == NULL) {
_PyOS_ReadlineLock = PyThread_allocate_lock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (_PyOS_ReadlineLock == NULL) {
PyErr_SetString(PyExc_MemoryError, "can't allocate lock");
return NULL;
}
}

Py_BEGIN_ALLOW_THREADS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the GIL though? We don't release it for the regular build?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// GH-123321: We need to acquire the lock before setting
// _PyOS_ReadlineTState and after the release of the GIL, otherwise
// the variable may be nullified by a different thread or a deadlock
// may occur if the GIL is taken in any sub-function.
PyThread_acquire_lock(_PyOS_ReadlineLock, 1);
_PyOS_ReadlineTState = tstate;

/* This is needed to handle the unlikely case that the
* interpreter is in interactive mode *and* stdin/out are not
* a tty. This can happen, for example if python is run like
Expand All @@ -426,9 +414,8 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt)

// gh-123321: Must set the variable and then release the lock before
// taking the GIL. Otherwise a deadlock or segfault may occur.
_PyOS_ReadlineTState = NULL;
PyThread_release_lock(_PyOS_ReadlineLock);

_Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, NULL);
PyMutex_Unlock(&_PyOS_ReadlineLock);
Py_END_ALLOW_THREADS

if (rv == NULL)
Expand Down
Loading