Skip to content

gh-144446: Fix thread-safety of FrameLocalsProxy on executing frames #145183

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

Draft
colesbury wants to merge 1 commit into
base: main
Choose a base branch
from
+180 −13
Draft

gh-144446: Fix thread-safety of FrameLocalsProxy on executing frames #145183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 56 additions & 0 deletions Lib/test/test_free_threading/test_frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,62 @@ def writer(frame):

run_with_frame([reader, writer, reader, writer])

def test_concurrent_f_locals_read_values(self):
def runner():
a = 1
b = "hello"
c = [1, 2, 3]
for i in range(100):
a += i

def reader(frame):
locals_dict = frame.f_locals
list(locals_dict.keys())
list(locals_dict.values())

run_with_frame(reader, runner=runner)

def test_concurrent_f_locals_write(self):
def runner():
x = 0
for i in range(100):
x += i

def writer(frame):
frame.f_locals["new_var"] = 42

run_with_frame(writer, runner=runner)

def test_concurrent_f_locals_read_write(self):
def runner():
a = 1
b = 2
for i in range(100):
a += i

def reader(frame):
_ = frame.f_locals.get("a")
_ = frame.f_locals.get("b")

def writer(frame):
frame.f_locals["a"] = 42

run_with_frame([reader, writer, reader, writer], runner=runner)

def test_concurrent_f_locals_iteration(self):
def runner():
a = 1
b = "hello"
c = [1, 2, 3]
for i in range(100):
a += i

def iterator(frame):
for key, value in frame.f_locals.items():
pass

run_with_frame(iterator, runner=runner)

def test_concurrent_frame_clear(self):
# Test race between frame.clear() and attribute reads.
def create_frame():
Expand Down
137 changes: 124 additions & 13 deletions Objects/frameobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include "pycore_object.h" // _PyObject_GC_UNTRACK()
#include "pycore_opcode_metadata.h" // _PyOpcode_Caches
#include "pycore_optimizer.h" // _Py_Executors_InvalidateDependency()
#include "pycore_pystate.h" // _PyEval_StopTheWorld()
#include "pycore_tstate.h" // _PyThreadStateImpl
#include "pycore_unicodeobject.h" // _PyUnicode_Equal()

#include "frameobject.h" // PyFrameLocalsProxyObject
Expand All @@ -38,6 +40,52 @@ class frame "PyFrameObject *" "&PyFrame_Type"
/*[clinic end generated code: output=da39a3ee5e6b4b0d input=2d1dbf2e06cf351f]*/


#ifdef Py_GIL_DISABLED
// Returns 1 if the frame is currently executing on a different thread.
// Must be called while holding the critical section on the frame object.
static int
_frame_is_on_other_thread(PyFrameObject *frame)
{
_PyInterpreterFrame *iframe = frame->f_frame;
if (iframe->owner == FRAME_OWNED_BY_THREAD) {
PyThreadState *tstate = _PyThreadState_GET();
int32_t our_tlbc = ((_PyThreadStateImpl *)tstate)->tlbc_index;
return iframe->tlbc_index != our_tlbc;
}
// TODO(gh-144446): handle FRAME_OWNED_BY_GENERATOR by locking
// the generator's frame state to synchronize with gen_send_ex2.
return 0;
}
#endif

// Call `call` synchronized with the frame's owning thread and store
// the result in `result`. If the frame is executing on another thread,
// pause all threads (stop-the-world) before calling.
#ifdef Py_GIL_DISABLED
#define FRAMELOCALSPROXY_SYNCHRONIZED(FRAME, CALL, RESULT) \
do { \
int _stw; \
Py_BEGIN_CRITICAL_SECTION(FRAME); \
_stw = _frame_is_on_other_thread(FRAME); \
if (!_stw) { \
RESULT = CALL; \
} \
Py_END_CRITICAL_SECTION(); \
if (_stw) { \
PyInterpreterState *interp = _PyInterpreterState_GET(); \
_PyEval_StopTheWorld(interp); \
RESULT = CALL; \
_PyEval_StartTheWorld(interp); \
} \
} while (0)
#else
#define FRAMELOCALSPROXY_SYNCHRONIZED(FRAME, CALL, RESULT) \
do { \
RESULT = CALL; \
} while (0)
#endif


// Returns new reference or NULL
static PyObject *
framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
Expand Down Expand Up @@ -184,9 +232,8 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject *key, bool read, PyO
}

static PyObject *
framelocalsproxy_getitem(PyObject *self, PyObject *key)
framelocalsproxy_getitem_lock_held(PyFrameObject *frame, PyObject *key)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyObject *value = NULL;

int i = framelocalsproxy_getkeyindex(frame, key, true, &value);
Expand Down Expand Up @@ -215,6 +262,16 @@ framelocalsproxy_getitem(PyObject *self, PyObject *key)
return NULL;
}

static PyObject *
framelocalsproxy_getitem(PyObject *self, PyObject *key)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyObject *result;
FRAMELOCALSPROXY_SYNCHRONIZED(frame,
framelocalsproxy_getitem_lock_held(frame, key), result);
return result;
}

static int
add_overwritten_fast_local(PyFrameObject *frame, PyObject *obj)
{
Expand Down Expand Up @@ -243,10 +300,10 @@ add_overwritten_fast_local(PyFrameObject *frame, PyObject *obj)
}

static int
framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
framelocalsproxy_setitem_lock_held(PyFrameObject *frame, PyObject *key,
PyObject *value)
{
/* Merge locals into fast locals */
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
_PyStackRef *fast = _PyFrame_GetLocalsArray(frame->f_frame);
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);

Expand Down Expand Up @@ -319,6 +376,16 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
}
}

static int
framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
int result;
FRAMELOCALSPROXY_SYNCHRONIZED(frame,
framelocalsproxy_setitem_lock_held(frame, key, value), result);
return result;
}

static int
framelocalsproxy_merge(PyObject* self, PyObject* other)
{
Expand Down Expand Up @@ -369,9 +436,8 @@ framelocalsproxy_merge(PyObject* self, PyObject* other)
}

static PyObject *
framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored))
framelocalsproxy_keys_lock_held(PyFrameObject *frame)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *names = PyList_New(0);
if (names == NULL) {
Expand Down Expand Up @@ -407,6 +473,16 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored))
return names;
}

static PyObject *
framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored))
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyObject *result;
FRAMELOCALSPROXY_SYNCHRONIZED(frame,
framelocalsproxy_keys_lock_held(frame), result);
return result;
}

static void
framelocalsproxy_dealloc(PyObject *self)
{
Expand Down Expand Up @@ -578,9 +654,8 @@ framelocalsproxy_inplace_or(PyObject *self, PyObject *other)
}

static PyObject *
framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored))
framelocalsproxy_values_lock_held(PyFrameObject *frame)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *values = PyList_New(0);
if (values == NULL) {
Expand Down Expand Up @@ -616,9 +691,18 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored))
}

static PyObject *
framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored))
framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored))
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyObject *result;
FRAMELOCALSPROXY_SYNCHRONIZED(frame,
framelocalsproxy_values_lock_held(frame), result);
return result;
}

static PyObject *
framelocalsproxy_items_lock_held(PyFrameObject *frame)
{
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *items = PyList_New(0);
if (items == NULL) {
Expand Down Expand Up @@ -674,10 +758,19 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored))
return items;
}

static Py_ssize_t
framelocalsproxy_length(PyObject *self)
static PyObject *
framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored))
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyObject *result;
FRAMELOCALSPROXY_SYNCHRONIZED(frame,
framelocalsproxy_items_lock_held(frame), result);
return result;
}

static Py_ssize_t
framelocalsproxy_length_lock_held(PyFrameObject *frame)
{
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
Py_ssize_t size = 0;

Expand All @@ -694,11 +787,19 @@ framelocalsproxy_length(PyObject *self)
return size;
}

static int
framelocalsproxy_contains(PyObject *self, PyObject *key)
static Py_ssize_t
framelocalsproxy_length(PyObject *self)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
Py_ssize_t result;
FRAMELOCALSPROXY_SYNCHRONIZED(frame,
framelocalsproxy_length_lock_held(frame), result);
return result;
}

static int
framelocalsproxy_contains_lock_held(PyFrameObject *frame, PyObject *key)
{
int i = framelocalsproxy_getkeyindex(frame, key, true, NULL);
if (i == -2) {
return -1;
Expand All @@ -715,6 +816,16 @@ framelocalsproxy_contains(PyObject *self, PyObject *key)
return 0;
}

static int
framelocalsproxy_contains(PyObject *self, PyObject *key)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
int result;
FRAMELOCALSPROXY_SYNCHRONIZED(frame,
framelocalsproxy_contains_lock_held(frame, key), result);
return result;
}

static PyObject* framelocalsproxy___contains__(PyObject *self, PyObject *key)
{
int result = framelocalsproxy_contains(self, key);
Expand Down
Loading