Skip to content

Commit 4b614d3

Browse files
gh-144446: Fix thread safety of gi_frame, cr_frame and ag_frame in free-threading build
1 parent af49df9 commit 4b614d3

6 files changed

Lines changed: 98 additions & 9 deletions

File tree

Include/internal/pycore_interpframe.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#include "pycore_code.h" // _PyCode_CODE()
99
#include "pycore_interpframe_structs.h" // _PyInterpreterFrame
10+
#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_PTR_ACQUIRE()
1011
#include "pycore_stackref.h" // PyStackRef_AsPyObjectBorrow()
1112
#include "pycore_stats.h" // CALL_STAT_INC()
1213

@@ -344,7 +345,7 @@ _PyFrame_GetFrameObject(_PyInterpreterFrame *frame)
344345
{
345346

346347
assert(!_PyFrame_IsIncomplete(frame));
347-
PyFrameObject *res = frame->frame_obj;
348+
PyFrameObject *res = FT_ATOMIC_LOAD_PTR_ACQUIRE(frame->frame_obj);
348349
if (res != NULL) {
349350
return res;
350351
}

Lib/test/test_free_threading/test_generators.py

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,3 +155,64 @@ def closer():
155155
done.set()
156156

157157
threading_helper.run_concurrently([reader, closer])
158+
159+
def test_gi_frame_teardown_race(self):
160+
ROUNDS = 20000
161+
162+
def gen():
163+
yield 1
164+
165+
barrier = Barrier(2)
166+
shared = {}
167+
captured = []
168+
169+
def reader():
170+
for _ in range(ROUNDS):
171+
barrier.wait()
172+
frame = shared['gen'].gi_frame
173+
if frame is not None:
174+
captured.append(frame)
175+
barrier.wait()
176+
177+
def driver():
178+
for _ in range(ROUNDS):
179+
g = gen()
180+
next(g)
181+
shared['gen'] = g
182+
barrier.wait()
183+
try:
184+
next(g)
185+
except StopIteration:
186+
pass
187+
barrier.wait()
188+
189+
threading_helper.run_concurrently([reader, driver])
190+
shared.clear()
191+
for frame in captured:
192+
self.assertIsNotNone(frame.f_lineno)
193+
194+
def test_concurrent_gi_frame(self):
195+
frames = set()
196+
def gen():
197+
for i in range(10000):
198+
yield i
199+
200+
g = gen()
201+
done = threading.Event()
202+
203+
def runner():
204+
for _ in g:
205+
pass
206+
done.set()
207+
208+
def reader():
209+
while not done.is_set():
210+
frame = g.gi_frame
211+
if frame:
212+
frame.f_code
213+
frame.f_locals
214+
frames.add(frame)
215+
self.assertIsNone(g.gi_frame)
216+
217+
threading_helper.run_concurrently([runner, reader])
218+
self.assertEqual(len(frames), 1)
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix thread safety of the :attr:`generator.gi_frame`,
2+
:attr:`coroutine.cr_frame` and :attr:`agen.ag_frame` attributes in the
3+
free-threading build.

Objects/genobject.c

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,9 @@ gen_clear_frame(PyGenObject *gen)
170170
_PyInterpreterFrame *frame = &gen->gi_iframe;
171171
_PyThreadState_UpdateLastProfiledFrame(_PyThreadState_GET(), frame, frame->previous);
172172
frame->previous = NULL;
173+
Py_BEGIN_CRITICAL_SECTION(gen);
173174
_PyFrame_ClearExceptCode(frame);
175+
Py_END_CRITICAL_SECTION();
174176
_PyErr_ClearExcState(&gen->gi_exc_state);
175177
}
176178

@@ -960,8 +962,17 @@ _gen_getframe(PyGenObject *gen, const char *const name)
960962
if (FRAME_STATE_FINISHED(frame_state)) {
961963
Py_RETURN_NONE;
962964
}
963-
// TODO: still not thread-safe with free threading
964-
return _Py_XNewRef((PyObject *)_PyFrame_GetFrameObject(&gen->gi_iframe));
965+
PyObject *frame = NULL;
966+
Py_BEGIN_CRITICAL_SECTION(gen);
967+
frame_state = FT_ATOMIC_LOAD_INT8_RELAXED(gen->gi_frame_state);
968+
if (FRAME_STATE_FINISHED(frame_state)) {
969+
frame = Py_None;
970+
}
971+
else {
972+
frame = _Py_XNewRef((PyObject *)_PyFrame_GetFrameObject(&gen->gi_iframe));
973+
}
974+
Py_END_CRITICAL_SECTION();
975+
return frame;
965976
}
966977

967978
static PyObject *

Python/ceval.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1996,9 +1996,11 @@ clear_gen_frame(PyThreadState *tstate, _PyInterpreterFrame * frame)
19961996
assert(tstate->exc_info == &gen->gi_exc_state);
19971997
tstate->exc_info = gen->gi_exc_state.previous_item;
19981998
gen->gi_exc_state.previous_item = NULL;
1999-
assert(frame->frame_obj == NULL || frame->frame_obj->f_frame == frame);
20001999
frame->previous = NULL;
2000+
Py_BEGIN_CRITICAL_SECTION(gen);
2001+
assert(frame->frame_obj == NULL || frame->frame_obj->f_frame == frame);
20012002
_PyFrame_ClearExceptCode(frame);
2003+
Py_END_CRITICAL_SECTION();
20022004
_PyErr_ClearExcState(&gen->gi_exc_state);
20032005
// gh-143939: There must not be any escaping calls between setting
20042006
// the generator return kind and returning from _PyEval_EvalFrame.

Python/frame.c

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ _PyFrame_Traverse(_PyInterpreterFrame *frame, visitproc visit, void *arg)
2020
PyFrameObject *
2121
_PyFrame_MakeAndSetFrameObject(_PyInterpreterFrame *frame)
2222
{
23-
assert(frame->frame_obj == NULL);
2423
PyObject *exc = PyErr_GetRaisedException();
2524

2625
PyFrameObject *f = _PyFrame_New_NoTrack(_PyFrame_GetCode(frame));
@@ -37,10 +36,18 @@ _PyFrame_MakeAndSetFrameObject(_PyInterpreterFrame *frame)
3736
// Notice that _PyFrame_New_NoTrack() can potentially raise a MemoryError,
3837
// but it won't allocate a traceback until the frame unwinds, so we are safe
3938
// here.
40-
assert(frame->frame_obj == NULL);
4139
assert(frame->owner != FRAME_OWNED_BY_FRAME_OBJECT);
4240
f->f_frame = frame;
41+
#ifdef Py_GIL_DISABLED
42+
PyFrameObject *expected = NULL;
43+
if (!_Py_atomic_compare_exchange_ptr(&frame->frame_obj, &expected, f)) {
44+
Py_DECREF(f);
45+
return expected;
46+
}
47+
#else
48+
assert(frame->frame_obj == NULL);
4349
frame->frame_obj = f;
50+
#endif
4451
return f;
4552
}
4653

@@ -113,9 +120,13 @@ _PyFrame_ClearExceptCode(_PyInterpreterFrame *frame)
113120
// GH-99729: Clearing this frame can expose the stack (via finalizers). It's
114121
// crucial that this frame has been unlinked, and is no longer visible:
115122
assert(_PyThreadState_GET()->current_frame != frame);
116-
if (frame->frame_obj) {
117-
PyFrameObject *f = frame->frame_obj;
118-
frame->frame_obj = NULL;
123+
#ifdef Py_GIL_DISABLED
124+
PyFrameObject *f = _Py_atomic_exchange_ptr(&frame->frame_obj, NULL);
125+
#else
126+
PyFrameObject *f = frame->frame_obj;
127+
frame->frame_obj = NULL;
128+
#endif
129+
if (f != NULL) {
119130
if (!_PyObject_IsUniquelyReferenced((PyObject *)f)) {
120131
take_ownership(f, frame);
121132
Py_DECREF(f);

0 commit comments

Comments
 (0)