PyGILState_Ensure on non-Python thread causes fatal error #65090
Comments
|
In Python 3.4rc3, calling PyGILState_Ensure() from a thread that was not created by Python and without any calls to PyEval_InitThreads() will cause a fatal exit: Fatal Python error: take_gil: NULL tstate I believe this was added in http://hg.python.org/cpython/rev/dc4e805ec68a. The attached file has a minimal repro (for Windows, sorry, but someone familiar with low-level *nix threading APIs should be able to translate it easily). Basically, the sequence looks like this:
The attached repro has two workarounds (commented out). The first is to call PyThreadState_New()/PyEval_RestoreThread() from the new thread, and the second is to call PyEval_InitThreads() from the main thread. The latter is inappropriate for my use, since I am writing a debugger that can inject itself into a running CPython process - there is no way to require that PyEval_InitThreads() has been called and I can't reliably inject it onto the main thread. The first workaround is okay, but I'd rather not have to special-case 3.4 by rewriting PyGILState_Ensure() so that I can safely call PyGILState_Ensure(). |
|
Should have linked to bpo-19576 as well, which is the issue associated with that changeset. |
|
IMO it's a bug in PyEval_InitThreads(). |
|
ptest.c: Portable example using Python API. |
|
Attached patch should fix the issue. |
|
Is this fix released? I can't find it in the changelog… (I faced this bug on 3.5.2, released a couple of months after this bug was closed…) |
Oops, I lost track of this issue, but it wasn't fixed, no. I just proposed my old fix as a pull requets: PR 4650.
The workaround is to call PyEval_InitThreads() before spawning your first non-Python thread. |
|
I have to check if Python 2.7 is impacted as well. |
|
Ok, the bug is now fixed in Python 2.7, 3.6 and master (future 3.7). On 3.6 and master, the fix comes with an unit test. Thanks Steve Dower for the bug report, sorry for the long delay, I completely forgot this old bug! Marcin Kasperski: Thanks for the reminder. Sadly, we will have to wait for the next release to get the fix. In the meanwhile, you can workaround the bug by calling PyEval_InitThreads() after Py_Initialize() but before running actual code. |
|
"./Program/_testembed.exe bpo-20891" fails randomly on macOS: Current thread 0x00007fffa5dff3c0 (most recent call first): In test_bpo20891() of Program/_testembed.c, Py_BEGIN_ALLOW_THREADS calls PyEval_SaveThread() which fails with a fatal error: PyThreadState *tstate = PyThreadState_Swap(NULL);
if (tstate == NULL)
Py_FatalError("PyEval_SaveThread: NULL tstate"); <~~~ HEREI'm not sure that it's safe to create the GIL in PyGILState_Ensure() in a "non-Python" thread, while the main thread (which is a Python thread) "is running". I found a working fix: call PyEval_InitThreads() in PyThread_start_new_thread(). So the GIL is created as soon as a second thread is spawned. The GIL cannot be created anymore while two threads are running. At least, with the "python" binary. It doesn't fix the issue if a thread is not spawned by Python, but this thread calls PyGILState_Ensure(). Maybe we need to better document how threads have to be initialized to prevent this race condition / bug? |
|
Why not *always* call PyEval_InitThreads() at interpreter initialization? Are there any downsides? |
The code is quite old: commit 1984f1e (...) +void Current version of the code: void
PyEval_InitThreads(void)
{
if (gil_created())
return;
create_gil();
take_gil(PyThreadState_GET());
_PyRuntime.ceval.pending.main_thread = PyThread_get_thread_ident();
if (!_PyRuntime.ceval.pending.lock)
_PyRuntime.ceval.pending.lock = PyThread_allocate_lock();
} |
|
The "if (gil_created())" check can be found in the Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS macros which call PyEval_SaveThread() / PyEval_RestoreThread(): PyThreadState *
PyEval_SaveThread(void)
{
PyThreadState *tstate = PyThreadState_Swap(NULL);
if (tstate == NULL)
Py_FatalError("PyEval_SaveThread: NULL tstate");
if (gil_created()) <~~~~ HERE
drop_gil(tstate);
return tstate;
}
void
PyEval_RestoreThread(PyThreadState *tstate)
{
if (tstate == NULL)
Py_FatalError("PyEval_RestoreThread: NULL tstate");
if (gil_created()) { <~~~~ HERE
int err = errno;
take_gil(tstate);
/* _Py_Finalizing is protected by the GIL */
if (_Py_IsFinalizing() && !_Py_CURRENTLY_FINALIZING(tstate)) {
drop_gil(tstate);
PyThread_exit_thread();
Py_UNREACHABLE();
}
errno = err;
}
PyThreadState_Swap(tstate);
}I guess that the intent of dynamically created GIL is to reduce the "overhead" of the GIL when 100% of the code is run in single thread. I understand that this potential overhead must be measured to decide if it's ok to always create the GIL. This bug was reported once in 2014, then Marcin Kasperski asked for an update last month: 3 years later. It's not a common bug impacting many users. So if there is an overhead, I'm not sure that it's a good idea to change Python to fix this rare use case. |
I'm not sure it is ok to guess here. The original code was written in 1992, when system threading APIs were buggy and not always available. Also the original GIL logic released the lock every N opcodes, which is a fragile heuristic. The current GIL shouldn't have any visible performance cost in single-threaded mode, and in even in multi-threaded mode it's been a long time since anyone complained about a real-world performance degradation due to the GIL's overhead. Perhaps Guido remembers the original rationale?
From a maintenance POV, initializing the GIL eagerly also makes maintenance easier for us, instead of making all those dynamic checks. |
|
Yeah, the original reasoning was that threads were something esoteric and (Note: I haven't read the entire thread, just the first and last message.) |
|
I wrote the PR 4700 to create the GIL in Py_Initialize(). Would you be ok to backport this fix to Python 2.7 and 3.6 as well? |
|
For 2.7 I hesitate to OK this, who knows what skeletons are still in that |
|
+1 for making this change 3.6+ only. Victor, could you run your patch through the performance benchmarks? While I suspect Antoine is right that our current GIL management heuristics will mean we don't need the lazy initialisation optimisation any more, it's still preferable to have the specific numbers before merging it. |
|
I ran pyperformance on my PR 4700. Differences of at least 5%: haypo@speed-python$ python3 -m perf compare_to ~/json/uploaded/2017-12-18_12-29-master-bd6ec4d79e85.json.gz /home/haypo/json/patch/2017-12-18_12-29-master-bd6ec4d79e85-patch-4700.json.gz --table --min-speed=5 +----------------------+--------------------------------------+-------------------------------------------------+ Not significant (55): 2to3; chameleon; chaos; (...) |
I tested again these 5 benchmarks were Python was slower with my PR. I ran these benchmarks manually on my laptop using CPU isolation. Result: vstinner@apu$ python3 -m perf compare_to ref.json patch.json --table Ok, that was expected: no significant difference. |
|
I proposed PR 5421 to fix Python 3.6: modify Py_Initialize() to call PyEval_InitThreads(). I'm not sure of Python 2.7. Not only the backport is not straighforward, but I'm not sure that I want to touch the super-stable 2.7 branch. Maybe for 2.7, I should just remove the currently skipped test_bpo20891() test from test_capi. What do you think? |
|
Antoine Pitrou considers that my PR 5421 for Python 3.6 should not be merged: |
|
Antoine Pitrou:
Since only two users complained about https://bugs.python.org/issue20891 in 3 years, I agree that it's ok to not fix Python 2.7 and 3.6. The workaround is to call PyEval_InitThreads() before starting the first thread. I wasn't excited to make such stable in stable 2.7 and 3.6 anyway :-) test_embed.test_bpo20891() is enabled again on master. I now close the issue. Thanks Steve Dower for the bug report! |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: