gh-99113: Make Sure the GIL is Acquired at the Right Places#104208

ericsnowcurrently

This is a pre-requisite for a per-interpreter GIL. Without it this change isn't strictly necessary. However, there is no downside otherwise.

(This change is broken out from gh-99114.)

Issue: Implement PEP 684: A Per-Interpreter GIL #99113

Eclips4

There's a failure, I can reproduce it on Windows. (originally taken from test_xxsubinterpreters/test_in_thread)
Minimal reproducible example:

import threading
import _xxsubinterpreters as interpreters

def foo():
    t = threading.Thread(target=interpreters.create)
    t.start()
    t.join()

foo()
foo()

Traceback:

C:\Users\KIRILL-1\CLionProjects\cpython> ./python example.py
Running Debug|x64 interpreter...
Fatal Python error: drop_gil: drop_gil: GIL is not locked
Python runtime state: initialized

Current thread 0x00001900 (most recent call first):
  <no Python frame>

ericsnowcurrently

@Eclips4, do you get the same failure with #99114?

ericsnowcurrently

@Eclips4, I've rebased this branch. Do you still get the crash?

Eclips4

Hello Eric, sorry for the wait
Tried on commit 2404310dea7c14a59fba9a7ffae4f203005bbf4f.
Same error:

PS C:\Users\KIRILL-1\CLionProjects\cpython> ./python example.py
Running Debug|x64 interpreter...
Fatal Python error: drop_gil: drop_gil: GIL is not locked
Python runtime state: initialized

Current thread 0x00003a74 (most recent call first):
  <no Python frame>

Eclips4

It's kinda interesting why it fails only on Windows. Maybe try build with buildbots?

ericsnowcurrently

Ah, looks like I was releasing the GIL unnecessarily in new_interpreter() (in pystate.c).

ericsnowcurrently

FTR, there's the stack trace before my fix:

(expand)

 	KernelBase.dll!00007ff911127b02()	Unknown
>	python312_d.dll!fatal_error_exit(int status) Line 2677	C
 	python312_d.dll!fatal_error(int fd, int header, const char * prefix, const char * msg, int status) Line 2859	C
 	python312_d.dll!_Py_FatalErrorFunc(const char * func, const char * msg) Line 2875	C
 	python312_d.dll!drop_gil(_ceval_state * ceval, _ts * tstate) Line 287	C
 	python312_d.dll!_PyEval_ReleaseLock(_ts * tstate) Line 625	C
 	python312_d.dll!_PyThreadState_Swap(pyruntimestate * runtime, _ts * newts) Line 1912	C
 	python312_d.dll!_extensions_cache_set(_object * filename, _object * name, PyModuleDef * def) Line 947	C
 	python312_d.dll!fix_up_extension(_object * mod, _object * name, _object * filename) Line 1177	C
 	python312_d.dll!_PyImport_FixupBuiltin(_object * mod, const char * name, _object * modules) Line 1310	C
 	python312_d.dll!_PySys_Create(_ts * tstate, _object * * sysmod_p) Line 3471	C
 	python312_d.dll!pycore_interp_init(_ts * tstate) Line 852	C
 	python312_d.dll!new_interpreter(_ts * * tstate_p, const PyInterpreterConfig * config) Line 2066	C
 	python312_d.dll!Py_NewInterpreterFromConfig(_ts * * tstate_p, const PyInterpreterConfig * config) Line 2101	C
 	python312_d.dll!interp_create(_object * self, _object * args, _object * kwds) Line 522	C
 	python312_d.dll!cfunction_call(_object * func, _object * args, _object * kwargs) Line 537	C
 	python312_d.dll!_PyObject_Call(_ts * tstate, _object * callable, _object * args, _object * kwargs) Line 367	C
 	python312_d.dll!PyObject_Call(_object * callable, _object * args, _object * kwargs) Line 380	C
 	python312_d.dll!_PyEval_EvalFrameDefault(_ts * tstate, _PyInterpreterFrame * frame, int throwflag) Line 3125	C
 	python312_d.dll!_PyEval_Vector(_ts * tstate, PyFunctionObject * func, _object * locals, _object * const * args, unsigned __int64 argcount, _object * kwnames) Line 1576	C
 	python312_d.dll!_PyFunction_Vectorcall(_object * func, _object * const * stack, unsigned __int64 nargsf, _object * kwnames) Line 419	C
 	python312_d.dll!_PyObject_VectorcallTstate(_ts * tstate, _object * callable, _object * const * args, unsigned __int64 nargsf, _object * kwnames) Line 92	C
 	python312_d.dll!method_vectorcall(_object * method, _object * const * args, unsigned __int64 nargsf, _object * kwnames) Line 67	C
 	python312_d.dll!_PyVectorcall_Call(_ts * tstate, _object *(*)(_object *, _object * const *, unsigned __int64, _object *) func, _object * callable, _object * tuple, _object * kwargs) Line 271	C
 	python312_d.dll!_PyObject_Call(_ts * tstate, _object * callable, _object * args, _object * kwargs) Line 354	C
 	python312_d.dll!PyObject_Call(_object * callable, _object * args, _object * kwargs) Line 380	C
 	python312_d.dll!thread_run(void * boot_raw) Line 1081	C
 	python312_d.dll!bootstrap(void * call) Line 182	C
 	[External Code]

Eclips4

Yeah, after last commit error will go away.

ericsnowcurrently

It's kinda interesting why it fails only on Windows. Maybe try build with buildbots?

That is interesting. I figured it would be worth exploring why that unexpected situation happened. Below is my analysis.

tl;dr in addition to dropping that _PyEval_ReleaseThread() call, there was a bug in _PyEval_InitGIL() that needs fixing.

(So thanks for pointing this out.)

Analysis

(expand)

The immediate problem was that we were trying to release the GIL when it wasn't held (but did already exist). This was happening, specifically, in _PyThreadState_Swap() via the pycore_interp_init() call right after init_interp_create_gil() call in new_interpreter() (in pystate.c). I expected that, when init_interp_create_gil() returned, the current thread would hold the GIL, regardless of whether or not the GIL was released earlier (which apparently we were doing).

This may mean I broke that expectation somewhere with one of my relatively recent commits (or in this PR). Or it might be a long-standing bug that I exposed with some new branch in the code.

Regardless, the failure implies broken expectations somewhere. Here are the relevant expectations I thought of, particularly relative to the failure and leading up to the call to _PySys_Create():

✅the "current" thread state when new_interpreter() (in pylifecycle.c) gets called is consistent (whether NULL or not) across platforms
✅in new_interpreter(), the "current" thread state should not be change anywhere before the _PyThreadState_SwapNoGIL()
✅_PyThreadState_SwapNoGIL() sets the "current" thread state correctly and returns the previous one
✅after the first _PyThreadState_SwapNoGIL() call, save_tstate is consistent across platforms
✅the current thread state does not change again in new_interpreter() (except at the end if it fails)
✅_PyEval_ReleaseLock() always releases the GIL if held (and crashes otherwise)
the call to _PyConfig_Copy() does not need (or touch) the GIL
✅the call to init_interp_settings() does not need (or touch) the GIL
✅the init_interp_create_gil() call correctly sets interp->ceval.gil
✅it makes sure the GIL is held
✅when pycore_interp_init() is called, the GIL is held
✅pycore_init_global_objects() does not release the GIL
✅_PyGC_Init() does not release the GIL
_Py_Deepfreeze_Init() does not release the GIL
✅pycore_init_types() does not release the GIL
✅_PyWarnings_InitState() does not release the GIL
✅_PyAtExit_Init() does not release the GIL

I checked each of these in turn, comparing on linux (where I develop) and Windows (where the failure happened), and the expectations were valid. However, in two places the GIL changed state unexpectedly (noted above without a check mark). This only happened on Windows.

I'm pretty sure that the following happened:

during the _PyConfig_Copy() call, the main thread re-acquired the GIL (since we had just released it in the sub-thread)
during the _Py_Deepfreeze_Init() call, the main interpreter released the GIL again when it started up the second sub-thread

Why did this only happen on Windows. I expect (but have not verified) that the semantics of releasing and re-acquiring the GIL in the eval loop are different on Windows and linux.

Conclusion

All that demonstrates two issues (one of which I've already addressed):

the _PyEval_ReleaseLock() call wasn't necessary (but did reveal the actual problem)
in _PyEval_InitGIL(), we skip acquiring the GIL if it's already held, but we don't make sure it's held by the current thread

The solution for _PyEval_InitGIL() is to check main_interp->ceval.gil->last_holder. I'll do that.

…thongh-104208) This is a pre-requisite for a per-interpreter GIL. Without it this change isn't strictly necessary. However, there is no real downside otherwise.

asottile

@ericsnowcurrently a bit of a long shot and way late to the party -- but I've recently bisected uwsgi (which deadlocks on python 3.12 only after a reload) and the bisection claims this commit as the culprit -- you can find more analysis in unbit/uwsgi#2659

my guess without debugging much yet is last_holder isn't forksafe? (need to poke around in gdb some more though)

asottile

I believe there's an oversight in this patch which changes the behaviour of PyThreadState_Swap -- previously it did not attempt to acquire the GIL at the end but now it does.

this patch "fixes" my problem:

diff --git a/Python/pystate.c b/Python/pystate.c
index f14934361da..218c08a8528 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -1919,7 +1919,7 @@ _PyThreadState_Swap(_PyRuntimeState *runtime, PyThreadState *newts)
 PyThreadState *
 PyThreadState_Swap(PyThreadState *newts)
 {
-    return _PyThreadState_Swap(&_PyRuntime, newts);
+    return _PyThreadState_SwapNoGIL(newts);
 }

in python#104208 it was inadvertently (?) changed to acquire the GIL

asottile

I opened #123079 with the potential fix

ericsnowcurrently added the skip news label May 5, 2023

bedevere-bot mentioned this pull request May 5, 2023

Implement PEP 684: A Per-Interpreter GIL #99113

Closed

This was referenced May 5, 2023

gh-99113: A Per-Interpreter GIL! #104210

Merged

gh-99113: A Per-Interpreter GIL! #99114

Closed

ericsnowcurrently force-pushed the fix-acquiring-gil branch from ad1e40b to d6c15f9 Compare May 5, 2023 21:46

ericsnowcurrently added 3 commits May 5, 2023 16:01

Add _PyEval_AcquireLock().

ca1f78e

Add _PyThreadState_SwapNoGIL().

c5d3b2c

Acquire the GIL when not owned.

2404310

ericsnowcurrently force-pushed the fix-acquiring-gil branch from d6c15f9 to 2404310 Compare May 5, 2023 22:03

ericsnowcurrently marked this pull request as ready for review May 5, 2023 22:03

ericsnowcurrently added 2 commits May 6, 2023 10:34

Fix a typo.

85702ae

Do not release the GIL when not necessary.

3cc9dc3

Make sure the GIL is held by the current thread in _PyEval_InitGIL().

3104796

erlend-aasland reviewed May 6, 2023

View reviewed changes

Release the GIL after swapping thread states.

56064c7

ericsnowcurrently merged commit 92d8bff into python:main May 6, 2023

ericsnowcurrently deleted the fix-acquiring-gil branch May 6, 2023 21:59

Eclips4 mentioned this pull request May 9, 2023

GH-104308: socket.getnameinfo should release the GIL #104307

Merged

asottile mentioned this pull request Aug 16, 2024

[3.12] gh-99113: restore behaviour of PyThreadState_Swap #123079

Closed

asottile added a commit to asottile/cpython that referenced this pull request Aug 16, 2024

restore behaviour of PyThreadState_Swap …

01f131e

in python#104208 it was inadvertently (?) changed to acquire the GIL

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-99113: Make Sure the GIL is Acquired at the Right Places#104208

gh-99113: Make Sure the GIL is Acquired at the Right Places#104208
ericsnowcurrently merged 7 commits into
python:mainfrom
ericsnowcurrently:fix-acquiring-gil

ericsnowcurrently commented May 5, 2023 •

edited

Loading

Uh oh!

Eclips4 commented May 5, 2023 •

edited

Loading

Uh oh!

ericsnowcurrently commented May 5, 2023

Uh oh!

ericsnowcurrently commented May 5, 2023

Uh oh!

Eclips4 commented May 6, 2023 •

edited

Loading

Uh oh!

Eclips4 commented May 6, 2023

Uh oh!

ericsnowcurrently commented May 6, 2023

Uh oh!

ericsnowcurrently commented May 6, 2023

Uh oh!

Eclips4 commented May 6, 2023

Uh oh!

ericsnowcurrently commented May 6, 2023 •

edited

Loading

Uh oh!

asottile commented Aug 16, 2024

Uh oh!

asottile commented Aug 16, 2024

Uh oh!

asottile commented Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ericsnowcurrently commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eclips4 commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented May 5, 2023

Uh oh!

ericsnowcurrently commented May 5, 2023

Uh oh!

Eclips4 commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eclips4 commented May 6, 2023

Uh oh!

ericsnowcurrently commented May 6, 2023

Uh oh!

ericsnowcurrently commented May 6, 2023

Uh oh!

Eclips4 commented May 6, 2023

Uh oh!

ericsnowcurrently commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Analysis

Conclusion

Uh oh!

asottile commented Aug 16, 2024

Uh oh!

asottile commented Aug 16, 2024

Uh oh!

asottile commented Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericsnowcurrently commented May 5, 2023 •

edited

Loading

Eclips4 commented May 5, 2023 •

edited

Loading

Eclips4 commented May 6, 2023 •

edited

Loading

ericsnowcurrently commented May 6, 2023 •

edited

Loading