From 7e8a4cfe451e13e1f72249d94c3f6f36cd252505 Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Thu, 12 Nov 2020 18:09:33 -0800 Subject: [PATCH 1/9] Avoid thread termination in scoped_released Do not call `PyEval_RestoreThread()` from `~gil_scoped_release()` if python runtime is finalizing, as it will result in thread termination in Python runtime newer than 3.6, as documented in https://docs.python.org/3/c-api/init.html#c.PyEval_RestoreThread Similarly do not call `PyThreadState_DeleteCurrent` from `~gil_scoped_acquire()` if runtime is finalizing. Discovered while debugging PyTorch crash using Python-3.9 described in https://github.com/pytorch/pytorch/issues/47776 --- include/pybind11/pybind11.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index e2ddda020b..c2f83834ed 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2121,7 +2121,12 @@ class gil_scoped_acquire { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); #endif PyThreadState_Clear(tstate); +#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION > 6) && !defined(Py_LIMITED_API) + if (!_Py_IsFinalizing()) + PyThreadState_DeleteCurrent(); +#else PyThreadState_DeleteCurrent(); +#endif PYBIND11_TLS_DELETE_VALUE(detail::get_internals().tstate); release = false; } @@ -2153,7 +2158,14 @@ class gil_scoped_release { ~gil_scoped_release() { if (!tstate) return; +#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION > 6) && !defined(Py_LIMITED_API) + // PyEval_RestoreThread() should not be called if runtime is finilizing + // See https://docs.python.org/3/c-api/init.html#c.PyEval_RestoreThread + if (!_Py_IsFinalizing()) + PyEval_RestoreThread(tstate); +#else PyEval_RestoreThread(tstate); +#endif if (disassoc) { auto key = detail::get_internals().tstate; PYBIND11_TLS_REPLACE_VALUE(key, tstate); From 9e8bd73d434f374762218eace41b39ca2afc3fdb Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Fri, 13 Nov 2020 12:59:32 -0800 Subject: [PATCH 2/9] Simplify _Py_IsFinalizing() availability check --- include/pybind11/pybind11.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c2f83834ed..32a06587d1 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2121,7 +2121,7 @@ class gil_scoped_acquire { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); #endif PyThreadState_Clear(tstate); -#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION > 6) && !defined(Py_LIMITED_API) +#if PY_VERSION_HEX >= 0x03070000 if (!_Py_IsFinalizing()) PyThreadState_DeleteCurrent(); #else @@ -2158,7 +2158,7 @@ class gil_scoped_release { ~gil_scoped_release() { if (!tstate) return; -#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION > 6) && !defined(Py_LIMITED_API) +#if PY_VERSION_HEX >= 0x03070000 // PyEval_RestoreThread() should not be called if runtime is finilizing // See https://docs.python.org/3/c-api/init.html#c.PyEval_RestoreThread if (!_Py_IsFinalizing()) From c82145de8754d0bc1d16103444e9e815e5dc0e58 Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Mon, 16 Nov 2020 18:38:21 -0800 Subject: [PATCH 3/9] Fix typo --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 32a06587d1..0bd832bd26 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2159,7 +2159,7 @@ class gil_scoped_release { if (!tstate) return; #if PY_VERSION_HEX >= 0x03070000 - // PyEval_RestoreThread() should not be called if runtime is finilizing + // PyEval_RestoreThread() should not be called if runtime is finalizing // See https://docs.python.org/3/c-api/init.html#c.PyEval_RestoreThread if (!_Py_IsFinalizing()) PyEval_RestoreThread(tstate); From 7b707874c64f530a19c5a227492a17f91498e487 Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Mon, 16 Nov 2020 19:05:17 -0800 Subject: [PATCH 4/9] Add version agnostic `detail::finalization_guard()` --- include/pybind11/detail/internals.h | 13 +++++++++++++ include/pybind11/pybind11.h | 15 +++------------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index a455715bfd..ed59a17f78 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -328,6 +328,19 @@ const char *c_str(Args &&...args) { return strings.front().c_str(); } +// Thread state manipulation C API should not be called while Python runtime is finalizing. +// For more detail see https://docs.python.org/3.7/c-api/init.html#c.PyEval_RestoreThread +// `finalization_guard()` provides a version agnostic way to check if runtime is finalizing. +inline bool finalization_guard() { +#if PY_MAJOR_VERSION < 3 + return false; +#elif PY_VERSION_HEX >= 0x03070000 + return _Py_IsFinalizing(); +#else + return _Py_Finalizing; +#endif +} + PYBIND11_NAMESPACE_END(detail) /// Returns a named pointer that is shared among all extension modules (using the same diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 0bd832bd26..36618c68c5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2121,12 +2121,8 @@ class gil_scoped_acquire { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); #endif PyThreadState_Clear(tstate); -#if PY_VERSION_HEX >= 0x03070000 - if (!_Py_IsFinalizing()) + if (!detail::finalization_guard()) PyThreadState_DeleteCurrent(); -#else - PyThreadState_DeleteCurrent(); -#endif PYBIND11_TLS_DELETE_VALUE(detail::get_internals().tstate); release = false; } @@ -2158,14 +2154,9 @@ class gil_scoped_release { ~gil_scoped_release() { if (!tstate) return; -#if PY_VERSION_HEX >= 0x03070000 - // PyEval_RestoreThread() should not be called if runtime is finalizing - // See https://docs.python.org/3/c-api/init.html#c.PyEval_RestoreThread - if (!_Py_IsFinalizing()) + // `PyEval_RestoreThread()` should not be called if runtime is finalizing + if (!detail::finalization_guard()) PyEval_RestoreThread(tstate); -#else - PyEval_RestoreThread(tstate); -#endif if (disassoc) { auto key = detail::get_internals().tstate; PYBIND11_TLS_REPLACE_VALUE(key, tstate); From 461e81af2f2a2c5218f4ae24cecbc6e0a1a844de Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Tue, 17 Nov 2020 07:21:20 -0800 Subject: [PATCH 5/9] Move `finalization_guard` to detail/common.h And rename it to `is_finalizing` --- include/pybind11/detail/common.h | 13 +++++++++++++ include/pybind11/detail/internals.h | 13 ------------- include/pybind11/pybind11.h | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 56385f5fa6..18ef773a37 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -695,6 +695,19 @@ using expand_side_effects = bool[]; #define PYBIND11_EXPAND_SIDE_EFFECTS(PATTERN) (void)pybind11::detail::expand_side_effects{ ((PATTERN), void(), false)..., false } #endif +// Thread state manipulation C API should not be called while Python runtime is finalizing. +// For more detail see https://docs.python.org/3.7/c-api/init.html#c.PyEval_RestoreThread +// `is_finalizing()` provides a version agnostic way to check if runtime is finalizing. +inline bool is_finalizing() { +#if PY_MAJOR_VERSION < 3 + return false; +#elif PY_VERSION_HEX >= 0x03070000 + return _Py_IsFinalizing(); +#else + return _Py_Finalizing; +#endif +} + PYBIND11_NAMESPACE_END(detail) /// C++ bindings of builtin Python exceptions diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index ed59a17f78..a455715bfd 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -328,19 +328,6 @@ const char *c_str(Args &&...args) { return strings.front().c_str(); } -// Thread state manipulation C API should not be called while Python runtime is finalizing. -// For more detail see https://docs.python.org/3.7/c-api/init.html#c.PyEval_RestoreThread -// `finalization_guard()` provides a version agnostic way to check if runtime is finalizing. -inline bool finalization_guard() { -#if PY_MAJOR_VERSION < 3 - return false; -#elif PY_VERSION_HEX >= 0x03070000 - return _Py_IsFinalizing(); -#else - return _Py_Finalizing; -#endif -} - PYBIND11_NAMESPACE_END(detail) /// Returns a named pointer that is shared among all extension modules (using the same diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 36618c68c5..00caeff7a3 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2121,7 +2121,7 @@ class gil_scoped_acquire { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); #endif PyThreadState_Clear(tstate); - if (!detail::finalization_guard()) + if (!detail::is_finalizing()) PyThreadState_DeleteCurrent(); PYBIND11_TLS_DELETE_VALUE(detail::get_internals().tstate); release = false; @@ -2155,7 +2155,7 @@ class gil_scoped_release { if (!tstate) return; // `PyEval_RestoreThread()` should not be called if runtime is finalizing - if (!detail::finalization_guard()) + if (!detail::is_finalizing()) PyEval_RestoreThread(tstate); if (disassoc) { auto key = detail::get_internals().tstate; From f5804e391dfffeab723c30c293671ae6f05bc5b8 Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Tue, 17 Nov 2020 09:36:22 -0800 Subject: [PATCH 6/9] Move `is_finalizing()` back to pybind11.h --- include/pybind11/detail/common.h | 13 ------------- include/pybind11/pybind11.h | 13 +++++++++++++ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 18ef773a37..56385f5fa6 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -695,19 +695,6 @@ using expand_side_effects = bool[]; #define PYBIND11_EXPAND_SIDE_EFFECTS(PATTERN) (void)pybind11::detail::expand_side_effects{ ((PATTERN), void(), false)..., false } #endif -// Thread state manipulation C API should not be called while Python runtime is finalizing. -// For more detail see https://docs.python.org/3.7/c-api/init.html#c.PyEval_RestoreThread -// `is_finalizing()` provides a version agnostic way to check if runtime is finalizing. -inline bool is_finalizing() { -#if PY_MAJOR_VERSION < 3 - return false; -#elif PY_VERSION_HEX >= 0x03070000 - return _Py_IsFinalizing(); -#else - return _Py_Finalizing; -#endif -} - PYBIND11_NAMESPACE_END(detail) /// C++ bindings of builtin Python exceptions diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 00caeff7a3..b83d9ba4fb 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1168,6 +1168,19 @@ inline void add_class_method(object& cls, const char *name_, const cpp_function } } +// Thread state manipulation C API should not be called while Python runtime is finalizing. +// For more detail see https://docs.python.org/3.7/c-api/init.html#c.PyEval_RestoreThread +// `is_finalizing()` provides a version agnostic way to check if runtime is finalizing. +inline bool is_finalizing() { +#if PY_MAJOR_VERSION < 3 + return false; +#elif PY_VERSION_HEX >= 0x03070000 + return _Py_IsFinalizing(); +#else + return _Py_Finalizing; +#endif +} + PYBIND11_NAMESPACE_END(detail) /// Given a pointer to a member function, cast it to its `Derived` version. From e5fe8d3c65c67687bae8aaef2c7b01d2c4219046 Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Tue, 17 Nov 2020 16:32:36 -0800 Subject: [PATCH 7/9] Simplify `is_finalizing()` check One should follow documentation rather than make any assumptions --- include/pybind11/pybind11.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b83d9ba4fb..2d61f2cadb 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1172,12 +1172,10 @@ inline void add_class_method(object& cls, const char *name_, const cpp_function // For more detail see https://docs.python.org/3.7/c-api/init.html#c.PyEval_RestoreThread // `is_finalizing()` provides a version agnostic way to check if runtime is finalizing. inline bool is_finalizing() { -#if PY_MAJOR_VERSION < 3 - return false; -#elif PY_VERSION_HEX >= 0x03070000 +#if PY_VERSION_HEX >= 0x03070000 return _Py_IsFinalizing(); #else - return _Py_Finalizing; + return false; #endif } From bb8cee6c7ec3b43da9adc20d435dadcccbfe2395 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Tue, 15 Dec 2020 23:50:33 -0500 Subject: [PATCH 8/9] feat: disarm --- include/pybind11/pybind11.h | 44 ++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 2d61f2cadb..ada7128c8f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1168,17 +1168,6 @@ inline void add_class_method(object& cls, const char *name_, const cpp_function } } -// Thread state manipulation C API should not be called while Python runtime is finalizing. -// For more detail see https://docs.python.org/3.7/c-api/init.html#c.PyEval_RestoreThread -// `is_finalizing()` provides a version agnostic way to check if runtime is finalizing. -inline bool is_finalizing() { -#if PY_VERSION_HEX >= 0x03070000 - return _Py_IsFinalizing(); -#else - return false; -#endif -} - PYBIND11_NAMESPACE_END(detail) /// Given a pointer to a member function, cast it to its `Derived` version. @@ -2132,13 +2121,21 @@ class gil_scoped_acquire { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); #endif PyThreadState_Clear(tstate); - if (!detail::is_finalizing()) + if (active) PyThreadState_DeleteCurrent(); PYBIND11_TLS_DELETE_VALUE(detail::get_internals().tstate); release = false; } } + /// This method will disable the PyThreadState_DeleteCurrent call. This + /// should be called if the interpreter is shutting down, as the thread + /// deletion is not allowed during shutdown. + /// (_Py_IsFinalizing() on Python 3.7+) + PYBIND11_NOINLINE void disarm() { + active = false; + } + PYBIND11_NOINLINE ~gil_scoped_acquire() { dec_ref(); if (release) @@ -2147,6 +2144,7 @@ class gil_scoped_acquire { private: PyThreadState *tstate = nullptr; bool release = true; + bool active = true; }; class gil_scoped_release { @@ -2162,11 +2160,20 @@ class gil_scoped_release { PYBIND11_TLS_DELETE_VALUE(key); } } + // + /// This method will disable the PyThreadState_DeleteCurrent call. This + /// should be called if the interpreter is shutting down, as the thread + /// deletion is not allowed during shutdown. + /// (_Py_IsFinalizing() on Python 3.7+) + PYBIND11_NOINLINE void disarm() { + active = false; + } + ~gil_scoped_release() { if (!tstate) return; // `PyEval_RestoreThread()` should not be called if runtime is finalizing - if (!detail::is_finalizing()) + if (active) PyEval_RestoreThread(tstate); if (disassoc) { auto key = detail::get_internals().tstate; @@ -2176,6 +2183,7 @@ class gil_scoped_release { private: PyThreadState *tstate; bool disassoc; + bool active = true; }; #elif defined(PYPY_VERSION) class gil_scoped_acquire { @@ -2183,6 +2191,7 @@ class gil_scoped_acquire { public: gil_scoped_acquire() { state = PyGILState_Ensure(); } ~gil_scoped_acquire() { PyGILState_Release(state); } + void disarm() {} }; class gil_scoped_release { @@ -2190,10 +2199,15 @@ class gil_scoped_release { public: gil_scoped_release() { state = PyEval_SaveThread(); } ~gil_scoped_release() { PyEval_RestoreThread(state); } + void disarm() {} }; #else -class gil_scoped_acquire { }; -class gil_scoped_release { }; +class gil_scoped_acquire { + void disarm() {} +}; +class gil_scoped_release { + void disarm() {} +}; #endif error_already_set::~error_already_set() { From 93639b8fb0e3a0da085378d38b044a1c43c7c4a3 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Fri, 18 Dec 2020 15:36:46 -0500 Subject: [PATCH 9/9] docs: fix comment --- include/pybind11/pybind11.h | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index ada7128c8f..6d8f7ab269 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2128,10 +2128,11 @@ class gil_scoped_acquire { } } - /// This method will disable the PyThreadState_DeleteCurrent call. This - /// should be called if the interpreter is shutting down, as the thread - /// deletion is not allowed during shutdown. - /// (_Py_IsFinalizing() on Python 3.7+) + /// This method will disable the PyThreadState_DeleteCurrent call and the + /// GIL won't be acquired. This method should be used if the interpreter + /// could be shutting down when this is called, as thread deletion is not + /// allowed during shutdown. Check _Py_IsFinalizing() on Python 3.7+, and + /// protect subsequent code. PYBIND11_NOINLINE void disarm() { active = false; } @@ -2160,11 +2161,12 @@ class gil_scoped_release { PYBIND11_TLS_DELETE_VALUE(key); } } - // - /// This method will disable the PyThreadState_DeleteCurrent call. This - /// should be called if the interpreter is shutting down, as the thread - /// deletion is not allowed during shutdown. - /// (_Py_IsFinalizing() on Python 3.7+) + + /// This method will disable the PyThreadState_DeleteCurrent call and the + /// GIL won't be acquired. This method should be used if the interpreter + /// could be shutting down when this is called, as thread deletion is not + /// allowed during shutdown. Check _Py_IsFinalizing() on Python 3.7+, and + /// protect subsequent code. PYBIND11_NOINLINE void disarm() { active = false; }