From 31e169c191ee0078c4d822a913254806604a7f61 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 16 Feb 2018 10:49:13 -0500 Subject: [PATCH] Add option to use the PyGILState API in gil_scoped_acquire (#1276) --- include/pybind11/options.h | 7 ++++ include/pybind11/pybind11.h | 78 ++++++++++++++++++++++--------------- tests/CMakeLists.txt | 1 + tests/conftest.py | 5 +++ tests/test_use_gilstate.cpp | 53 +++++++++++++++++++++++++ tests/test_use_gilstate.py | 31 +++++++++++++++ 6 files changed, 144 insertions(+), 31 deletions(-) create mode 100644 tests/test_use_gilstate.cpp create mode 100644 tests/test_use_gilstate.py diff --git a/include/pybind11/options.h b/include/pybind11/options.h index cc1e1f6f0f..e3afa34cb6 100644 --- a/include/pybind11/options.h +++ b/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() { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 977045d623..8a36a3a007 100644 --- a/include/pybind11/pybind11.h +++ b/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; }; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8f2f300ef7..3258736cbb 100644 --- a/tests/CMakeLists.txt +++ b/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 ) diff --git a/tests/conftest.py b/tests/conftest.py index f4c228260b..8630f829c9 100644 --- a/tests/conftest.py +++ b/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"), diff --git a/tests/test_use_gilstate.cpp b/tests/test_use_gilstate.cpp new file mode 100644 index 0000000000..83000a1247 --- /dev/null +++ b/tests/test_use_gilstate.cpp @@ -0,0 +1,53 @@ +#include "pybind11_tests.h" + +#if defined(WITH_THREAD) + +#include + +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()) + .def("check_default", []() -> bool { + py::gil_scoped_acquire acquire; + return check_internal(false); + }, py::call_guard()) + .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()); +} + +#endif diff --git a/tests/test_use_gilstate.py b/tests/test_use_gilstate.py new file mode 100644 index 0000000000..458459c688 --- /dev/null +++ b/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)