fix: subinterpreter creation concurrency issues in 3.12 #5779
Conversation
…, easiest workaround is just to lock during it.
| */ | ||
| #if PY_VERSION_HEX < 0x030D0000 && defined(Py_MOD_PER_INTERPRETER_GIL_SUPPORTED) | ||
| static std::mutex one_at_a_time; | ||
| std::lock_guard<std::mutex> guard(one_at_a_time); |
There was a problem hiding this comment.
Could this make us vulnerable to double-locking deadlocks?
Lock 1 = GIL
Lock 2 = this C++ mutex
If Py_NewInterpreterFromConfig releases (and re-acquires) the GIL, we could have a deadlock.
The usual trick is to release the GIL just before acquiring the C++ mutex, then re-acquire the GIL before calling back into the Python C API (Py_NewInterpreterFromConfig in this case).
There was a problem hiding this comment.
Py_NewInterpreterFromConfig requires that the main interpreter GIL be held when being called. Internally it releases the main GIL and acquires the new sub-interpreter's GIL and then performs initialization.
The problem appears to be that the initializing isn't thread-safe so with independent GILs it sometimes causes crashes.
I believe it's safe it have a mutex here, because it is only ever acquired in this one place, and the main GIL is acquired first, so the mutex could only ever be locked if the main GIL is already locked. The main GIL is released before this mutex, allowing another thread to potentially get to the lock here and then wait ..... since the function doesn't reacquire the main GIL, it can continue on, return, and release the mutex. Subsequently it does have to reacquire the main GIL, but that's after this mutex has been released.
Since this crosses the boundary of two different GILs, I can't think of any other way to prevent the problem except with another lock. The only other option would be for us to leave it broken and just document that this is a big unstable on 3.12 (and also fix the unit test to not exercise this behavior).
There was a problem hiding this comment.
I can't think of any other way to prevent the problem except with another lock. The only other option would be for us to leave it broken
The frequent CI failures are pretty distracting. Thanks for the explanation. Let's merge this and see if we actually get any deadlocks.
| */ | ||
| #if PY_VERSION_HEX < 0x030D0000 && defined(Py_MOD_PER_INTERPRETER_GIL_SUPPORTED) | ||
| static std::mutex one_at_a_time; | ||
| std::lock_guard<std::mutex> guard(one_at_a_time); |
There was a problem hiding this comment.
I can't think of any other way to prevent the problem except with another lock. The only other option would be for us to leave it broken
The frequent CI failures are pretty distracting. Thanks for the explanation. Let's merge this and see if we actually get any deadlocks.
|
I'm ignoring the unrelated CI flake: CI / 🐍 (macos-13, 3.13t, -DCMAKE_CXX_STANDARD=11) / 🧪 (pull_request) |
henryiii
changed the title
Description
Looks like just lack of good support for subinterpreters internally in 3.12. Seems like the sporadic test crashes may just be from initialization issues in CPython's internal modules (including
builtin).Also, setting
threading = 1in the interpreter config appears to fix theAssertionErrorwhen shutting down a subinterpreter, so change the default config tothreading = 1.Sample ASAN report when running w/
PYTHONMALLOC=debugon ubuntu 24.04 w/python 3.12.3:And also
Suggested changelog entry: