Skip to content

Commit 2f4a1cf

Browse files
committed
Make green_switch (python level greenlet.switch) and green_throw check for (prohibit) cross-thread switches early on free-threaded builds. This fixes some corruption around argument handling. The TLBC issue may also be corrected; at least, I don't reproduce it anymore.
1 parent a0c2a2a commit 2f4a1cf

9 files changed

Lines changed: 86 additions & 37 deletions

File tree

.github/workflows/tests.yml

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -114,20 +114,6 @@ jobs:
114114
python -c 'import greenlet._greenlet as G; assert G.GREENLET_USE_STANDARD_THREADING'
115115
python -m unittest discover -v greenlet.tests
116116
- name: Doctest
117-
env:
118-
# XXX: On 3.14t, when the thread-local bytecode cache is
119-
# enabled, sphinx crashes on module cleanup: there is a
120-
# reference to pdb.set_trace in ``glob``, and when the
121-
# shutdown sequence tries to clear that value, it segfaults
122-
# dec-reffing it after taking it out of the module dict. The
123-
# object appears to be corrupt, but it is utterly unclear how
124-
# we could have done this. It is 100% reliable on bath Mac and
125-
# Linux. It can be traced down to a very simple doctest
126-
# snippet in greenlet_gc.rst, but running that same snippet
127-
# standalone in a unit test doesn't produce the error.
128-
#
129-
# So this is a temporary workaround.
130-
PYTHON_TLBC: "0"
131117
run: |
132118
sphinx-build -b doctest -d docs/_build/doctrees2 docs docs/_build/doctest2
133119
- name: Lint

CHANGES.rst

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,22 @@
1616
<https://github.com/python-greenlet/greenlet/pull/499>`_ by Nicolas
1717
Bouvrette.
1818
- Address the results of an automated code audit performed by
19-
devdanzin. This includes several minor correctness changes that
19+
Daniel Diniz. This includes several minor correctness changes that
2020
theoretically could have been crashing bugs, but typically only in
2121
very rare circumstances.
2222

2323
See `PR 502 <https://github.com/python-greenlet/greenlet/pull/502>`_.
2424

25+
- Fix several race conditions that could arise in free-threaded
26+
builds when using greenlet objects from multiple threads, some of
27+
which could lead to assertion failures or interpreter crashes. On CI,
28+
this also eliminated the need to disable the thread-local bytecode
29+
cache.
30+
31+
See `issue 503
32+
<https://github.com/python-greenlet/greenlet/issues/503>`_, with
33+
thanks to Nitay Dariel and Daniel Diniz.
34+
2535
3.3.2 (2026-02-20)
2636
==================
2737

docs/caveats.rst

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ Free-threading Is Experimental
3737
==============================
3838

3939
Beginning with greenlet 3.3.0, support for Python 3.14's free-threaded
40-
mode is enabled. Use caution, as it has only limited testing.
40+
mode is enabled; greenlet 3.4.0 expands this support and fixes several
41+
race conditions. Use caution, as it has only limited testing.
4142

4243
There are known issues running greenlets in a free-threaded CPython.
4344
These include:
@@ -52,4 +53,6 @@ These include:
5253
hence the specializing interpreter) to avoid a rare crash. If your
5354
process crashes on accessing an attribute or object, or at shutdown
5455
during module cleanup, try setting the environment variable
55-
``PYTHON_TLBC=0`` or using the ``-X tlbc=0`` argument.
56+
``PYTHON_TLBC=0`` or using the ``-X tlbc=0`` argument. (If you
57+
encounter this with greenlet 3.4, please open an issue and let the
58+
maintainers know.)

src/greenlet/PyGreenlet.cpp

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,29 @@ PyDoc_STRVAR(
372372
static PyObject*
373373
green_switch(PyGreenlet* self, PyObject* args, PyObject* kwargs)
374374
{
375+
// Our use of Greenlet::args() makes this method non-reentrant.
376+
// Therefore, check to be sure the switch will be allowed ---
377+
// we're calling from the same thread that ``self`` belongs to ---
378+
// BEFORE doing anything with args(). If we don't do this, we can
379+
// find args() getting clobbered by switches that will never
380+
// succeed.
381+
//
382+
// TODO: We're only doing this for free-threaded builds because
383+
// those are the only ones that have demonstrated an issue,
384+
// trusting our later checks in g_switch to perform the same
385+
// function and the GIL to keep us from being reentered in regular
386+
// builds. BUT should we always do this as an extra measure of
387+
// safety in case we run code at unexpected times (e.g., a GC?)
388+
#ifdef Py_GIL_DISABLED
389+
try {
390+
self->pimpl->check_switch_allowed();
391+
}
392+
catch (const PyErrOccurred&) {
393+
return nullptr;
394+
}
395+
#endif
396+
397+
375398
using greenlet::SwitchingArgs;
376399
SwitchingArgs switch_args(OwnedObject::owning(args), OwnedObject::owning(kwargs));
377400
self->pimpl->may_switch_away();
@@ -454,6 +477,16 @@ PyDoc_STRVAR(
454477
static PyObject*
455478
green_throw(PyGreenlet* self, PyObject* args)
456479
{
480+
// See green_switch for why we call this early.
481+
#ifdef Py_GIL_DISABLED
482+
try {
483+
self->pimpl->check_switch_allowed();
484+
}
485+
catch (const PyErrOccurred&) {
486+
return nullptr;
487+
}
488+
#endif
489+
457490
PyArgParseParam typ(mod_globs->PyExc_GreenletExit);
458491
PyArgParseParam val;
459492
PyArgParseParam tb;

src/greenlet/TGreenlet.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ Greenlet::g_switchstack(void)
198198

199199
// No stack-based variables are valid anymore.
200200

201-
// But the global is volatile so we can reload it without the
201+
// But the global is thread_local volatile so we can reload it without the
202202
// compiler caching it from earlier.
203203
Greenlet* greenlet_that_switched_in = switching_thread_state; // aka this
204204
switching_thread_state = nullptr;
@@ -234,14 +234,14 @@ Greenlet::check_switch_allowed() const
234234
// TODO: Give the objects an API to determine if they belong
235235
// to a dead thread.
236236

237-
const BorrowedMainGreenlet main_greenlet = this->find_main_greenlet_in_lineage();
237+
const BorrowedMainGreenlet my_main_greenlet = this->find_main_greenlet_in_lineage();
238238

239-
if (!main_greenlet) {
239+
if (!my_main_greenlet) {
240240
throw PyErrOccurred(mod_globs->PyExc_GreenletError,
241241
"cannot switch to a garbage collected greenlet");
242242
}
243243

244-
if (!main_greenlet->thread_state()) {
244+
if (!my_main_greenlet->thread_state()) {
245245
throw PyErrOccurred(mod_globs->PyExc_GreenletError,
246246
"cannot switch to a different thread (which happens to have exited)");
247247
}
@@ -255,26 +255,26 @@ Greenlet::check_switch_allowed() const
255255
// may not be visible yet. So we need to check against the
256256
// current thread state (once the cheaper checks are out of
257257
// the way)
258-
const BorrowedMainGreenlet current_main_greenlet = GET_THREAD_STATE().state().borrow_main_greenlet();
258+
const BorrowedMainGreenlet main_greenlet_cur_thread = GET_THREAD_STATE().state().borrow_main_greenlet();
259259
if (
260260
// lineage main greenlet is not this thread's greenlet
261-
current_main_greenlet != main_greenlet
261+
main_greenlet_cur_thread != my_main_greenlet
262262
|| (
263263
// atteched to some thread
264264
this->main_greenlet()
265265
// XXX: Same condition as above. Was this supposed to be
266266
// this->main_greenlet()?
267-
&& current_main_greenlet != main_greenlet)
267+
&& main_greenlet_cur_thread != my_main_greenlet)
268268
// switching into a known dead thread (XXX: which, if we get here,
269269
// is bad, because we just accessed the thread state, which is
270270
// gone!)
271-
|| (!current_main_greenlet->thread_state())) {
271+
|| (!main_greenlet_cur_thread->thread_state())) {
272272
// CAUTION: This may trigger memory allocations, gc, and
273273
// arbitrary Python code.
274274
throw PyErrOccurred(
275275
mod_globs->PyExc_GreenletError,
276276
"Cannot switch to a different thread\n\tCurrent: %R\n\tExpected: %R",
277-
current_main_greenlet, main_greenlet);
277+
main_greenlet_cur_thread, my_main_greenlet);
278278
}
279279
}
280280

src/greenlet/TGreenlet.hpp

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,18 @@ namespace greenlet
403403
}
404404

405405
virtual OwnedObject throw_GreenletExit_during_dealloc(const ThreadState& current_thread_state);
406+
407+
/**
408+
* Depends on the state of this->args() or the current Python
409+
* error indicator. Thus, it is not threadsafe or reentrant.
410+
* You (you being ``green_switch``, the Python-level
411+
* ``greenlet.switch`` method) should call
412+
* ``check_switch_allowed`` in free-threaded builds before
413+
* calling this method and catch ``PyErrOccurred`` if it isn't
414+
* a valid switch. This method should also call that method
415+
* because there are places where we can switch internally
416+
* without going through the Python method.
417+
*/
406418
virtual OwnedObject g_switch() = 0;
407419
/**
408420
* Force the greenlet to appear dead. Used when it's not
@@ -500,6 +512,11 @@ namespace greenlet
500512
// slp_switch() failed.
501513
virtual bool force_slp_switch_error() const noexcept;
502514

515+
// Check the preconditions for switching to this greenlet; if they
516+
// aren't met, throws PyErrOccurred. Most callers will want to
517+
// catch this and clear the arguments if they've been set.
518+
inline void check_switch_allowed() const;
519+
503520
protected:
504521
inline void release_args();
505522

@@ -566,11 +583,6 @@ namespace greenlet
566583
// Returns the previous greenlet we just switched away from.
567584
virtual OwnedGreenlet g_switchstack_success() noexcept;
568585

569-
570-
// Check the preconditions for switching to this greenlet; if they
571-
// aren't met, throws PyErrOccurred. Most callers will want to
572-
// catch this and clear the arguments
573-
inline void check_switch_allowed() const;
574586
class GreenletStartedWhileInPython : public std::runtime_error
575587
{
576588
public:

src/greenlet/TUserGreenlet.cpp

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,16 @@ UserGreenlet::g_switch()
119119
{
120120
try {
121121
if (!this->args() && !PyErr_Occurred()) {
122-
// we have nothing to send as the result of switching, most
123-
// likely the result of trying to switch to a dead
124-
// greenlet.
122+
// we have nothing to send as the result of switching,
123+
// most likely because we've somehow allowed concurrent
124+
// uses of switch from multiple threads (which may or may
125+
// not be allowed by check_switch_allowed)
126+
// ``green_switch`` defends against this by calling
127+
// ``check_switch_allowed`` before messing with
128+
// ``args()``, but we have at least one internal caller
129+
// (``throw_GreenletExit_during_dealloc``) so we keep both
130+
// this explicit check and our call to
131+
// ``check_switch_allowed``
125132
throw PyErrOccurred(mod_globs->PyExc_GreenletError,
126133
"cannot switch with no pending arguments or exception");
127134
}

src/greenlet/greenlet_refs.hpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -446,8 +446,6 @@ namespace greenlet {
446446
void CLEAR()
447447
{
448448
Py_CLEAR(this->p);
449-
// XXX: Have failed this assertion in free-threaded builds
450-
// using multiple threads
451449
assert(this->p == nullptr);
452450
}
453451
};

tox.ini

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[tox]
22
envlist =
3-
py{37,38,39,310,311,312,313,314},py{310,311,312,313,314}-ns,docs,py314t,tsan-314#,tsan-314t
3+
py{310,311,312,313,314},docs,py314t,tsan-314,tsan-314t
44

55
[testenv]
66
commands =

0 commit comments

Comments
 (0)