Avoid GIL deadlocks by calling PyGILState_GetThisThreadState when using gil_scoped_acquire.

[bzarco]

This avoids GIL deadlocking when pybind11 tries to acquire the GIL in a thread that already acquired it using standard Python API (e.g. when running from a Python thread).

Simple code reproducing the problem:

#include <pybind11/pybind11.h>
#include <pybind11/functional.h>

PYBIND11_MODULE(example, m) {
  m.def("func", [](const std::function<void()> &func) { func(); });
}

import example

thread = threading.Thread(target=lambda: example.func(lambda: None))
thread.start()
thread.join()

I know this can be solved by releasing the GIL during C++ execution, but I don't think it should be something that is required (which currently is in a multi-threaded Python application). Moreover, PyGILState_Ensure doesn't deadlock when running the same code.

Please let me know if I am missing anything. And thank you for creating and maintaining such a great project!

[bzarco]

Some more details about this issue:

Currently in gil_scoped_acquire, a new thread state is created if none was saved under the internals.tstate key (if gil_scoped_acquire didn't create it earlier). But it could be that the thread state was created by the PyGILState_* API (e.g. if calling from a Python thread), which is saved under a different key.

The problem is that if we try to acquire the GIL in one of these threads (where state was created in PyGILState_Ensure), gil_scoped_acquire will create a new state and call PyEval_AcquireThread, which will deadlock (thread already has the GIL).

The solution proposed in this pull request checks if a thread state was created using the PyGILState_* API before creating a new one. If the state is found this way, it still uses the same logic as if created by gil_scoped_acquire (increasing its reference and decreasing it on destruction). Note there is no risk of deleting the state, since the reference count will be > 0 and it will be properly deleted by the creator (with the matching call to PyGILState_Release).

[wjakob]

This looks good to me. I've verified that it doesn't break the "advanced" use case in pybind11 (migrating Python to a different thread).

[jagerman]

Looks good to me. Should we tag this for 2.2.2? It's a "feature" of sorts, but the actual code change here is just one Python functional call.

[bzarco]

Thank you for taking a look @wjakob and @jagerman. Is there anything else that needs to be done before merging this change? (I just rebased with the latest master since it has been a while).

[wjakob]

It's fine to merge for me. However, there seems to be something fishy with the CI bots.. (potentially unrelated).

[bzarco]

Seems like an environment issue? Only one OS X run is failing:

Xcode: xcode9 C++ PYTHON=3.6 CPP=14 CLANG DEBUG=1

Error: python 2.7.13_1 is already installed
To upgrade to 3.6.4_3, run `brew upgrade python`

[heyer2]

Fixes a problem I've been struggling with for quite some time

[oremanj]

I'll put in a vote for this change as well - I ran into the problem it's fixing, and I don't see how py::gil_scoped_acquire ever worked on a non-main Python thread without this change.

[heyer2]

Probably time to merge this.

[davidbaraff]

This solved my problem as well, and I would also love to see it merged in asap.

[wjakob]

I re-ran the CI, and it passed all tests now. Apologies that it took quite a while to merge.

[knedlsepp]

@wjakob: Could we have a release with this patch included?

[wjakob]

@knedlsepp : huh? It was merged some time ago.

[knedlsepp]

I'm sorry about the noise. I did only check: e2b884c and github usually lists the tags/branches that contain a particular commit below the commit message. It didn't show that for me when I checked. I did again check the link and now it is listed as part of master and 2.3.0. Don't know why I couldn't observe it before. Thanks for getting back to me!