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:

1. main() calls Py_Initialize()
2. main() starts new C-level thread
3. main() calls Py_BEGIN_ALLOW_THREADS and waits for the new thread
4. New thread calls PyGILState_Ensure()
5. PyGILState_Ensure() sees that there is no thread state and calls PyEval_InitThreads() (this is what changed in the linked changeset)
6. PyEval_InitThreads() creates the GIL
7. PyEval_InitThreads() calls take_gil(PyThreadState_GET()) (this is what aborts - there is still no thread state)
8. (expected) PyGILState_Ensure() calls PyThreadState_New() and PyEval_RestoreThread().

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 #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…)

> Is this fix released? I can't find it in the changelog…

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.

> (I faced this bug on 3.5.2, released a couple of months after this bug was closed…)

The workaround is to call PyEval_InitThreads() before spawning your first non-Python thread.
https://docs.python.org/dev/c-api/init.html#c.PyEval_InitThreads

I have to check if Python 2.7 is impacted as well.

New changeset b4d1e1f7c1af6ae33f0e371576c8bcafedb099db by Victor Stinner in branch 'master':
bpo-20891: Fix PyGILState_Ensure() (#4650)
https://github.com/python/cpython/commit/b4d1e1f7c1af6ae33f0e371576c8bcafedb099db

New changeset be6b74c0795b709c7a04e2187a7e32d08f5155f6 by Victor Stinner in branch '2.7':
bpo-20891: Fix PyGILState_Ensure() (#4650) (#4657)
https://github.com/python/cpython/commit/be6b74c0795b709c7a04e2187a7e32d08f5155f6

New changeset e10c9de9d74fd4c26b32e6719d96f04a5be6987d by Victor Stinner in branch '3.6':
bpo-20891: Fix PyGILState_Ensure() (#4650) (#4655)
https://github.com/python/cpython/commit/e10c9de9d74fd4c26b32e6719d96f04a5be6987d

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 bpo20891" fails randomly on macOS:
---
macbook:master haypo$ while true; do ./Programs/_testembed bpo20891 ||break; date; done
Lun  4 déc 2017 12:46:34 CET
Lun  4 déc 2017 12:46:34 CET
Lun  4 déc 2017 12:46:34 CET
Fatal Python error: PyEval_SaveThread: NULL tstate

Current thread 0x00007fffa5dff3c0 (most recent call first):
Abort trap: 6
---

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"); <~~~ HERE

I'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?

> Why not *always* call PyEval_InitThreads() at interpreter initialization?  Are there any downsides?

The code is quite old:

commit 1984f1e1c6306d4e8073c28d2395638f80ea509b
Author: Guido van Rossum <guido@python.org>
Date:   Tue Aug 4 12:41:02 1992 +0000

    * Makefile adapted to changes below.
    * split pythonmain.c in two: most stuff goes to pythonrun.c, in the library.
    * new optional built-in threadmodule.c, build upon Sjoerd's thread.{c,h}.
    * new module from Sjoerd: mmmodule.c (dynamically loaded).
    * new module from Sjoerd: sv (svgen.py, svmodule.c.proto).
    * new files thread.{c,h} (from Sjoerd).
    * new xxmodule.c (example only).
    * myselect.h: bzero -> memset
    * select.c: bzero -> memset; removed global variable

(...)

+void
+init_save_thread()
+{
+#ifdef USE_THREAD
+       if (interpreter_lock)
+               fatal("2nd call to init_save_thread");
+       interpreter_lock = allocate_lock();
+       acquire_lock(interpreter_lock, 1);
+#endif
+}
+#endif


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 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'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?

> 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.

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
not used by most code, and at the time we definitely felt that always using
the GIL would cause a (tiny) slowdown and increase the risk of crashes due
to bugs in the GIL code. I'd be happy to learn that we no longer need to
worry about this and can just always initialize it.

(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
closet. It doesn't sound like a security fix to me. For 3.6 I'm fine with
it as a bugfix.

+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

+----------------------+--------------------------------------+-------------------------------------------------+
| Benchmark            | 2017-12-18_12-29-master-bd6ec4d79e85 | 2017-12-18_12-29-master-bd6ec4d79e85-patch-4700 |
+======================+======================================+=================================================+
| pathlib              | 41.8 ms                              | 44.3 ms: 1.06x slower (+6%)                     |
+----------------------+--------------------------------------+-------------------------------------------------+
| scimark_monte_carlo  | 197 ms                               | 210 ms: 1.07x slower (+7%)                      |
+----------------------+--------------------------------------+-------------------------------------------------+
| spectral_norm        | 243 ms                               | 269 ms: 1.11x slower (+11%)                     |
+----------------------+--------------------------------------+-------------------------------------------------+
| sqlite_synth         | 7.30 us                              | 8.13 us: 1.11x slower (+11%)                    |
+----------------------+--------------------------------------+-------------------------------------------------+
| unpickle_pure_python | 707 us                               | 796 us: 1.13x slower (+13%)                     |
+----------------------+--------------------------------------+-------------------------------------------------+

Not significant (55): 2to3; chameleon; chaos; (...)

New changeset 550ee051d605b909dd75ef686d8e1244a0994394 by Victor Stinner in branch 'master':
bpo-20891: Skip test_embed.test_bpo20891() (#4967)
https://github.com/python/cpython/commit/550ee051d605b909dd75ef686d8e1244a0994394

New changeset 2e1ef00171179a8b906631b175cde2e68a804522 by Victor Stinner in branch '3.6':
bpo-20891: Skip test_embed.test_bpo20891() (#4967) (#4969)
https://github.com/python/cpython/commit/2e1ef00171179a8b906631b175cde2e68a804522

> I ran pyperformance on my PR 4700. Differences of at least 5%: (...)

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
Not significant (5): unpickle_pure_python; sqlite_synth; spectral_norm; pathlib; scimark_monte_carlo

Ok, that was expected: no significant difference.

New changeset 2914bb32e2adf8dff77c0ca58b33201bc94e398c by Victor Stinner in branch 'master':
bpo-20891: Py_Initialize() now creates the GIL (#4700)
https://github.com/python/cpython/commit/2914bb32e2adf8dff77c0ca58b33201bc94e398c

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:
"@vstinner I don't think so. People can already call PyEval_InitThreads."
https://github.com/python/cpython/pull/5421#issuecomment-361214537

New changeset d951157268b2122109098c792562b71ccc41920b by Victor Stinner in branch 'master':
bpo-20891: Reenable test_embed.test_bpo20891() (GH-5420)
https://github.com/python/cpython/commit/d951157268b2122109098c792562b71ccc41920b

Antoine Pitrou:
> @vstinner I don't think so. People can already call PyEval_InitThreads.

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!

New changeset 0cecc22842dcc4090eb9cb99e7dababea7034a87 by Victor Stinner in branch '3.6':
bpo-20891: Remove test_capi.test_bpo20891() (#5425)
https://github.com/python/cpython/commit/0cecc22842dcc4090eb9cb99e7dababea7034a87