fix(subinterpreter): don't touch the thread state before create() attaches one - #6127
Conversation
There was a problem hiding this comment.
🟢 Ready to approve
The change is narrowly scoped, matches the documented API contract, and includes a targeted regression test covering the previously crashing scenarios.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes a crash in py::subinterpreter::create() when called from a thread with no current PyThreadState (e.g., after PyEval_SaveThread() or from a never-before-Python worker thread), aligning behavior with the function’s documented contract.
Changes:
- Construct
error_scopeonly aftersubinterpreter_scoped_activate main_guard(main())has attached a thread state, preventingPyErr_Fetch()/PyErr_Restore()from operating on a nulltstate. - Improves unwinding order so
~error_scoperuns while the main interpreter thread state is still active. - Adds a Catch2 regression test that exercises both “dropped tstate” and “never had a tstate” entry paths.
File summaries
| File | Description |
|---|---|
| include/pybind11/subinterpreter.h | Moves error_scope inside the main_guard scope and documents why it must be constructed after a thread state is attached. |
| tests/test_with_catch/test_subinterpreter.cpp | Adds a regression test ensuring subinterpreter::create() works when the calling thread has no PyThreadState. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
We'll probably do it when we merge, but I'd prefer Linux style trailers ( |
…aches one
`subinterpreter::create()` documents that "the main interpreter and its GIL
are not required to be held prior to calling this function", but its first
statement is `error_scope err_scope;`, i.e. `PyErr_Fetch()`, before
`main_guard` attaches a thread state. With no current `PyThreadState`,
`PyErr_Fetch()` -> `_PyErr_GetRaisedException(NULL)` dereferences null and
the process dies (SIGSEGV; 0xC0000005 on Windows). `~error_scope` is the
mirror image: it calls `PyErr_Restore()` after `main_guard` has already
swapped the thread state back away.
Two ordinary situations reach `create()` with no thread state:
- an embedder that ends its initialization with `PyEval_SaveThread()`,
which is the documented way to hand the GIL back after
`Py_InitializeFromConfig()`;
- any worker thread that has never touched Python.
Existing tests never hit this because they all run under the
`py::scoped_interpreter guard{}` in catch.cpp, which keeps the GIL held on
the main thread for the whole run.
Move `error_scope` inside the `main_guard` scope. The case it exists for is
unaffected: a caller that already holds the main GIL takes
`subinterpreter_scoped_activate`'s `simple_gil_` fast path, which keeps the
same thread state, so its pending error is still saved across
`Py_NewInterpreterFromConfig()` and restored afterwards. A caller sitting on
some other interpreter never had its error indicator touched in the first
place, since everything inside the block runs on the main interpreter's
thread state and `PyThreadState_Swap()` does not move error indicators. It
also makes the `pybind11_fail()` path unwind in a safer order: `~error_scope`
now runs while `main_guard` is still alive.
Add "Create Subinterpreter without a thread state", covering both a thread
that dropped its thread state via `gil_scoped_release` and a thread that
never had one. It segfaults without the fix and passes with it.
Verified on Windows / MSVC 14.51 / CPython 3.13.14: test_with_catch goes
from 33 to 34 test cases, all passing.
Assisted-by: ClaudeCode:claude-opus-5
4ed2765 to
3466888
Compare
|
Fixed on my side, so no need to do it at merge time: the commit now ends with |
|
@ymwang78 thanks a lot for fixing this! |
Assisted-by: ClaudeCode:claude-opus-5
* docs: add 3.1 entries to the changelog Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Henry Schreiner <henryfs@princeton.edu> * chore: prepare 3.1.0 release Assisted-by: ClaudeCode:claude-opus-5 * [skip ci] Change release date to August 4, 2026 * docs: add #6127 entry and set release date to August 6, 2026 Assisted-by: ClaudeCode:claude-opus-5 --------- Signed-off-by: Henry Schreiner <henryfs@princeton.edu> Co-authored-by: Ralf W. Grosse-Kunstleve <rwgkio@gmail.com>
Description
The problem
subinterpreter::create()documents that no GIL is required up front:But its very first statement is
error_scope err_scope;— i.e.PyErr_Fetch()— beforemain_guardattaches a thread state:With no current
PyThreadState,PyErr_Fetch()reaches_PyErr_GetRaisedException(NULL), which readststate->current_exceptionoff a null pointer, and the process dies (SIGSEGV;0xC0000005on Windows).~error_scopeis the mirror image: it callsPyErr_Restore()aftermain_guardhas already swapped the thread state back away.Two entirely ordinary situations reach
create()with no thread state:PyEval_SaveThread()— the documented way to hand the GIL back afterPy_InitializeFromConfig();That is how I ran into it. An embedding project calls
PyEval_SaveThread()at the end of its setup, and every subsequentpy::subinterpreter::create()from that thread crashed immediately. Instrumenting the call site:Wrapping the call in
py::gil_scoped_acquireworks around it, but that contradicts the documented contract.The existing tests never hit this: they all run under the
py::scoped_interpreter guard{}incatch.cpp, which keeps the GIL held on the main thread for the whole run, socreate()is only ever reached with a thread state already attached.The fix
Move
error_scopeinside themain_guardscope, so it is constructed only once a thread state is attached.The case
error_scopeexists for is unaffected. A caller that already holds the main GIL takessubinterpreter_scoped_activate'ssimple_gil_fast path, which keeps the same thread state, so its pending error is still saved acrossPy_NewInterpreterFromConfig()and restored afterwards — exactly as before.A caller sitting on some other interpreter never had its error indicator touched in the first place: everything inside the block runs on the main interpreter's thread state, and
PyThreadState_Swap()does not move error indicators. Leaving that caller's exception pending across the call is therefore correct as well.As a side benefit, the
pybind11_fail("failed to create new sub-interpreter")path now unwinds in a safer order:~error_scoperuns whilemain_guardis still alive, rather than after the thread state has been swapped away (or, previously, when there may have been none at all).The test
Create Subinterpreter without a thread statecovers both entry points — a thread that dropped its thread state viagil_scoped_release, and a thread that never had one:EXIT=139(SIGSEGV)[ OK ]test_with_catchVerified on Windows / MSVC 14.51 / CPython 3.13.14.
clang-format --style=fileis clean on both files.Suggested changelog entry:
py::subinterpreter::create()when called without a currentPyThreadState, which its documentation explicitly allows — for example from an embedder that ended initialization withPyEval_SaveThread(), or from a worker thread that has never touched Python.error_scopeis now constructed after a thread state has been attached instead of before.📚 Documentation preview 📚: https://pybind11--6127.org.readthedocs.build/