From 75acc11895b6aa4ecf1fe81cf89896f597334937 Mon Sep 17 00:00:00 2001 From: Arnim Balzer Date: Wed, 5 Oct 2022 09:40:40 +0100 Subject: [PATCH 01/34] Add option to force the use of the PYPY GIL scoped acquire/release logic to support nested gil access, see https://github.com/pybind/pybind11/issues/1276 and https://github.com/pytorch/pytorch/issues/83101 --- CMakeLists.txt | 5 +++++ include/pybind11/gil.h | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3284e21eb4..48506c9399 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -91,10 +91,15 @@ endif() option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_NOPYTHON "Disable search for Python" OFF) +option(PYBIND11_USE_PYPY_GIL "Force use of PYPY GIL acquire/release logic in gil.h" OFF) set(PYBIND11_INTERNALS_VERSION "" CACHE STRING "Override the ABI version, may be used to enable the unstable ABI.") +if(PYBIND11_USE_PYPY_GIL) + add_compile_definitions(USE_PYPY_GIL) +endif() + cmake_dependent_option( USE_PYTHON_INCLUDE_DIR "Install pybind11 headers in Python include directory instead of default installation prefix" diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 1ef5f0a8c8..59bd105a9c 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -21,7 +21,7 @@ PyThreadState *get_thread_state_unchecked(); PYBIND11_NAMESPACE_END(detail) -#if defined(WITH_THREAD) && !defined(PYPY_VERSION) +#if defined(WITH_THREAD) && !defined(PYPY_VERSION) && !defined(USE_PYPY_GIL) /* The functions below essentially reproduce the PyGILState_* API using a RAII * pattern, but there are a few important differences: @@ -178,7 +178,7 @@ class gil_scoped_release { bool disassoc; bool active = true; }; -#elif defined(PYPY_VERSION) +#elif defined(PYPY_VERSION) || defined(USE_PYPY_GIL) class gil_scoped_acquire { PyGILState_STATE state; From 1cd4cff768260bfdf1d13c5419aa200ebe134c26 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Fri, 21 Oct 2022 17:05:59 -0400 Subject: [PATCH 02/34] Apply suggestions from code review --- CMakeLists.txt | 6 +++--- include/pybind11/gil.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 48506c9399..01ae84311a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -91,13 +91,13 @@ endif() option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_NOPYTHON "Disable search for Python" OFF) -option(PYBIND11_USE_PYPY_GIL "Force use of PYPY GIL acquire/release logic in gil.h" OFF) +option(PYBIND11_SIMPLE_GIL "Force use of PYPY GIL acquire/release logic in gil.h" OFF) set(PYBIND11_INTERNALS_VERSION "" CACHE STRING "Override the ABI version, may be used to enable the unstable ABI.") -if(PYBIND11_USE_PYPY_GIL) - add_compile_definitions(USE_PYPY_GIL) +if(PYBIND11_SIMPLE_GIL) + add_compile_definitions(PYBIND11_SIMPLE_GIL) endif() cmake_dependent_option( diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 59bd105a9c..e709725f94 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -21,7 +21,7 @@ PyThreadState *get_thread_state_unchecked(); PYBIND11_NAMESPACE_END(detail) -#if defined(WITH_THREAD) && !defined(PYPY_VERSION) && !defined(USE_PYPY_GIL) +#if defined(WITH_THREAD) && !defined(PYPY_VERSION) && !defined(PYBIND11_SIMPLE_GIL) /* The functions below essentially reproduce the PyGILState_* API using a RAII * pattern, but there are a few important differences: @@ -178,7 +178,7 @@ class gil_scoped_release { bool disassoc; bool active = true; }; -#elif defined(PYPY_VERSION) || defined(USE_PYPY_GIL) +#elif defined(PYPY_VERSION) || defined(PYBIND11_SIMPLE_GIL) class gil_scoped_acquire { PyGILState_STATE state; From b855af205f1135d7f5ec2eadd84020e0212c33af Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Fri, 21 Oct 2022 17:34:43 -0400 Subject: [PATCH 03/34] Update CMakeLists.txt --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 01ae84311a..632da9bb02 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -91,7 +91,7 @@ endif() option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_NOPYTHON "Disable search for Python" OFF) -option(PYBIND11_SIMPLE_GIL "Force use of PYPY GIL acquire/release logic in gil.h" OFF) +option(PYBIND11_SIMPLE_GIL "Use simpler GIL access logic that does not support disassociation" OFF) set(PYBIND11_INTERNALS_VERSION "" CACHE STRING "Override the ABI version, may be used to enable the unstable ABI.") From fb233f0efc7fa2034d985cb05e08b4f3acd0117d Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sat, 22 Oct 2022 23:50:34 -0400 Subject: [PATCH 04/34] docs: update upgrade guide --- docs/upgrade.rst | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/upgrade.rst b/docs/upgrade.rst index 6a9db2d08f..5a32ddadeb 100644 --- a/docs/upgrade.rst +++ b/docs/upgrade.rst @@ -8,6 +8,22 @@ to a new version. But it goes into more detail. This includes things like deprecated APIs and their replacements, build system changes, general code modernization and other useful information. +.. _upgrade-guide-2.10: + +v2.10 +===== + +The current scoped GIL implementation doesn't support nested access. In pybind11 +In 2.10.1, a configuration option ``PYBIND11_SIMPLE_GIL`` was added, defaulting +to OFF; the simpler GIL implemntation supports nested access, but does not +support dissociation (the ``true`` parameter of ``gil_scope_release``). In +pybind11 2.11, we plan to change the default to ON. If you need the old +behavior, please set ``PYBIND11_SIMPLE_GIL`` to OFF. We plan to have an example +for manually supporting dissociation. + +There may be an unconfirmed ABI breakage between 2.9 and 2.10. We plan to bump +the internals number in 2.11. + .. _upgrade-guide-2.9: v2.9 From 7c4f8febe887aa7cd88afb3d64d19cba928439b0 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sat, 22 Oct 2022 23:53:44 -0400 Subject: [PATCH 05/34] Update docs/upgrade.rst --- docs/upgrade.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/upgrade.rst b/docs/upgrade.rst index 5a32ddadeb..63c3821d08 100644 --- a/docs/upgrade.rst +++ b/docs/upgrade.rst @@ -15,7 +15,7 @@ v2.10 The current scoped GIL implementation doesn't support nested access. In pybind11 In 2.10.1, a configuration option ``PYBIND11_SIMPLE_GIL`` was added, defaulting -to OFF; the simpler GIL implemntation supports nested access, but does not +to OFF; the simpler GIL implementation supports nested access, but does not support dissociation (the ``true`` parameter of ``gil_scope_release``). In pybind11 2.11, we plan to change the default to ON. If you need the old behavior, please set ``PYBIND11_SIMPLE_GIL`` to OFF. We plan to have an example From 6f7317e4b596d8a24dbbc8eb3641835abdd0a095 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 25 Oct 2022 23:12:59 -0700 Subject: [PATCH 06/34] All bells & whistles. --- .github/workflows/ci.yml | 4 ++ CMakeLists.txt | 7 ++-- docs/upgrade.rst | 18 ++++----- include/pybind11/detail/common.h | 4 ++ include/pybind11/detail/internals.h | 12 ++++++ include/pybind11/gil.h | 53 ++++++++++++++++++++------- tests/test_embed/test_interpreter.cpp | 1 - 7 files changed, 72 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b56e8f447f..a11cae1ab0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,10 +102,12 @@ jobs: run: python -m pip install pytest-github-actions-annotate-failures # First build - C++11 mode and inplace + # More-or-less randomly adding -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON here. - name: Configure C++11 ${{ matrix.args }} run: > cmake -S . -B . -DPYBIND11_WERROR=ON + -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=11 @@ -129,10 +131,12 @@ jobs: run: git clean -fdx # Second build - C++17 mode and in a build directory + # More-or-less randomly adding -DPYBIND11_SIMPLE_GIL_MANAGEMENT=OFF here. - name: Configure C++17 run: > cmake -S . -B build2 -DPYBIND11_WERROR=ON + -DPYBIND11_SIMPLE_GIL_MANAGEMENT=OFF -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=17 diff --git a/CMakeLists.txt b/CMakeLists.txt index 632da9bb02..0d93203881 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -91,13 +91,14 @@ endif() option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_NOPYTHON "Disable search for Python" OFF) -option(PYBIND11_SIMPLE_GIL "Use simpler GIL access logic that does not support disassociation" OFF) +option(PYBIND11_SIMPLE_GIL_MANAGEMENT + "Use simpler GIL management logic that does not support disassociation" OFF) set(PYBIND11_INTERNALS_VERSION "" CACHE STRING "Override the ABI version, may be used to enable the unstable ABI.") -if(PYBIND11_SIMPLE_GIL) - add_compile_definitions(PYBIND11_SIMPLE_GIL) +if(PYBIND11_SIMPLE_GIL_MANAGEMENT) + add_compile_definitions(PYBIND11_SIMPLE_GIL_MANAGEMENT) endif() cmake_dependent_option( diff --git a/docs/upgrade.rst b/docs/upgrade.rst index 63c3821d08..19b0b09b7d 100644 --- a/docs/upgrade.rst +++ b/docs/upgrade.rst @@ -13,16 +13,14 @@ modernization and other useful information. v2.10 ===== -The current scoped GIL implementation doesn't support nested access. In pybind11 -In 2.10.1, a configuration option ``PYBIND11_SIMPLE_GIL`` was added, defaulting -to OFF; the simpler GIL implementation supports nested access, but does not -support dissociation (the ``true`` parameter of ``gil_scope_release``). In -pybind11 2.11, we plan to change the default to ON. If you need the old -behavior, please set ``PYBIND11_SIMPLE_GIL`` to OFF. We plan to have an example -for manually supporting dissociation. - -There may be an unconfirmed ABI breakage between 2.9 and 2.10. We plan to bump -the internals number in 2.11. +``py::gil_scoped_acquire`` & ``py::gil_scoped_release`` in pybind11 versions +< v2.10.1 do not support nested access. In v2.10.1, a configuration option +``PYBIND11_SIMPLE_GIL_MANAGEMENT`` was added, defaulting to ``OFF``; the +simpler implementations support nested access, but do not support dissociation +(``py::gil_scoped_release(true)``). In pybind11 2.11, we plan to change the +default to ``ON``, to avoid pitfalls of the implementations with dissociation +(see #4216 for more information). Note that the dissociation feature is very +rarely used and not exercised in any pybind11 unit tests. .. _upgrade-guide-2.9: diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index b43100b95a..e446c78f43 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -229,6 +229,10 @@ # undef copysign #endif +#if defined(PYPY_VERSION) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +# define PYBIND11_SIMPLE_GIL_MANAGEMENT +#endif + #if defined(_MSC_VER) # if defined(PYBIND11_DEBUG_MARKER) # define _DEBUG diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index d47084e263..d2a77472b8 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -9,6 +9,10 @@ #pragma once +#if defined(WITH_THREAD) && defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +# include "../gil.h" +#endif + #include "../pytypes.h" #include @@ -169,10 +173,12 @@ struct internals { PyTypeObject *default_metaclass; PyObject *instance_base; #if defined(WITH_THREAD) + // Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: PYBIND11_TLS_KEY_INIT(tstate) # if PYBIND11_INTERNALS_VERSION > 4 PYBIND11_TLS_KEY_INIT(loader_life_support_tls_key) # endif // PYBIND11_INTERNALS_VERSION > 4 + // Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: PyInterpreterState *istate = nullptr; ~internals() { # if PYBIND11_INTERNALS_VERSION > 4 @@ -408,6 +414,10 @@ PYBIND11_NOINLINE internals &get_internals() { return **internals_pp; } +#if defined(WITH_THREAD) +# if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) + gil_scoped_acquire gil; +# else // Ensure that the GIL is held since we will need to make Python calls. // Cannot use py::gil_scoped_acquire here since that constructor calls get_internals. struct gil_scoped_acquire_local { @@ -417,6 +427,8 @@ PYBIND11_NOINLINE internals &get_internals() { ~gil_scoped_acquire_local() { PyGILState_Release(state); } const PyGILState_STATE state; } gil; +# endif +#endif error_scope err_scope; PYBIND11_STR_TYPE id(PYBIND11_INTERNALS_ID); diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index e709725f94..cb0028d505 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -10,7 +10,10 @@ #pragma once #include "detail/common.h" -#include "detail/internals.h" + +#if defined(WITH_THREAD) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +# include "detail/internals.h" +#endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -21,7 +24,9 @@ PyThreadState *get_thread_state_unchecked(); PYBIND11_NAMESPACE_END(detail) -#if defined(WITH_THREAD) && !defined(PYPY_VERSION) && !defined(PYBIND11_SIMPLE_GIL) +#if defined(WITH_THREAD) + +# if !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) /* The functions below essentially reproduce the PyGILState_* API using a RAII * pattern, but there are a few important differences: @@ -62,11 +67,11 @@ class gil_scoped_acquire { if (!tstate) { tstate = PyThreadState_New(internals.istate); -# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) +# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) if (!tstate) { pybind11_fail("scoped_acquire: could not create thread state!"); } -# endif +# endif tstate->gilstate_counter = 0; PYBIND11_TLS_REPLACE_VALUE(internals.tstate, tstate); } else { @@ -87,20 +92,20 @@ class gil_scoped_acquire { PYBIND11_NOINLINE void dec_ref() { --tstate->gilstate_counter; -# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) +# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) if (detail::get_thread_state_unchecked() != tstate) { pybind11_fail("scoped_acquire::dec_ref(): thread state must be current!"); } if (tstate->gilstate_counter < 0) { pybind11_fail("scoped_acquire::dec_ref(): reference count underflow!"); } -# endif +# endif if (tstate->gilstate_counter == 0) { -# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) +# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) if (!release) { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); } -# endif +# endif PyThreadState_Clear(tstate); if (active) { PyThreadState_DeleteCurrent(); @@ -178,12 +183,14 @@ class gil_scoped_release { bool disassoc; bool active = true; }; -#elif defined(PYPY_VERSION) || defined(PYBIND11_SIMPLE_GIL) + +# else // PYBIND11_SIMPLE_GIL_MANAGEMENT + class gil_scoped_acquire { PyGILState_STATE state; public: - gil_scoped_acquire() { state = PyGILState_Ensure(); } + gil_scoped_acquire() : state{PyGILState_Ensure()} {} gil_scoped_acquire(const gil_scoped_acquire &) = delete; gil_scoped_acquire &operator=(const gil_scoped_acquire &) = delete; ~gil_scoped_acquire() { PyGILState_Release(state); } @@ -194,19 +201,39 @@ class gil_scoped_release { PyThreadState *state; public: - gil_scoped_release() { state = PyEval_SaveThread(); } + gil_scoped_release() : state{PyEval_SaveThread()} {} gil_scoped_release(const gil_scoped_release &) = delete; gil_scoped_release &operator=(const gil_scoped_acquire &) = delete; ~gil_scoped_release() { PyEval_RestoreThread(state); } void disarm() {} }; -#else + +# endif // PYBIND11_SIMPLE_GIL_MANAGEMENT + +#else // WITH_THREAD + class gil_scoped_acquire { +public: + gil_scoped_acquire() { + // Trick to suppress `unused variable` error messages (at call sites). + (void) (this != (this + 1)); + } + gil_scoped_acquire(const gil_scoped_acquire &) = delete; + gil_scoped_acquire &operator=(const gil_scoped_acquire &) = delete; void disarm() {} }; + class gil_scoped_release { +public: + gil_scoped_release() { + // Trick to suppress `unused variable` error messages (at call sites). + (void) (this != (this + 1)); + } + gil_scoped_release(const gil_scoped_release &) = delete; + gil_scoped_release &operator=(const gil_scoped_acquire &) = delete; void disarm() {} }; -#endif + +#endif // WITH_THREAD PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 6299293b91..44dcd1fdb0 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -293,7 +293,6 @@ TEST_CASE("Threads") { { py::gil_scoped_release gil_release{}; - REQUIRE(has_pybind11_internals_static()); auto threads = std::vector(); for (auto i = 0; i < num_threads; ++i) { From cf1bb86022b5ca043f5e26134791e75dfb2c623c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 25 Oct 2022 23:55:11 -0700 Subject: [PATCH 07/34] Add Reminder to common.h, so that we will not forget to purge `!WITH_THREAD` branches when dropping Python 3.6 --- include/pybind11/detail/common.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index e446c78f43..a3e0bc9b37 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -206,6 +206,7 @@ #endif #include +// Reminder: WITH_THREAD is always defined if PY_VERSION_HEX >= 0x03070000 #if PY_VERSION_HEX < 0x03060000 # error "PYTHON < 3.6 IS UNSUPPORTED. pybind11 v2.9 was the last to support Python 2 and 3.5." #endif From 8885e790896b3590b50ac2298e0d446686decf8f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 26 Oct 2022 00:23:00 -0700 Subject: [PATCH 08/34] New sentence instead of semicolon. --- docs/upgrade.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/upgrade.rst b/docs/upgrade.rst index 19b0b09b7d..f5247273ed 100644 --- a/docs/upgrade.rst +++ b/docs/upgrade.rst @@ -15,7 +15,7 @@ v2.10 ``py::gil_scoped_acquire`` & ``py::gil_scoped_release`` in pybind11 versions < v2.10.1 do not support nested access. In v2.10.1, a configuration option -``PYBIND11_SIMPLE_GIL_MANAGEMENT`` was added, defaulting to ``OFF``; the +``PYBIND11_SIMPLE_GIL_MANAGEMENT`` was added, defaulting to ``OFF``. The simpler implementations support nested access, but do not support dissociation (``py::gil_scoped_release(true)``). In pybind11 2.11, we plan to change the default to ``ON``, to avoid pitfalls of the implementations with dissociation From f2538ba24d088bcfe43c4174eb43bd44cb223170 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 26 Oct 2022 11:19:42 -0700 Subject: [PATCH 09/34] Temporarily pull in snapshot of PR #4246 --- include/pybind11/detail/common.h | 9 +++++++++ include/pybind11/pytypes.h | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index a3e0bc9b37..4be11fa340 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -265,6 +265,15 @@ # define PYBIND11_HAS_U8STRING #endif +// See description of PR #4246: +#if !defined(NDEBUG) && !defined(PY_ASSERT_GIL_HELD_INCREF_DECREF) \ + && !(defined(PYPY_VERSION) \ + && defined(_MSC_VER)) /* PyPy Windows: pytest hangs indefinitely at the end of the \ + process (see PR #4268) */ \ + && !defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) +# define PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF +#endif + // #define PYBIND11_STR_LEGACY_PERMISSIVE // If DEFINED, pybind11::str can hold PyUnicodeObject or PyBytesObject // (probably surprising and never documented, but this was the diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 80a2e2228e..91707570d5 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -246,6 +246,11 @@ class handle : public detail::object_api { const handle &inc_ref() const & { #ifdef PYBIND11_HANDLE_REF_DEBUG inc_ref_counter(1); +#endif +#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) + if (m_ptr != nullptr && !PyGILState_Check()) { + throw std::runtime_error("pybind11::handle::inc_ref() PyGILState_Check() failure."); + } #endif Py_XINCREF(m_ptr); return *this; @@ -257,6 +262,11 @@ class handle : public detail::object_api { this function automatically. Returns a reference to itself. \endrst */ const handle &dec_ref() const & { +#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) + if (m_ptr != nullptr && !PyGILState_Check()) { + throw std::runtime_error("pybind11::handle::dec_ref() PyGILState_Check() failure."); + } +#endif Py_XDECREF(m_ptr); return *this; } From 52629ac0193512558416f205bf5f7b533e9964de Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 26 Oct 2022 11:25:23 -0700 Subject: [PATCH 10/34] Add `test_release_acquire` --- tests/test_gil_scoped.cpp | 5 +++++ tests/test_gil_scoped.py | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp index 97efdc1616..21e2eacc33 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -44,4 +44,9 @@ TEST_SUBMODULE(gil_scoped, m) { py::gil_scoped_release gil_release; gil_acquire(); }); + m.def("test_release_acquire", [](const py::object &obj) { + py::gil_scoped_release gil_released; + py::gil_scoped_acquire gil_acquired; + return py::str(obj); + }); } diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 52374b0cce..30e266ed3d 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -91,3 +91,7 @@ def test_python_to_cpp_to_python_from_process(): def test_cross_module_gil(): """Makes sure that the GIL can be acquired by another module from a GIL-released state.""" m.test_cross_module_gil() # Should not raise a SIGSEGV + + +def test_release_acquire(): + assert m.test_release_acquire(0xAB) == "171" From 40623878c3cc3b92af0674aef3a2905f1d243337 Mon Sep 17 00:00:00 2001 From: Arnim Balzer Date: Thu, 27 Oct 2022 10:27:25 +0100 Subject: [PATCH 11/34] Add more unit tests for nested gil locking --- tests/cross_module_gil_utils.cpp | 44 ++++++++++++++++++- tests/test_gil_scoped.cpp | 63 ++++++++++++++++++++++++--- tests/test_gil_scoped.py | 73 +++++++++++++++++++++++++++++++- 3 files changed, 171 insertions(+), 9 deletions(-) diff --git a/tests/cross_module_gil_utils.cpp b/tests/cross_module_gil_utils.cpp index 1436c35d6e..93ccdbcd24 100644 --- a/tests/cross_module_gil_utils.cpp +++ b/tests/cross_module_gil_utils.cpp @@ -9,6 +9,7 @@ #include #include +#include // This file mimics a DSO that makes pybind11 calls but does not define a // PYBIND11_MODULE. The purpose is to test that such a DSO can create a @@ -25,11 +26,43 @@ void gil_acquire() { py::gil_scoped_acquire gil; } constexpr char kModuleName[] = "cross_module_gil_utils"; +struct CustomAutoGIL { + CustomAutoGIL() : gstate(PyGILState_Ensure()) {} + ~CustomAutoGIL() { PyGILState_Release(gstate); } + + PyGILState_STATE gstate; +}; +struct CustomAutoNoGIL { + CustomAutoNoGIL() : save(PyEval_SaveThread()) {} + ~CustomAutoNoGIL() { PyEval_RestoreThread(save); } + + PyThreadState *save; +}; + +template +void gil_acquire_inner() { + Acquire acquire_outer; + Acquire acquire_inner; + Release release; +} + +template +void gil_acquire_nested() { + Acquire acquire_outer; + Acquire acquire_inner; + Release release; + auto thread = std::thread(&gil_acquire_inner); + thread.join(); +} + struct PyModuleDef moduledef = { PyModuleDef_HEAD_INIT, kModuleName, nullptr, 0, nullptr, nullptr, nullptr, nullptr, nullptr}; } // namespace +#define ADD_FUNCTION(Name, ...) \ + PyModule_AddObject(m, Name, PyLong_FromVoidPtr(reinterpret_cast(&__VA_ARGS__))); + extern "C" PYBIND11_EXPORT PyObject *PyInit_cross_module_gil_utils() { PyObject *m = PyModule_Create(&moduledef); @@ -37,8 +70,15 @@ extern "C" PYBIND11_EXPORT PyObject *PyInit_cross_module_gil_utils() { if (m != nullptr) { static_assert(sizeof(&gil_acquire) == sizeof(void *), "Function pointer must have the same size as void*"); - PyModule_AddObject( - m, "gil_acquire_funcaddr", PyLong_FromVoidPtr(reinterpret_cast(&gil_acquire))); + ADD_FUNCTION("gil_acquire_funcaddr", gil_acquire) + ADD_FUNCTION("gil_acquire_inner_custom_funcaddr", + gil_acquire_inner) + ADD_FUNCTION("gil_acquire_nested_custom_funcaddr", + gil_acquire_nested) + ADD_FUNCTION("gil_acquire_inner_pybind11_funcaddr", + gil_acquire_inner) + ADD_FUNCTION("gil_acquire_nested_pybind11_funcaddr", + gil_acquire_nested) } return m; diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp index 21e2eacc33..f0d708bf94 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -11,6 +11,10 @@ #include "pybind11_tests.h" +#define CROSS_MODULE(Function) \ + auto cm = py::module_::import("cross_module_gil_utils"); \ + auto target = reinterpret_cast(PyLong_AsVoidPtr(cm.attr(Function).ptr())); + class VirtClass { public: virtual ~VirtClass() = default; @@ -37,16 +41,65 @@ TEST_SUBMODULE(gil_scoped, m) { m.def("test_callback_std_func", [](const std::function &func) { func(); }); m.def("test_callback_virtual_func", [](VirtClass &virt) { virt.virtual_func(); }); m.def("test_callback_pure_virtual_func", [](VirtClass &virt) { virt.pure_virtual_func(); }); - m.def("test_cross_module_gil", []() { - auto cm = py::module_::import("cross_module_gil_utils"); - auto gil_acquire = reinterpret_cast( - PyLong_AsVoidPtr(cm.attr("gil_acquire_funcaddr").ptr())); + m.def("test_cross_module_gil_released", []() { + CROSS_MODULE("gil_acquire_funcaddr") py::gil_scoped_release gil_release; - gil_acquire(); + target(); + }); + m.def("test_cross_module_gil_acquired", []() { + CROSS_MODULE("gil_acquire_funcaddr") + py::gil_scoped_acquire gil_acquire; + target(); + }); + m.def("test_cross_module_gil_inner_custom_released", []() { + CROSS_MODULE("gil_acquire_inner_custom_funcaddr") + py::gil_scoped_release gil_release; + target(); + }); + m.def("test_cross_module_gil_inner_custom_acquired", []() { + CROSS_MODULE("gil_acquire_inner_custom_funcaddr") + py::gil_scoped_acquire gil_acquire; + target(); + }); + m.def("test_cross_module_gil_inner_pybind11_released", []() { + CROSS_MODULE("gil_acquire_inner_pybind11_funcaddr") + py::gil_scoped_release gil_release; + target(); + }); + m.def("test_cross_module_gil_inner_pybind11_acquired", []() { + CROSS_MODULE("gil_acquire_inner_pybind11_funcaddr") + py::gil_scoped_acquire gil_acquire; + target(); + }); + m.def("test_cross_module_gil_nested_custom_released", []() { + CROSS_MODULE("gil_acquire_nested_custom_funcaddr") + py::gil_scoped_release gil_release; + target(); + }); + m.def("test_cross_module_gil_nested_custom_acquired", []() { + CROSS_MODULE("gil_acquire_nested_custom_funcaddr") + py::gil_scoped_acquire gil_acquire; + target(); + }); + m.def("test_cross_module_gil_nested_pybind11_released", []() { + CROSS_MODULE("gil_acquire_nested_pybind11_funcaddr") + py::gil_scoped_release gil_release; + target(); + }); + m.def("test_cross_module_gil_nested_pybind11_acquired", []() { + CROSS_MODULE("gil_acquire_nested_pybind11_funcaddr") + py::gil_scoped_acquire gil_acquire; + target(); }); m.def("test_release_acquire", [](const py::object &obj) { py::gil_scoped_release gil_released; py::gil_scoped_acquire gil_acquired; return py::str(obj); }); + m.def("test_nested_acquire", [](const py::object &obj) { + py::gil_scoped_release gil_released; + py::gil_scoped_acquire gil_acquired_outer; + py::gil_scoped_acquire gil_acquired_inner; + return py::str(obj); + }); } diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 30e266ed3d..c669792c6c 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -33,6 +33,18 @@ def pure_virtual_func(self): m.test_callback_std_func(lambda: None) m.test_callback_virtual_func(extended) m.test_callback_pure_virtual_func(extended) + m.test_cross_module_gil_released() + m.test_cross_module_gil_acquired() + m.test_cross_module_gil_inner_custom_released() + m.test_cross_module_gil_inner_custom_acquired() + m.test_cross_module_gil_inner_pybind11_released() + m.test_cross_module_gil_inner_pybind11_acquired() + m.test_cross_module_gil_nested_custom_released() + m.test_cross_module_gil_nested_custom_acquired() + m.test_cross_module_gil_nested_pybind11_released() + # m.test_cross_module_gil_nested_pybind11_acquire() # this one dies in test_python_to_cpp_to_python_from_process + assert m.test_release_acquire(0xAB) == "171" + assert m.test_nested_acquire(0xAB) == "171" def _python_to_cpp_to_python_from_threads(num_threads, parallel=False): @@ -88,10 +100,67 @@ def test_python_to_cpp_to_python_from_process(): assert _run_in_process(_python_to_cpp_to_python) == 0 -def test_cross_module_gil(): +def test_cross_module_gil_released(): """Makes sure that the GIL can be acquired by another module from a GIL-released state.""" - m.test_cross_module_gil() # Should not raise a SIGSEGV + m.test_cross_module_gil_released() # Should not raise a SIGSEGV + + +def test_cross_module_gil_acquired(): + """Makes sure that the GIL can be acquired by another module from a GIL-acquired state.""" + m.test_cross_module_gil_acquired() # Should not raise a SIGSEGV + + +def test_cross_module_gil_inner_custom_released(): + """Makes sure that the GIL can be acquired/released by another module + from a GIL-released state using custom locking logic.""" + m.test_cross_module_gil_inner_custom_released() + + +def test_cross_module_gil_inner_custom_acquired(): + """Makes sure that the GIL can be acquired/acquired by another module + from a GIL-acquired state using custom locking logic.""" + m.test_cross_module_gil_inner_custom_acquired() + + +def test_cross_module_gil_inner_pybind11_released(): + """Makes sure that the GIL can be acquired/released by another module + from a GIL-released state using pybind11 locking logic.""" + m.test_cross_module_gil_inner_pybind11_released() + + +def test_cross_module_gil_inner_pybind11_acquired(): + """Makes sure that the GIL can be acquired/acquired by another module + from a GIL-acquired state using pybind11 locking logic.""" + m.test_cross_module_gil_inner_pybind11_acquired() + + +def test_cross_module_gil_nested_custom_released(): + """Makes sure that the GIL can be nested acquired/released by another module + from a GIL-released state using custom locking logic.""" + m.test_cross_module_gil_nested_custom_released() + + +def test_cross_module_gil_nested_custom_acquired(): + """Makes sure that the GIL can be nested acquired/acquired by another module + from a GIL-acquired state using custom locking logic.""" + m.test_cross_module_gil_nested_custom_acquired() + + +def test_cross_module_gil_nested_pybind11_released(): + """Makes sure that the GIL can be nested acquired/released by another module + from a GIL-released state using pybind11 locking logic.""" + m.test_cross_module_gil_nested_pybind11_released() + + +def test_cross_module_gil_nested_pybind11_acquired(): + """Makes sure that the GIL can be nested acquired/acquired by another module + from a GIL-acquired state using pybind11 locking logic.""" + m.test_cross_module_gil_nested_pybind11_acquired() def test_release_acquire(): assert m.test_release_acquire(0xAB) == "171" + + +def test_nested_acquire(): + assert m.test_nested_acquire(0xAB) == "171" From 31b3868c208e6b3f3722a5f0ae054a8951151f4a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Oct 2022 11:23:45 -0700 Subject: [PATCH 12/34] Add test_report_builtins_internals_keys --- tests/test_gil_scoped.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index c669792c6c..d7b4be4476 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -1,9 +1,21 @@ +import builtins import multiprocessing import threading +import pytest + from pybind11_tests import gil_scoped as m +def get_pybind11_internals_keys(): + keys = [] + for key in sorted(dir(builtins)): + if key.startswith("__pybind11_internals_"): + assert key.endswith("__") + keys.append(key) + return tuple(keys) + + def _run_in_process(target, *args, **kwargs): """Runs target in process and returns its exitcode after 10s (None if still alive).""" process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) @@ -110,6 +122,14 @@ def test_cross_module_gil_acquired(): m.test_cross_module_gil_acquired() # Should not raise a SIGSEGV +def test_report_builtins_internals_keys(): + """For reporting, not an actual test.""" + m.test_cross_module_gil_released() # Any test that imports cross_module_gil_utils + keys = get_pybind11_internals_keys() + assert len(keys) != 0 + pytest.skip("builtins internals keys: %s" % ", ".join(keys)) + + def test_cross_module_gil_inner_custom_released(): """Makes sure that the GIL can be acquired/released by another module from a GIL-released state using custom locking logic.""" From 87e50b06703f0c76e47c19f3024670584c2d0dde Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Oct 2022 14:01:37 -0700 Subject: [PATCH 13/34] Very minor enhancement: sort list only after filtering. --- tests/test_gil_scoped.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index d7b4be4476..2b9d915e1d 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -9,11 +9,11 @@ def get_pybind11_internals_keys(): keys = [] - for key in sorted(dir(builtins)): + for key in dir(builtins): if key.startswith("__pybind11_internals_"): assert key.endswith("__") keys.append(key) - return tuple(keys) + return tuple(sorted(keys)) def _run_in_process(target, *args, **kwargs): From fe7470d46d21e307dfb4cc216b355f0f65679ea1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 00:37:50 -0700 Subject: [PATCH 14/34] Revert change in docs/upgrade.rst --- docs/upgrade.rst | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/docs/upgrade.rst b/docs/upgrade.rst index f5247273ed..6a9db2d08f 100644 --- a/docs/upgrade.rst +++ b/docs/upgrade.rst @@ -8,20 +8,6 @@ to a new version. But it goes into more detail. This includes things like deprecated APIs and their replacements, build system changes, general code modernization and other useful information. -.. _upgrade-guide-2.10: - -v2.10 -===== - -``py::gil_scoped_acquire`` & ``py::gil_scoped_release`` in pybind11 versions -< v2.10.1 do not support nested access. In v2.10.1, a configuration option -``PYBIND11_SIMPLE_GIL_MANAGEMENT`` was added, defaulting to ``OFF``. The -simpler implementations support nested access, but do not support dissociation -(``py::gil_scoped_release(true)``). In pybind11 2.11, we plan to change the -default to ``ON``, to avoid pitfalls of the implementations with dissociation -(see #4216 for more information). Note that the dissociation feature is very -rarely used and not exercised in any pybind11 unit tests. - .. _upgrade-guide-2.9: v2.9 From 897c9dd546baa9071a56ed1367e9d4804b30c30b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 00:36:25 -0700 Subject: [PATCH 15/34] Add test_multi_acquire_release_cross_module, while also forcing unique PYBIND11_INTERNALS_VERSION for cross_module_gil_utils.cpp --- tests/cross_module_gil_utils.cpp | 25 ++++++++++++++++++++++- tests/test_gil_scoped.cpp | 29 +++++++++++++++++++++++++++ tests/test_gil_scoped.py | 34 ++++++++++++-------------------- 3 files changed, 66 insertions(+), 22 deletions(-) diff --git a/tests/cross_module_gil_utils.cpp b/tests/cross_module_gil_utils.cpp index 93ccdbcd24..ce6b012deb 100644 --- a/tests/cross_module_gil_utils.cpp +++ b/tests/cross_module_gil_utils.cpp @@ -6,9 +6,14 @@ All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. */ +#if defined(PYBIND11_INTERNALS_VERSION) +# undef PYBIND11_INTERNALS_VERSION +#endif +#define PYBIND11_INTERNALS_VERSION 21814642 // Ensure this module has its own `internals` instance. #include #include +#include #include // This file mimics a DSO that makes pybind11 calls but does not define a @@ -22,9 +27,24 @@ namespace { namespace py = pybind11; + void gil_acquire() { py::gil_scoped_acquire gil; } -constexpr char kModuleName[] = "cross_module_gil_utils"; +std::string gil_multi_acquire_release(unsigned bits) { + if (bits & 0x1u) { + py::gil_scoped_acquire gil; + } + if (bits & 0x2u) { + py::gil_scoped_release gil; + } + if (bits & 0x4u) { + py::gil_scoped_acquire gil; + } + if (bits & 0x8u) { + py::gil_scoped_release gil; + } + return PYBIND11_INTERNALS_ID; +} struct CustomAutoGIL { CustomAutoGIL() : gstate(PyGILState_Ensure()) {} @@ -55,6 +75,8 @@ void gil_acquire_nested() { thread.join(); } +constexpr char kModuleName[] = "cross_module_gil_utils"; + struct PyModuleDef moduledef = { PyModuleDef_HEAD_INIT, kModuleName, nullptr, 0, nullptr, nullptr, nullptr, nullptr, nullptr}; @@ -71,6 +93,7 @@ extern "C" PYBIND11_EXPORT PyObject *PyInit_cross_module_gil_utils() { static_assert(sizeof(&gil_acquire) == sizeof(void *), "Function pointer must have the same size as void*"); ADD_FUNCTION("gil_acquire_funcaddr", gil_acquire) + ADD_FUNCTION("gil_multi_acquire_release_funcaddr", gil_multi_acquire_release) ADD_FUNCTION("gil_acquire_inner_custom_funcaddr", gil_acquire_inner) ADD_FUNCTION("gil_acquire_nested_custom_funcaddr", diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp index f0d708bf94..8a681d843c 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -11,6 +11,9 @@ #include "pybind11_tests.h" +#include +#include + #define CROSS_MODULE(Function) \ auto cm = py::module_::import("cross_module_gil_utils"); \ auto target = reinterpret_cast(PyLong_AsVoidPtr(cm.attr(Function).ptr())); @@ -102,4 +105,30 @@ TEST_SUBMODULE(gil_scoped, m) { py::gil_scoped_acquire gil_acquired_inner; return py::str(obj); }); + m.def("test_multi_acquire_release_cross_module", [](unsigned bits) { + py::set internals_ids; + internals_ids.add(PYBIND11_INTERNALS_ID); + { + py::gil_scoped_release gil_released; + auto thread_f = [bits, &internals_ids]() { + py::gil_scoped_acquire gil_acquired; + auto cm = py::module_::import("cross_module_gil_utils"); + auto target = reinterpret_cast( + PyLong_AsVoidPtr(cm.attr("gil_multi_acquire_release_funcaddr").ptr())); + std::string cm_internals_id = target(bits >> 3); + internals_ids.add(cm_internals_id); + }; + if (bits & 0x1u) { + thread_f(); + } + if (bits & 0x2u) { + std::thread non_python_thread(std::move(thread_f)); + non_python_thread.join(); + } + if (bits & 0x4u) { + thread_f(); + } + } + return internals_ids; + }); } diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 2b9d915e1d..8eb7e93550 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -1,21 +1,9 @@ -import builtins import multiprocessing import threading -import pytest - from pybind11_tests import gil_scoped as m -def get_pybind11_internals_keys(): - keys = [] - for key in dir(builtins): - if key.startswith("__pybind11_internals_"): - assert key.endswith("__") - keys.append(key) - return tuple(sorted(keys)) - - def _run_in_process(target, *args, **kwargs): """Runs target in process and returns its exitcode after 10s (None if still alive).""" process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) @@ -45,6 +33,7 @@ def pure_virtual_func(self): m.test_callback_std_func(lambda: None) m.test_callback_virtual_func(extended) m.test_callback_pure_virtual_func(extended) + m.test_cross_module_gil_released() m.test_cross_module_gil_acquired() m.test_cross_module_gil_inner_custom_released() @@ -54,10 +43,15 @@ def pure_virtual_func(self): m.test_cross_module_gil_nested_custom_released() m.test_cross_module_gil_nested_custom_acquired() m.test_cross_module_gil_nested_pybind11_released() - # m.test_cross_module_gil_nested_pybind11_acquire() # this one dies in test_python_to_cpp_to_python_from_process + m.test_cross_module_gil_nested_pybind11_acquired() + assert m.test_release_acquire(0xAB) == "171" assert m.test_nested_acquire(0xAB) == "171" + for bits in range(16 * 8): + internals_ids = m.test_multi_acquire_release_cross_module(bits) + assert len(internals_ids) == 2 if bits % 8 else 1 + def _python_to_cpp_to_python_from_threads(num_threads, parallel=False): """Calls different C++ functions that come back to Python, from Python threads.""" @@ -122,14 +116,6 @@ def test_cross_module_gil_acquired(): m.test_cross_module_gil_acquired() # Should not raise a SIGSEGV -def test_report_builtins_internals_keys(): - """For reporting, not an actual test.""" - m.test_cross_module_gil_released() # Any test that imports cross_module_gil_utils - keys = get_pybind11_internals_keys() - assert len(keys) != 0 - pytest.skip("builtins internals keys: %s" % ", ".join(keys)) - - def test_cross_module_gil_inner_custom_released(): """Makes sure that the GIL can be acquired/released by another module from a GIL-released state using custom locking logic.""" @@ -184,3 +170,9 @@ def test_release_acquire(): def test_nested_acquire(): assert m.test_nested_acquire(0xAB) == "171" + + +def test_multi_acquire_release_cross_module(): + for bits in range(16 * 8): + internals_ids = m.test_multi_acquire_release_cross_module(bits) + assert len(internals_ids) == 2 if bits % 8 else 1 From 620cc702d0e0b532b3a9187561ae8fb1a3f4f411 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 01:14:08 -0700 Subject: [PATCH 16/34] Hopefully fix apparently new ICC error. ``` 2022-10-28T07:57:54.5187728Z -- The CXX compiler identification is Intel 2021.7.0.20220726 ... 2022-10-28T07:58:53.6758994Z icpc: remark #10441: The Intel(R) C++ Compiler Classic (ICC) is deprecated and will be removed from product release in the second half of 2023. The Intel(R) oneAPI DPC++/C++ Compiler (ICX) is the recommended compiler moving forward. Please transition to use this compiler. Use '-diag-disable=10441' to disable this message. 2022-10-28T07:58:54.5801597Z In file included from /home/runner/work/pybind11/pybind11/include/pybind11/detail/../detail/type_caster_base.h(15), 2022-10-28T07:58:54.5803794Z from /home/runner/work/pybind11/pybind11/include/pybind11/detail/../cast.h(15), 2022-10-28T07:58:54.5805740Z from /home/runner/work/pybind11/pybind11/include/pybind11/detail/../attr.h(14), 2022-10-28T07:58:54.5809556Z from /home/runner/work/pybind11/pybind11/include/pybind11/detail/class.h(12), 2022-10-28T07:58:54.5812154Z from /home/runner/work/pybind11/pybind11/include/pybind11/pybind11.h(13), 2022-10-28T07:58:54.5948523Z from /home/runner/work/pybind11/pybind11/tests/cross_module_gil_utils.cpp(13): 2022-10-28T07:58:54.5949009Z /home/runner/work/pybind11/pybind11/include/pybind11/detail/../detail/internals.h(177): error #2282: unrecognized GCC pragma 2022-10-28T07:58:54.5949374Z PYBIND11_TLS_KEY_INIT(tstate) 2022-10-28T07:58:54.5949579Z ^ 2022-10-28T07:58:54.5949695Z ``` --- include/pybind11/detail/internals.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index d2a77472b8..5a33cd8b93 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -53,7 +53,7 @@ inline PyObject *make_object_base_type(PyTypeObject *metaclass); // `Py_LIMITED_API` anyway. # if PYBIND11_INTERNALS_VERSION > 4 # define PYBIND11_TLS_KEY_REF Py_tss_t & -# ifdef __GNUC__ +# if defined(__GNUC__) && !defined(__INTEL_COMPILER) // Clang on macOS warns due to `Py_tss_NEEDS_INIT` not specifying an initializer // for every field. # define PYBIND11_TLS_KEY_INIT(var) \ From a8ac941ecf82ddf2caecf696f8fe7ec8e061b887 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 01:23:27 -0700 Subject: [PATCH 17/34] clang-tidy fixes --- tests/cross_module_gil_utils.cpp | 8 ++++---- tests/test_gil_scoped.cpp | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/cross_module_gil_utils.cpp b/tests/cross_module_gil_utils.cpp index ce6b012deb..7c20849dd9 100644 --- a/tests/cross_module_gil_utils.cpp +++ b/tests/cross_module_gil_utils.cpp @@ -31,16 +31,16 @@ namespace py = pybind11; void gil_acquire() { py::gil_scoped_acquire gil; } std::string gil_multi_acquire_release(unsigned bits) { - if (bits & 0x1u) { + if ((bits & 0x1u) != 0u) { py::gil_scoped_acquire gil; } - if (bits & 0x2u) { + if ((bits & 0x2u) != 0u) { py::gil_scoped_release gil; } - if (bits & 0x4u) { + if ((bits & 0x4u) != 0u) { py::gil_scoped_acquire gil; } - if (bits & 0x8u) { + if ((bits & 0x8u) != 0u) { py::gil_scoped_release gil; } return PYBIND11_INTERNALS_ID; diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp index 8a681d843c..d59e7cb7e9 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -118,14 +118,14 @@ TEST_SUBMODULE(gil_scoped, m) { std::string cm_internals_id = target(bits >> 3); internals_ids.add(cm_internals_id); }; - if (bits & 0x1u) { + if ((bits & 0x1u) != 0u) { thread_f(); } - if (bits & 0x2u) { - std::thread non_python_thread(std::move(thread_f)); + if ((bits & 0x2u) != 0u) { + std::thread non_python_thread(thread_f); non_python_thread.join(); } - if (bits & 0x4u) { + if ((bits & 0x4u) != 0u) { thread_f(); } } From e25bcd9258d128ff297ecd1c22f986bb66c63ffa Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 02:15:04 -0700 Subject: [PATCH 18/34] Workaround for PYPY WIN exitcode None --- tests/test_gil_scoped.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 8eb7e93550..a2ca901082 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -1,6 +1,9 @@ import multiprocessing import threading +import pytest + +import env from pybind11_tests import gil_scoped as m @@ -103,7 +106,10 @@ def test_python_to_cpp_to_python_from_process(): This test is for completion, but it was never an issue. """ - assert _run_in_process(_python_to_cpp_to_python) == 0 + exitcode = _run_in_process(_python_to_cpp_to_python) + if exitcode is None and env.PYPY and env.WIN: + pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") + assert exitcode == 0 def test_cross_module_gil_released(): From 1e72313b2bca252c89234f0a88aeba179f640b6f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 02:46:48 -0700 Subject: [PATCH 19/34] Revert "Temporarily pull in snapshot of PR #4246" This reverts commit 23ac16e859150f27fda25ca865cabcb4444e0770. --- include/pybind11/detail/common.h | 9 --------- include/pybind11/pytypes.h | 10 ---------- 2 files changed, 19 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 4be11fa340..a3e0bc9b37 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -265,15 +265,6 @@ # define PYBIND11_HAS_U8STRING #endif -// See description of PR #4246: -#if !defined(NDEBUG) && !defined(PY_ASSERT_GIL_HELD_INCREF_DECREF) \ - && !(defined(PYPY_VERSION) \ - && defined(_MSC_VER)) /* PyPy Windows: pytest hangs indefinitely at the end of the \ - process (see PR #4268) */ \ - && !defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) -# define PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF -#endif - // #define PYBIND11_STR_LEGACY_PERMISSIVE // If DEFINED, pybind11::str can hold PyUnicodeObject or PyBytesObject // (probably surprising and never documented, but this was the diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 91707570d5..80a2e2228e 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -246,11 +246,6 @@ class handle : public detail::object_api { const handle &inc_ref() const & { #ifdef PYBIND11_HANDLE_REF_DEBUG inc_ref_counter(1); -#endif -#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) - if (m_ptr != nullptr && !PyGILState_Check()) { - throw std::runtime_error("pybind11::handle::inc_ref() PyGILState_Check() failure."); - } #endif Py_XINCREF(m_ptr); return *this; @@ -262,11 +257,6 @@ class handle : public detail::object_api { this function automatically. Returns a reference to itself. \endrst */ const handle &dec_ref() const & { -#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) - if (m_ptr != nullptr && !PyGILState_Check()) { - throw std::runtime_error("pybind11::handle::dec_ref() PyGILState_Check() failure."); - } -#endif Py_XDECREF(m_ptr); return *this; } From af1d9eef855934e716eebcda98453feca9d7fc0f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 08:46:21 -0700 Subject: [PATCH 20/34] Another workaround for PYPY WIN exitcode None --- tests/test_gil_scoped.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index a2ca901082..bda4879cda 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -86,7 +86,10 @@ def test_python_to_cpp_to_python_from_thread_multiple_parallel(): It runs in a separate process to be able to stop and assert if it deadlocks. """ - assert _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True) == 0 + exitcode = _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True) + if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. + pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") + assert exitcode == 0 # TODO: FIXME on macOS Python 3.9 @@ -107,7 +110,7 @@ def test_python_to_cpp_to_python_from_process(): This test is for completion, but it was never an issue. """ exitcode = _run_in_process(_python_to_cpp_to_python) - if exitcode is None and env.PYPY and env.WIN: + if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") assert exitcode == 0 From 83c099d3662d0d0dbb26d77fa3b4b73c3b9cc121 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 09:12:13 -0700 Subject: [PATCH 21/34] Clean up how the tests are run "run in process" Part 1: uniformity --- tests/test_gil_scoped.py | 201 +++++++++++++++++++++------------------ 1 file changed, 106 insertions(+), 95 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index bda4879cda..fe58462269 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -7,55 +7,138 @@ from pybind11_tests import gil_scoped as m -def _run_in_process(target, *args, **kwargs): - """Runs target in process and returns its exitcode after 10s (None if still alive).""" - process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) - process.daemon = True - try: - process.start() - # Do not need to wait much, 10s should be more than enough. - process.join(timeout=10) - return process.exitcode - finally: - if process.is_alive(): - process.terminate() +class ExtendedVirtClass(m.VirtClass): + def virtual_func(self): + pass + def pure_virtual_func(self): + pass -def _python_to_cpp_to_python(): - """Calls different C++ functions that come back to Python.""" - class ExtendedVirtClass(m.VirtClass): - def virtual_func(self): - pass +def test_callback_py_obj(): + m.test_callback_py_obj(lambda: None) - def pure_virtual_func(self): - pass - extended = ExtendedVirtClass() - m.test_callback_py_obj(lambda: None) +def test_callback_std_func(): m.test_callback_std_func(lambda: None) + + +def test_callback_virtual_func(): + extended = ExtendedVirtClass() m.test_callback_virtual_func(extended) + + +def test_callback_pure_virtual_func(): + extended = ExtendedVirtClass() m.test_callback_pure_virtual_func(extended) - m.test_cross_module_gil_released() - m.test_cross_module_gil_acquired() + +def test_cross_module_gil_released(): + """Makes sure that the GIL can be acquired by another module from a GIL-released state.""" + m.test_cross_module_gil_released() # Should not raise a SIGSEGV + + +def test_cross_module_gil_acquired(): + """Makes sure that the GIL can be acquired by another module from a GIL-acquired state.""" + m.test_cross_module_gil_acquired() # Should not raise a SIGSEGV + + +def test_cross_module_gil_inner_custom_released(): + """Makes sure that the GIL can be acquired/released by another module + from a GIL-released state using custom locking logic.""" m.test_cross_module_gil_inner_custom_released() + + +def test_cross_module_gil_inner_custom_acquired(): + """Makes sure that the GIL can be acquired/acquired by another module + from a GIL-acquired state using custom locking logic.""" m.test_cross_module_gil_inner_custom_acquired() + + +def test_cross_module_gil_inner_pybind11_released(): + """Makes sure that the GIL can be acquired/released by another module + from a GIL-released state using pybind11 locking logic.""" m.test_cross_module_gil_inner_pybind11_released() + + +def test_cross_module_gil_inner_pybind11_acquired(): + """Makes sure that the GIL can be acquired/acquired by another module + from a GIL-acquired state using pybind11 locking logic.""" m.test_cross_module_gil_inner_pybind11_acquired() + + +def test_cross_module_gil_nested_custom_released(): + """Makes sure that the GIL can be nested acquired/released by another module + from a GIL-released state using custom locking logic.""" m.test_cross_module_gil_nested_custom_released() + + +def test_cross_module_gil_nested_custom_acquired(): + """Makes sure that the GIL can be nested acquired/acquired by another module + from a GIL-acquired state using custom locking logic.""" m.test_cross_module_gil_nested_custom_acquired() + + +def test_cross_module_gil_nested_pybind11_released(): + """Makes sure that the GIL can be nested acquired/released by another module + from a GIL-released state using pybind11 locking logic.""" m.test_cross_module_gil_nested_pybind11_released() + + +def test_cross_module_gil_nested_pybind11_acquired(): + """Makes sure that the GIL can be nested acquired/acquired by another module + from a GIL-acquired state using pybind11 locking logic.""" m.test_cross_module_gil_nested_pybind11_acquired() + +def test_release_acquire(): assert m.test_release_acquire(0xAB) == "171" + + +def test_nested_acquire(): assert m.test_nested_acquire(0xAB) == "171" + +def test_multi_acquire_release_cross_module(): for bits in range(16 * 8): internals_ids = m.test_multi_acquire_release_cross_module(bits) assert len(internals_ids) == 2 if bits % 8 else 1 +def _python_to_cpp_to_python(): + test_callback_py_obj() + test_callback_std_func() + test_callback_virtual_func() + test_callback_pure_virtual_func() + test_cross_module_gil_released() + test_cross_module_gil_acquired() + test_cross_module_gil_inner_custom_released() + test_cross_module_gil_inner_custom_acquired() + test_cross_module_gil_inner_pybind11_released() + test_cross_module_gil_inner_pybind11_acquired() + test_cross_module_gil_nested_custom_released() + test_cross_module_gil_nested_custom_acquired() + test_cross_module_gil_nested_pybind11_released() + test_cross_module_gil_nested_pybind11_acquired() + test_release_acquire() + test_nested_acquire() + test_multi_acquire_release_cross_module() + + +def _run_in_process(target, *args, **kwargs): + """Runs target in process and returns its exitcode after 10s (None if still alive).""" + process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) + process.daemon = True + try: + process.start() + # Do not need to wait much, 10s should be more than enough. + process.join(timeout=10) + return process.exitcode + finally: + if process.is_alive(): + process.terminate() + + def _python_to_cpp_to_python_from_threads(num_threads, parallel=False): """Calls different C++ functions that come back to Python, from Python threads.""" threads = [] @@ -113,75 +196,3 @@ def test_python_to_cpp_to_python_from_process(): if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") assert exitcode == 0 - - -def test_cross_module_gil_released(): - """Makes sure that the GIL can be acquired by another module from a GIL-released state.""" - m.test_cross_module_gil_released() # Should not raise a SIGSEGV - - -def test_cross_module_gil_acquired(): - """Makes sure that the GIL can be acquired by another module from a GIL-acquired state.""" - m.test_cross_module_gil_acquired() # Should not raise a SIGSEGV - - -def test_cross_module_gil_inner_custom_released(): - """Makes sure that the GIL can be acquired/released by another module - from a GIL-released state using custom locking logic.""" - m.test_cross_module_gil_inner_custom_released() - - -def test_cross_module_gil_inner_custom_acquired(): - """Makes sure that the GIL can be acquired/acquired by another module - from a GIL-acquired state using custom locking logic.""" - m.test_cross_module_gil_inner_custom_acquired() - - -def test_cross_module_gil_inner_pybind11_released(): - """Makes sure that the GIL can be acquired/released by another module - from a GIL-released state using pybind11 locking logic.""" - m.test_cross_module_gil_inner_pybind11_released() - - -def test_cross_module_gil_inner_pybind11_acquired(): - """Makes sure that the GIL can be acquired/acquired by another module - from a GIL-acquired state using pybind11 locking logic.""" - m.test_cross_module_gil_inner_pybind11_acquired() - - -def test_cross_module_gil_nested_custom_released(): - """Makes sure that the GIL can be nested acquired/released by another module - from a GIL-released state using custom locking logic.""" - m.test_cross_module_gil_nested_custom_released() - - -def test_cross_module_gil_nested_custom_acquired(): - """Makes sure that the GIL can be nested acquired/acquired by another module - from a GIL-acquired state using custom locking logic.""" - m.test_cross_module_gil_nested_custom_acquired() - - -def test_cross_module_gil_nested_pybind11_released(): - """Makes sure that the GIL can be nested acquired/released by another module - from a GIL-released state using pybind11 locking logic.""" - m.test_cross_module_gil_nested_pybind11_released() - - -def test_cross_module_gil_nested_pybind11_acquired(): - """Makes sure that the GIL can be nested acquired/acquired by another module - from a GIL-acquired state using pybind11 locking logic.""" - m.test_cross_module_gil_nested_pybind11_acquired() - - -def test_release_acquire(): - assert m.test_release_acquire(0xAB) == "171" - - -def test_nested_acquire(): - assert m.test_nested_acquire(0xAB) == "171" - - -def test_multi_acquire_release_cross_module(): - for bits in range(16 * 8): - internals_ids = m.test_multi_acquire_release_cross_module(bits) - assert len(internals_ids) == 2 if bits % 8 else 1 From ee8353e0451b5fb7f4f81eeac2705be8c928c928 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 10:46:13 -0700 Subject: [PATCH 22/34] Clean up how the tests are run "run in process" Part 2: use `@pytest.mark.parametrize` and clean up the naming. --- tests/test_gil_scoped.py | 76 ++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index fe58462269..63a9946bd8 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -105,24 +105,37 @@ def test_multi_acquire_release_cross_module(): assert len(internals_ids) == 2 if bits % 8 else 1 -def _python_to_cpp_to_python(): - test_callback_py_obj() - test_callback_std_func() - test_callback_virtual_func() - test_callback_pure_virtual_func() - test_cross_module_gil_released() - test_cross_module_gil_acquired() - test_cross_module_gil_inner_custom_released() - test_cross_module_gil_inner_custom_acquired() - test_cross_module_gil_inner_pybind11_released() - test_cross_module_gil_inner_pybind11_acquired() - test_cross_module_gil_nested_custom_released() - test_cross_module_gil_nested_custom_acquired() - test_cross_module_gil_nested_pybind11_released() - test_cross_module_gil_nested_pybind11_acquired() - test_release_acquire() - test_nested_acquire() - test_multi_acquire_release_cross_module() +# Intentionally putting human review in the loop here, to guard against accidents. +VARS_BEFORE_ALL_BASIC_TESTS = dict(vars()) # Make a copy of the dict (critical). +ALL_BASIC_TESTS = ( + test_callback_py_obj, + test_callback_std_func, + test_callback_virtual_func, + test_callback_pure_virtual_func, + test_cross_module_gil_released, + test_cross_module_gil_acquired, + test_cross_module_gil_inner_custom_released, + test_cross_module_gil_inner_custom_acquired, + test_cross_module_gil_inner_pybind11_released, + test_cross_module_gil_inner_pybind11_acquired, + test_cross_module_gil_nested_custom_released, + test_cross_module_gil_nested_custom_acquired, + test_cross_module_gil_nested_pybind11_released, + test_cross_module_gil_nested_pybind11_acquired, + test_release_acquire, + test_nested_acquire, + test_multi_acquire_release_cross_module, +) + + +def test_all_basic_tests_completeness(): + num_found = 0 + for key, value in VARS_BEFORE_ALL_BASIC_TESTS.items(): + if not key.startswith("test_"): + continue + assert value in ALL_BASIC_TESTS + num_found += 1 + assert len(ALL_BASIC_TESTS) == num_found def _run_in_process(target, *args, **kwargs): @@ -139,11 +152,10 @@ def _run_in_process(target, *args, **kwargs): process.terminate() -def _python_to_cpp_to_python_from_threads(num_threads, parallel=False): - """Calls different C++ functions that come back to Python, from Python threads.""" +def _run_in_threads(target, num_threads, parallel): threads = [] for _ in range(num_threads): - thread = threading.Thread(target=_python_to_cpp_to_python) + thread = threading.Thread(target=target) thread.daemon = True thread.start() if parallel: @@ -155,44 +167,46 @@ def _python_to_cpp_to_python_from_threads(num_threads, parallel=False): # TODO: FIXME, sometimes returns -11 (segfault) instead of 0 on macOS Python 3.9 -def test_python_to_cpp_to_python_from_thread(): +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +def test_run_in_process_one_thread(test_fn): """Makes sure there is no GIL deadlock when running in a thread. It runs in a separate process to be able to stop and assert if it deadlocks. """ - assert _run_in_process(_python_to_cpp_to_python_from_threads, 1) == 0 + assert _run_in_process(_run_in_threads, test_fn, num_threads=1, parallel=False) == 0 # TODO: FIXME on macOS Python 3.9 -def test_python_to_cpp_to_python_from_thread_multiple_parallel(): +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +def test_run_in_process_multiple_threads_parallel(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times in parallel. It runs in a separate process to be able to stop and assert if it deadlocks. """ - exitcode = _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True) + exitcode = _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=True) if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") assert exitcode == 0 # TODO: FIXME on macOS Python 3.9 -def test_python_to_cpp_to_python_from_thread_multiple_sequential(): +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +def test_run_in_process_multiple_threads_sequential(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times sequentially. It runs in a separate process to be able to stop and assert if it deadlocks. """ - assert ( - _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=False) == 0 - ) + assert _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=False) == 0 # TODO: FIXME on macOS Python 3.9 -def test_python_to_cpp_to_python_from_process(): +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +def test_run_in_process_direct(test_fn): """Makes sure there is no GIL deadlock when using processes. This test is for completion, but it was never an issue. """ - exitcode = _run_in_process(_python_to_cpp_to_python) + exitcode = _run_in_process(test_fn) if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") assert exitcode == 0 From 5019457a6857c617162035f7bca22f475b60433a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 11:42:05 -0700 Subject: [PATCH 23/34] Skip some tests `#if defined(THREAD_SANITIZER)` (tested with TSAN using the Google-internal toolchain). --- tests/test_gil_scoped.cpp | 7 +++++++ tests/test_gil_scoped.py | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp index d59e7cb7e9..b9b80826e9 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -35,6 +35,13 @@ class PyVirtClass : public VirtClass { }; TEST_SUBMODULE(gil_scoped, m) { + m.attr("defined_THREAD_SANITIZER") = +#if defined(THREAD_SANITIZER) + true; +#else + false; +#endif + py::class_(m, "VirtClass") .def(py::init<>()) .def("virtual_func", &VirtClass::virtual_func) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 63a9946bd8..d929feaf20 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -166,7 +166,13 @@ def _run_in_threads(target, num_threads, parallel): thread.join() +# m.defined_THREAD_SANITIZER is used below to skip tests triggering this error (#2754): +# ThreadSanitizer: starting new threads after multi-threaded fork is not supported. + # TODO: FIXME, sometimes returns -11 (segfault) instead of 0 on macOS Python 3.9 +@pytest.mark.skipif( + m.defined_THREAD_SANITIZER, reason="Not compatible with ThreadSanitizer" +) @pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) def test_run_in_process_one_thread(test_fn): """Makes sure there is no GIL deadlock when running in a thread. @@ -177,6 +183,9 @@ def test_run_in_process_one_thread(test_fn): # TODO: FIXME on macOS Python 3.9 +@pytest.mark.skipif( + m.defined_THREAD_SANITIZER, reason="Not compatible with ThreadSanitizer" +) @pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) def test_run_in_process_multiple_threads_parallel(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times in parallel. @@ -190,6 +199,9 @@ def test_run_in_process_multiple_threads_parallel(test_fn): # TODO: FIXME on macOS Python 3.9 +@pytest.mark.skipif( + m.defined_THREAD_SANITIZER, reason="Not compatible with ThreadSanitizer" +) @pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) def test_run_in_process_multiple_threads_sequential(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times sequentially. @@ -206,6 +218,14 @@ def test_run_in_process_direct(test_fn): This test is for completion, but it was never an issue. """ + if m.defined_THREAD_SANITIZER and test_fn in ( + test_cross_module_gil_nested_custom_released, + test_cross_module_gil_nested_custom_acquired, + test_cross_module_gil_nested_pybind11_released, + test_cross_module_gil_nested_pybind11_acquired, + test_multi_acquire_release_cross_module, + ): + pytest.skip("Not compatible with ThreadSanitizer") exitcode = _run_in_process(test_fn) if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") From ab453376b522eaaa6dbd31f32a92e4428361bca5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 12:26:17 -0700 Subject: [PATCH 24/34] Run all tests again but ignore ThreadSanitizer exitcode 66 (this is less likely to mask unrelated ThreadSanitizer issues in the future). --- tests/test_gil_scoped.py | 42 +++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index d929feaf20..ed1a1b6efd 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -138,6 +138,12 @@ def test_all_basic_tests_completeness(): assert len(ALL_BASIC_TESTS) == num_found +# Issue #2754: +ThreadSanitizer_exitcode_66_message = ( + "ThreadSanitizer: starting new threads after multi-threaded fork is not supported." +) + + def _run_in_process(target, *args, **kwargs): """Runs target in process and returns its exitcode after 10s (None if still alive).""" process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) @@ -146,6 +152,8 @@ def _run_in_process(target, *args, **kwargs): process.start() # Do not need to wait much, 10s should be more than enough. process.join(timeout=10) + if process.exitcode == 66: + pass # NICE-TO-HAVE: Check output for ThreadSanitizer_exitcode_66_message return process.exitcode finally: if process.is_alive(): @@ -166,26 +174,20 @@ def _run_in_threads(target, num_threads, parallel): thread.join() -# m.defined_THREAD_SANITIZER is used below to skip tests triggering this error (#2754): -# ThreadSanitizer: starting new threads after multi-threaded fork is not supported. - # TODO: FIXME, sometimes returns -11 (segfault) instead of 0 on macOS Python 3.9 -@pytest.mark.skipif( - m.defined_THREAD_SANITIZER, reason="Not compatible with ThreadSanitizer" -) @pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) def test_run_in_process_one_thread(test_fn): """Makes sure there is no GIL deadlock when running in a thread. It runs in a separate process to be able to stop and assert if it deadlocks. """ - assert _run_in_process(_run_in_threads, test_fn, num_threads=1, parallel=False) == 0 + exitcode = _run_in_process(_run_in_threads, test_fn, num_threads=1, parallel=False) + if exitcode == 66 and m.defined_THREAD_SANITIZER: + pytest.skip(ThreadSanitizer_exitcode_66_message) + assert exitcode == 0 # TODO: FIXME on macOS Python 3.9 -@pytest.mark.skipif( - m.defined_THREAD_SANITIZER, reason="Not compatible with ThreadSanitizer" -) @pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) def test_run_in_process_multiple_threads_parallel(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times in parallel. @@ -195,20 +197,22 @@ def test_run_in_process_multiple_threads_parallel(test_fn): exitcode = _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=True) if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") + if exitcode == 66 and m.defined_THREAD_SANITIZER: + pytest.skip(ThreadSanitizer_exitcode_66_message) assert exitcode == 0 # TODO: FIXME on macOS Python 3.9 -@pytest.mark.skipif( - m.defined_THREAD_SANITIZER, reason="Not compatible with ThreadSanitizer" -) @pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) def test_run_in_process_multiple_threads_sequential(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times sequentially. It runs in a separate process to be able to stop and assert if it deadlocks. """ - assert _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=False) == 0 + exitcode = _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=False) + if exitcode == 66 and m.defined_THREAD_SANITIZER: + pytest.skip(ThreadSanitizer_exitcode_66_message) + assert exitcode == 0 # TODO: FIXME on macOS Python 3.9 @@ -218,15 +222,9 @@ def test_run_in_process_direct(test_fn): This test is for completion, but it was never an issue. """ - if m.defined_THREAD_SANITIZER and test_fn in ( - test_cross_module_gil_nested_custom_released, - test_cross_module_gil_nested_custom_acquired, - test_cross_module_gil_nested_pybind11_released, - test_cross_module_gil_nested_pybind11_acquired, - test_multi_acquire_release_cross_module, - ): - pytest.skip("Not compatible with ThreadSanitizer") exitcode = _run_in_process(test_fn) if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") + if exitcode == 66 and m.defined_THREAD_SANITIZER: + pytest.skip(ThreadSanitizer_exitcode_66_message) assert exitcode == 0 From 857ee1a5b8d81b65bb8a64929700bb4008cba961 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Oct 2022 16:05:17 -0700 Subject: [PATCH 25/34] bug fix: missing common.h include before using `PYBIND11_SIMPLE_GIL_MANAGEMENT` For the tests in the github CI this does not matter, because `PYBIND11_SIMPLE_GIL_MANAGEMENT` is always defined from the command line, but when monkey-patching common.h locally, it matters. --- include/pybind11/detail/internals.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 5a33cd8b93..7de7794344 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -9,6 +9,8 @@ #pragma once +#include "common.h" + #if defined(WITH_THREAD) && defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) # include "../gil.h" #endif From 10b0334974095f5d21b916aff11fe518ea8dbea2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 29 Oct 2022 01:34:42 -0700 Subject: [PATCH 26/34] if process.exitcode is None: assert t_delta > 9.9 --- tests/test_gil_scoped.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index ed1a1b6efd..16c53ad51f 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -1,5 +1,6 @@ import multiprocessing import threading +import time import pytest @@ -149,9 +150,13 @@ def _run_in_process(target, *args, **kwargs): process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) process.daemon = True try: + t_start = time.time() process.start() # Do not need to wait much, 10s should be more than enough. process.join(timeout=10) + t_delta = time.time() - t_start + if process.exitcode is None: + assert t_delta > 9.9 if process.exitcode == 66: pass # NICE-TO-HAVE: Check output for ThreadSanitizer_exitcode_66_message return process.exitcode From ea8b132c116888d7ea624f52c2f04952895c363e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 29 Oct 2022 17:58:40 -0700 Subject: [PATCH 27/34] More sophisiticated `_run_in_process()` implementation, clearly reporting `DEADLOCK`, additionally exercised via added `intentional_deadlock()` --- tests/test_gil_scoped.cpp | 3 ++ tests/test_gil_scoped.py | 73 +++++++++++++++++++-------------------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp index b9b80826e9..f136086e84 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -42,6 +42,9 @@ TEST_SUBMODULE(gil_scoped, m) { false; #endif + m.def("intentional_deadlock", + []() { std::thread([]() { py::gil_scoped_acquire gil_acquired; }).join(); }); + py::class_(m, "VirtClass") .def(py::init<>()) .def("virtual_func", &VirtClass::virtual_func) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 16c53ad51f..526745ee5e 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -4,7 +4,6 @@ import pytest -import env from pybind11_tests import gil_scoped as m @@ -139,36 +138,50 @@ def test_all_basic_tests_completeness(): assert len(ALL_BASIC_TESTS) == num_found -# Issue #2754: -ThreadSanitizer_exitcode_66_message = ( - "ThreadSanitizer: starting new threads after multi-threaded fork is not supported." -) +ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK = ALL_BASIC_TESTS + (m.intentional_deadlock,) def _run_in_process(target, *args, **kwargs): - """Runs target in process and returns its exitcode after 10s (None if still alive).""" + if len(args) == 0: + test_fn = target + else: + test_fn = args[0] + # Do not need to wait much, 10s should be more than enough. + timeout = 0.1 if test_fn is m.intentional_deadlock else 10 process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) process.daemon = True try: t_start = time.time() process.start() - # Do not need to wait much, 10s should be more than enough. - process.join(timeout=10) + if timeout >= 100: # For debugging. + print("\nprocess.pid STARTED", process.pid, flush=True) + process.join(timeout=timeout) + if timeout >= 100: + print("\nprocess.pid JOINED", process.pid, flush=True) t_delta = time.time() - t_start - if process.exitcode is None: - assert t_delta > 9.9 - if process.exitcode == 66: - pass # NICE-TO-HAVE: Check output for ThreadSanitizer_exitcode_66_message + if process.exitcode == 66 and m.defined_THREAD_SANITIZER: # Issue #2754 + # WOULD-BE-NICE-TO-HAVE: Check that the message below is actually in the output. + pytest.skip( + "ThreadSanitizer: starting new threads after multi-threaded fork is not supported." + ) + elif test_fn is m.intentional_deadlock: + assert process.exitcode is None + return 0 + elif process.exitcode is None: + assert t_delta > 0.9 * timeout + raise RuntimeError( + "DEADLOCK, most likely, exactly what this test is meant to detect." + ) return process.exitcode finally: if process.is_alive(): process.terminate() -def _run_in_threads(target, num_threads, parallel): +def _run_in_threads(test_fn, num_threads, parallel): threads = [] for _ in range(num_threads): - thread = threading.Thread(target=target) + thread = threading.Thread(target=test_fn) thread.daemon = True thread.start() if parallel: @@ -180,56 +193,40 @@ def _run_in_threads(target, num_threads, parallel): # TODO: FIXME, sometimes returns -11 (segfault) instead of 0 on macOS Python 3.9 -@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK) def test_run_in_process_one_thread(test_fn): """Makes sure there is no GIL deadlock when running in a thread. It runs in a separate process to be able to stop and assert if it deadlocks. """ - exitcode = _run_in_process(_run_in_threads, test_fn, num_threads=1, parallel=False) - if exitcode == 66 and m.defined_THREAD_SANITIZER: - pytest.skip(ThreadSanitizer_exitcode_66_message) - assert exitcode == 0 + assert _run_in_process(_run_in_threads, test_fn, num_threads=1, parallel=False) == 0 # TODO: FIXME on macOS Python 3.9 -@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK) def test_run_in_process_multiple_threads_parallel(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times in parallel. It runs in a separate process to be able to stop and assert if it deadlocks. """ - exitcode = _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=True) - if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. - pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") - if exitcode == 66 and m.defined_THREAD_SANITIZER: - pytest.skip(ThreadSanitizer_exitcode_66_message) - assert exitcode == 0 + assert _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=True) == 0 # TODO: FIXME on macOS Python 3.9 -@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK) def test_run_in_process_multiple_threads_sequential(test_fn): """Makes sure there is no GIL deadlock when running in a thread multiple times sequentially. It runs in a separate process to be able to stop and assert if it deadlocks. """ - exitcode = _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=False) - if exitcode == 66 and m.defined_THREAD_SANITIZER: - pytest.skip(ThreadSanitizer_exitcode_66_message) - assert exitcode == 0 + assert _run_in_process(_run_in_threads, test_fn, num_threads=8, parallel=False) == 0 # TODO: FIXME on macOS Python 3.9 -@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS) +@pytest.mark.parametrize("test_fn", ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK) def test_run_in_process_direct(test_fn): """Makes sure there is no GIL deadlock when using processes. This test is for completion, but it was never an issue. """ - exitcode = _run_in_process(test_fn) - if exitcode is None and env.PYPY and env.WIN: # Seems to be flaky. - pytest.skip("Ignoring unexpected exitcode None (PYPY WIN)") - if exitcode == 66 and m.defined_THREAD_SANITIZER: - pytest.skip(ThreadSanitizer_exitcode_66_message) - assert exitcode == 0 + assert _run_in_process(test_fn) == 0 From 4c19a9ae8d1b4beb19b5c4fa4436d6a14cb399e4 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 29 Oct 2022 20:16:02 -0700 Subject: [PATCH 28/34] Wrap m.intentional_deadlock in a Python function, for `ForkingPickler` compatibility. ``` > ForkingPickler(file, protocol).dump(obj) E TypeError: cannot pickle 'PyCapsule' object ``` Observed with all Windows builds including mingw but not PyPy, and macos-latest with Python 3.9, 3.10, 3.11 but not 3.6. --- tests/test_gil_scoped.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 526745ee5e..5f4510e4a7 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -138,7 +138,11 @@ def test_all_basic_tests_completeness(): assert len(ALL_BASIC_TESTS) == num_found -ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK = ALL_BASIC_TESTS + (m.intentional_deadlock,) +def _intentional_deadlock(): + m.intentional_deadlock() + + +ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK = ALL_BASIC_TESTS + (_intentional_deadlock,) def _run_in_process(target, *args, **kwargs): @@ -147,7 +151,7 @@ def _run_in_process(target, *args, **kwargs): else: test_fn = args[0] # Do not need to wait much, 10s should be more than enough. - timeout = 0.1 if test_fn is m.intentional_deadlock else 10 + timeout = 0.1 if test_fn is _intentional_deadlock else 10 process = multiprocessing.Process(target=target, args=args, kwargs=kwargs) process.daemon = True try: @@ -164,7 +168,7 @@ def _run_in_process(target, *args, **kwargs): pytest.skip( "ThreadSanitizer: starting new threads after multi-threaded fork is not supported." ) - elif test_fn is m.intentional_deadlock: + elif test_fn is _intentional_deadlock: assert process.exitcode is None return 0 elif process.exitcode is None: From 2c5f864514afea442c2123826e02ecb35f5bf0d2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 29 Oct 2022 21:17:40 -0700 Subject: [PATCH 29/34] Add link to potential solution for WOULD-BE-NICE-TO-HAVE feature. --- tests/test_gil_scoped.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 5f4510e4a7..ed31ca564a 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -165,6 +165,8 @@ def _run_in_process(target, *args, **kwargs): t_delta = time.time() - t_start if process.exitcode == 66 and m.defined_THREAD_SANITIZER: # Issue #2754 # WOULD-BE-NICE-TO-HAVE: Check that the message below is actually in the output. + # Maybe this could work: + # https://gist.github.com/alexeygrigorev/01ce847f2e721b513b42ea4a6c96905e pytest.skip( "ThreadSanitizer: starting new threads after multi-threaded fork is not supported." ) From 19513d46dffed582bb7cfb258385e4f0369a42df Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 29 Oct 2022 21:36:54 -0700 Subject: [PATCH 30/34] Add `SKIP_IF_DEADLOCK = True` option, to not pollute the CI results with expected `DEADLOCK` failures while we figure out what to do about them. --- tests/test_gil_scoped.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index ed31ca564a..bc940a22eb 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -143,6 +143,7 @@ def _intentional_deadlock(): ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK = ALL_BASIC_TESTS + (_intentional_deadlock,) +SKIP_IF_DEADLOCK = True # See PR #4216 def _run_in_process(target, *args, **kwargs): @@ -175,9 +176,10 @@ def _run_in_process(target, *args, **kwargs): return 0 elif process.exitcode is None: assert t_delta > 0.9 * timeout - raise RuntimeError( - "DEADLOCK, most likely, exactly what this test is meant to detect." - ) + msg = "DEADLOCK, most likely, exactly what this test is meant to detect." + if SKIP_IF_DEADLOCK: + pytest.skip(msg) + raise RuntimeError(msg) return process.exitcode finally: if process.is_alive(): From 5ce396e0dc7102118ab813e85906e9e822a55a18 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 30 Oct 2022 00:28:38 -0700 Subject: [PATCH 31/34] Add COPY-PASTE-THIS: gdb ... command (to be used for debugging the detected deadlock) --- tests/test_gil_scoped.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index bc940a22eb..3171d95b74 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -1,4 +1,5 @@ import multiprocessing +import sys import threading import time @@ -159,7 +160,13 @@ def _run_in_process(target, *args, **kwargs): t_start = time.time() process.start() if timeout >= 100: # For debugging. - print("\nprocess.pid STARTED", process.pid, flush=True) + print( + "\nprocess.pid STARTED", process.pid, (sys.argv, target, args, kwargs) + ) + print( + "COPY-PASTE-THIS: gdb {} -p {}".format(sys.argv[0], process.pid), + flush=True, + ) process.join(timeout=timeout) if timeout >= 100: print("\nprocess.pid JOINED", process.pid, flush=True) From 8c9bce0fd58a282c045dd25b3ee34cff18b15914 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 30 Oct 2022 07:33:59 +0000 Subject: [PATCH 32/34] style: pre-commit fixes --- tests/test_gil_scoped.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 3171d95b74..246addf106 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -164,7 +164,7 @@ def _run_in_process(target, *args, **kwargs): "\nprocess.pid STARTED", process.pid, (sys.argv, target, args, kwargs) ) print( - "COPY-PASTE-THIS: gdb {} -p {}".format(sys.argv[0], process.pid), + f"COPY-PASTE-THIS: gdb {sys.argv[0]} -p {process.pid}", flush=True, ) process.join(timeout=timeout) From 065ac508bcbda140078bdeb84aa8b1fafc089559 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 30 Oct 2022 00:42:49 -0700 Subject: [PATCH 33/34] Do better than automatic pre-commit fixes. --- tests/test_gil_scoped.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 246addf106..e890a7b0c8 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -163,10 +163,7 @@ def _run_in_process(target, *args, **kwargs): print( "\nprocess.pid STARTED", process.pid, (sys.argv, target, args, kwargs) ) - print( - f"COPY-PASTE-THIS: gdb {sys.argv[0]} -p {process.pid}", - flush=True, - ) + print(f"COPY-PASTE-THIS: gdb {sys.argv[0]} -p {process.pid}", flush=True) process.join(timeout=timeout) if timeout >= 100: print("\nprocess.pid JOINED", process.pid, flush=True) From 6984cabc7c2f89c279a1da23a2b293a8b1046d0a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 30 Oct 2022 01:51:58 -0700 Subject: [PATCH 34/34] Add `PYBIND11_SIMPLE_GIL_MANAGEMENT` to `pytest_report_header()` (so that we can easily know when harvesting deadlock information from the CI logs). --- tests/conftest.py | 1 + tests/pybind11_tests.cpp | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 02ce263afc..f5ddb9f129 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -210,4 +210,5 @@ def pytest_report_header(config): f" {pybind11_tests.compiler_info}" f" {pybind11_tests.cpp_std}" f" {pybind11_tests.PYBIND11_INTERNALS_ID}" + f" PYBIND11_SIMPLE_GIL_MANAGEMENT={pybind11_tests.PYBIND11_SIMPLE_GIL_MANAGEMENT}" ) diff --git a/tests/pybind11_tests.cpp b/tests/pybind11_tests.cpp index aa3095594b..6240346487 100644 --- a/tests/pybind11_tests.cpp +++ b/tests/pybind11_tests.cpp @@ -89,6 +89,12 @@ PYBIND11_MODULE(pybind11_tests, m) { #endif m.attr("cpp_std") = cpp_std(); m.attr("PYBIND11_INTERNALS_ID") = PYBIND11_INTERNALS_ID; + m.attr("PYBIND11_SIMPLE_GIL_MANAGEMENT") = +#if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) + true; +#else + false; +#endif bind_ConstructorStats(m);