Add option to use the PyGILState API in gil_scoped_acquire (pybind#1276)

Author: KyleFromKitware

include/pybind11/options.h

@@ -38,12 +38,18 @@ class options {
 
     options& enable_function_signatures() & { global_state().show_function_signatures = true; return *this; }
 
+    options& disable_use_gilstate() & { global_state().use_gilstate = false; return *this; }
+
+    options& enable_use_gilstate() & { global_state().use_gilstate = true; return *this; }
+
     // Getter methods (return the global state):
 
     static bool show_user_defined_docstrings() { return global_state().show_user_defined_docstrings; }
 
     static bool show_function_signatures() { return global_state().show_function_signatures; }
 
+    static bool use_gilstate() { return global_state().use_gilstate; }
+
     // This type is not meant to be allocated on the heap.
     void* operator new(size_t) = delete;
 
@@ -52,6 +58,7 @@ class options {
     struct state {
         bool show_user_defined_docstrings = true;  //< Include user-supplied texts in docstrings.
         bool show_function_signatures = true;      //< Include auto-generated function signatures in docstrings.
+        bool use_gilstate = false;                 //< Use the PyGILState API in gil_scoped_acquire.
     };
 
     static state &global_state() {

include/pybind11/pybind11.h

@@ -1749,42 +1749,53 @@ void print(Args &&...args) {
  * example which uses features 2 and 3 to migrate the Python thread of
  * execution to another thread (to run the event loop on the original thread,
  * in this case).
+ *
+ * Due to the fact that gil_scoped_acquire creates its own internal thread
+ * state, this can get out of sync with the PyGILState_* API's thread state,
+ * causing GIL deadlocks in complicated setups with third party code that
+ * calls the PyGILState_* functions. To force gil_scoped_acquire to use the
+ * PyGILState_* API, create a pybind11::options object and call
+ * options::enable_use_gilstate().
  */
 
 class gil_scoped_acquire {
 public:
     PYBIND11_NOINLINE gil_scoped_acquire() {
-        auto const &internals = detail::get_internals();
-        tstate = (PyThreadState *) PyThread_get_key_value(internals.tstate);
-
-        if (!tstate) {
-            tstate = PyThreadState_New(internals.istate);
-            #if !defined(NDEBUG)
-                if (!tstate)
-                    pybind11_fail("scoped_acquire: could not create thread state!");
-            #endif
-            tstate->gilstate_counter = 0;
-            #if PY_MAJOR_VERSION < 3
-                PyThread_delete_key_value(internals.tstate);
-            #endif
-            PyThread_set_key_value(internals.tstate, tstate);
+        if (options::use_gilstate()) {
+            state = PyGILState_Ensure();
         } else {
-            release = detail::get_thread_state_unchecked() != tstate;
-        }
+            auto const &internals = detail::get_internals();
+            tstate = (PyThreadState *) PyThread_get_key_value(internals.tstate);
 
-        if (release) {
-            /* Work around an annoying assertion in PyThreadState_Swap */
-            #if defined(Py_DEBUG)
-                PyInterpreterState *interp = tstate->interp;
-                tstate->interp = nullptr;
-            #endif
-            PyEval_AcquireThread(tstate);
-            #if defined(Py_DEBUG)
-                tstate->interp = interp;
-            #endif
-        }
+            if (!tstate) {
+                tstate = PyThreadState_New(internals.istate);
+                #if !defined(NDEBUG)
+                    if (!tstate)
+                        pybind11_fail("scoped_acquire: could not create thread state!");
+                #endif
+                tstate->gilstate_counter = 0;
+                #if PY_MAJOR_VERSION < 3
+                    PyThread_delete_key_value(internals.tstate);
+                #endif
+                PyThread_set_key_value(internals.tstate, tstate);
+            } else {
+                release = detail::get_thread_state_unchecked() != tstate;
+            }
 
-        inc_ref();
+            if (release) {
+                /* Work around an annoying assertion in PyThreadState_Swap */
+                #if defined(Py_DEBUG)
+                    PyInterpreterState *interp = tstate->interp;
+                    tstate->interp = nullptr;
+                #endif
+                PyEval_AcquireThread(tstate);
+                #if defined(Py_DEBUG)
+                    tstate->interp = interp;
+                #endif
+            }
+
+            inc_ref();
+        }
     }
 
     void inc_ref() {
@@ -1812,12 +1823,17 @@ class gil_scoped_acquire {
     }
 
     PYBIND11_NOINLINE ~gil_scoped_acquire() {
-        dec_ref();
-        if (release)
-           PyEval_SaveThread();
+        if (options::use_gilstate()) {
+            PyGILState_Release(state);
+        } else {
+            dec_ref();
+            if (release)
+               PyEval_SaveThread();
+        }
     }
 private:
     PyThreadState *tstate = nullptr;
+    PyGILState_STATE state;
     bool release = true;
 };
 

tests/CMakeLists.txt

@@ -57,6 +57,7 @@ set(PYBIND11_TEST_FILES
   test_smart_ptr.cpp
   test_stl.cpp
   test_stl_binders.cpp
+  test_use_gilstate.cpp
   test_virtual_functions.cpp
 )
 

tests/conftest.py

@@ -199,6 +199,10 @@ def pytest_namespace():
         from pybind11_tests.eigen import have_eigen
     except ImportError:
         have_eigen = False
+    try:
+        import threading
+    except ImportError:
+        threading = None
     pypy = platform.python_implementation() == "PyPy"
 
     skipif = pytest.mark.skipif
@@ -210,6 +214,7 @@ def pytest_namespace():
                                            reason="eigen and/or numpy are not installed"),
         'requires_eigen_and_scipy': skipif(not have_eigen or not scipy,
                                            reason="eigen and/or scipy are not installed"),
+        'requires_threading': skipif(not threading, reason="no threading"),
         'unsupported_on_pypy': skipif(pypy, reason="unsupported on PyPy"),
         'unsupported_on_py2': skipif(sys.version_info.major < 3,
                                      reason="unsupported on Python 2.x"),

tests/test_use_gilstate.cpp

@@ -0,0 +1,53 @@
+#include "pybind11_tests.h"
+
+#if defined(WITH_THREAD)
+
+#include <thread>
+
+static bool check_internal(bool option_use_gilstate) {
+    auto const &internals = py::detail::get_internals();
+#if !defined(PYPY_VERSION)
+    if (option_use_gilstate)
+        return !PyThread_get_key_value(internals.tstate);
+    else
+        return !!PyThread_get_key_value(internals.tstate);
+#else
+    (void)option_use_gilstate;
+    return !PyThread_get_key_value(internals.tstate);
+#endif
+}
+
+TEST_SUBMODULE(use_gilstate, m) {
+    m.def("check_use_gilstate", [](bool option_use_gilstate) -> bool {
+        py::options options;
+        if (option_use_gilstate)
+            options.enable_use_gilstate();
+        else
+            options.disable_use_gilstate();
+        {
+            py::gil_scoped_acquire acquire;
+            return check_internal(option_use_gilstate);
+        }
+    }, py::call_guard<py::gil_scoped_release>())
+    .def("check_default", []() -> bool {
+        py::gil_scoped_acquire acquire;
+        return check_internal(false);
+    }, py::call_guard<py::gil_scoped_release>())
+    .def("check_use_gilstate_cthread", [](bool option_use_gilstate) -> bool {
+        py::options options;
+        if (option_use_gilstate)
+            options.enable_use_gilstate();
+        else
+            options.disable_use_gilstate();
+
+        bool result = false;
+        std::thread thread([option_use_gilstate, &result]() {
+            py::gil_scoped_acquire acquire;
+            result = check_internal(option_use_gilstate);
+        });
+        thread.join();
+        return result;
+    }, py::call_guard<py::gil_scoped_release>());
+}
+
+#endif

tests/test_use_gilstate.py

@@ -0,0 +1,31 @@
+import pytest
+
+pytestmark = pytest.requires_threading
+
+with pytest.suppress(ImportError):
+    import threading
+    from pybind11_tests import use_gilstate as ug
+
+
+def run_in_thread(target, args=()):
+    def thread_routine(target, args):
+        threading.current_thread()._return = target(*args)
+
+    thread = threading.Thread(target=thread_routine, args=(target, args))
+    thread.start()
+    thread.join()
+    return thread._return
+
+
+def test_use_gilstate():
+    assert run_in_thread(target=ug.check_use_gilstate, args=(False,))
+    assert run_in_thread(target=ug.check_use_gilstate, args=(True,))
+
+
+def test_default():
+    assert run_in_thread(target=ug.check_default)
+
+
+def test_cthread():
+    assert ug.check_use_gilstate_cthread(False)
+    assert ug.check_use_gilstate_cthread(True)