From 34e3b5932c2c98f327b274c3cb83e954e6eb94a4 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sat, 6 Oct 2018 12:45:47 +0100 Subject: [PATCH] Resolve threading issue with gil_scoped_acquire It was possible for python threads to execute c++ code which would subsequently call gil_scoped_acquire, incorrectly conclude that the gil was not held, and then deadlock. --- docs/changelog.rst | 7 ++++ include/pybind11/pybind11.h | 73 ++++++++++++++++++++++++++++++------- tests/test_callbacks.cpp | 3 ++ tests/test_callbacks.py | 18 +++++++++ 4 files changed, 88 insertions(+), 13 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index eb25578c31..a2e9ea485f 100644 --- a/docs/changelog.rst +++ b/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 `_. +* 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) ----------------------------------------------------- diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 14ea8dc396..af06b31556 100644 --- a/include/pybind11/pybind11.h +++ b/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; diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index 273eacc30c..f78fe4fb73 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -146,4 +146,7 @@ TEST_SUBMODULE(callbacks, m) { py::class_(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 f) { f(); }); } diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index 93c42c22b8..79edb87dc2 100644 --- a/tests/test_callbacks.py +++ b/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()