Resolve threading issue with gil_scoped_acquire

docs/changelog.rst

@@ -47,6 +47,13 @@ v2.3.0 (Not yet released)
 * ``pybind11_add_module()``: allow including Python as a ``SYSTEM`` include path.
   `#1416 <https://github.com/pybind/pybind11/pull/1416>`_.
 
+* Resolved an issue where std::function arguments to functions could create a deadlock
+  when called from threads other than the main thread.
+
+* py::gil_scoped_release no longer takes a boolean flag disassoc on construction,
+  a new method .detach() will provide this behaviour. The detach method can optionally
+  create a new thread context.
+
 v2.2.4 (September 11, 2018)
 -----------------------------------------------------
 

include/pybind11/pybind11.h

@@ -1854,17 +1854,31 @@ class gil_scoped_acquire {
         tstate = (PyThreadState *) PYBIND11_TLS_GET_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;
+            // pybind11 has no internals.tstate on this thread yet,
+            // however a thread state may exist in the underlying Python GIL API.
+            //
+            // This, for example, will happen between the point where a Python
+            // thread is created and gil_scoped_acquire is called in that thread
+            // for the first time.
+            tstate = PyGILState_GetThisThreadState();
+
+            if (!tstate) {
+                // no thread state - create new one
+                tstate = PyThreadState_New(internals.istate);
+                #if !defined(NDEBUG)
+                    if (!tstate)
+                        pybind11_fail("scoped_acquire: could not create thread state!");
+                #endif
+                tstate->gilstate_counter = 0;
+            }
+
             PYBIND11_TLS_REPLACE_VALUE(internals.tstate, tstate);
-        } else {
-            release = detail::get_thread_state_unchecked() != tstate;
         }
 
+        // Check if the current Python thread state in the underlying API
+        // is equal to this thread's state stored in internals
+        release = detail::get_thread_state_unchecked() != tstate;
+
         if (release) {
             /* Work around an annoying assertion in PyThreadState_Swap */
             #if defined(Py_DEBUG)
@@ -1916,17 +1930,50 @@ class gil_scoped_acquire {
 
 class gil_scoped_release {
 public:
-    explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) {
+    explicit gil_scoped_release() : disassoc(false) {
         // `get_internals()` must be called here unconditionally in order to initialize
         // `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an
         // initialization race could occur as multiple threads try `gil_scoped_acquire`.
-        const auto &internals = detail::get_internals();
+        detail::get_internals();
         tstate = PyEval_SaveThread();
-        if (disassoc) {
-            auto key = internals.tstate;
-            PYBIND11_TLS_DELETE_VALUE(key);
+
+    }
+
+    // Disassociate the current c++ thread from its Python thread.
+    //
+    // If create_new_tstate is set to true, a new Python thread context is created
+    // which will be used by future gil_scoped_acquire guards.
+    //
+    // If create_new_tstate is set to false, a future gil_scoped_acquire will detect
+    // the original Python thread's existence and reassociate the c++ thread.
+    //
+    // This is designed for advanced usage potentially transferring Python thread
+    // contexts between threads.
+    //
+    // When this gil_scoped_release guard falls out of scope, then the Python and
+    // C++ threads will be reassociated.
+    void detach(bool create_new_tstate) {
+        if (disassoc)
+            pybind11_fail("scoped_release: already detached!");
+
+        disassoc = true;
+
+        const auto &internals = detail::get_internals();
+        auto key = internals.tstate;
+        PYBIND11_TLS_DELETE_VALUE(key);
+
+        if (create_new_tstate) {
+            const auto new_tstate = PyThreadState_New(internals.istate);
+            #if !defined(NDEBUG)
+            if (!new_tstate)
+                pybind11_fail("scoped_release: could not create thread state!");
+            #endif
+            new_tstate->gilstate_counter = 0;
+
+            PYBIND11_TLS_REPLACE_VALUE(key, new_tstate);
         }
     }
+
     ~gil_scoped_release() {
         if (!tstate)
             return;

tests/test_callbacks.cpp

@@ -146,4 +146,7 @@ TEST_SUBMODULE(callbacks, m) {
     py::class_<CppBoundMethodTest>(m, "CppBoundMethodTest")
         .def(py::init<>())
         .def("triple", [](CppBoundMethodTest &, int val) { return 3 * val; });
+
+    // test_deadlock one-callback function
+    m.def("callback_void_std_function", [](std::function<void()> f) { f(); });
 }

tests/test_callbacks.py

@@ -105,3 +105,21 @@ def test_function_signatures(doc):
 
 def test_movable_object():
     assert m.callback_with_movable(lambda _: None) is True
+
+
+def test_no_deadlock():
+    # This code would deadlock - see issue 1525
+
+    def f():
+        pass
+
+    import threading
+
+    thread1 = threading.Thread(target=m.callback_void_std_function, args=(f,))
+    thread1.start()
+
+    thread2 = threading.Thread(target=m.callback_void_std_function, args=(f,))
+    thread2.start()
+
+    thread1.join()
+    thread2.join()