From 673907d95d087c5224ceb30408b4bfe119ab5520 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 26 Jan 2025 16:10:16 +0000 Subject: [PATCH 01/13] hang instead of pthread_exit during interpreter shutdown see https://github.com/python/cpython/issues/87135 and https://github.com/rust-lang/rust/issues/135929 --- pyo3-ffi/Cargo.toml | 1 + pyo3-ffi/build.rs | 18 +++++++++++++++ pyo3-ffi/src/acquire_gil.cpp | 45 ++++++++++++++++++++++++++++++++++++ pyo3-ffi/src/pystate.rs | 8 ++++++- 4 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 pyo3-ffi/src/acquire_gil.cpp diff --git a/pyo3-ffi/Cargo.toml b/pyo3-ffi/Cargo.toml index 656146e12aa..8d6fdbdb2d1 100644 --- a/pyo3-ffi/Cargo.toml +++ b/pyo3-ffi/Cargo.toml @@ -43,6 +43,7 @@ paste = "1" [build-dependencies] pyo3-build-config = { path = "../pyo3-build-config", version = "=0.23.5", features = ["resolve-config"] } +cc = "1.2" [lints] workspace = true diff --git a/pyo3-ffi/build.rs b/pyo3-ffi/build.rs index 096614c7961..078a05c08cd 100644 --- a/pyo3-ffi/build.rs +++ b/pyo3-ffi/build.rs @@ -182,6 +182,22 @@ fn emit_link_config(interpreter_config: &InterpreterConfig) -> Result<()> { Ok(()) } +fn do_cc(interpreter_config: &InterpreterConfig) -> Result<()> { + let implementation_def = match interpreter_config.implementation { + PythonImplementation::CPython => "PYTHON_IS_CPYTHON", + PythonImplementation::PyPy => "PYTHON_IS_PYPY", + PythonImplementation::GraalPy => "PYTHON_IS_GRAALPY", + }; + println!("cargo:rerun-if-changed=src/acquire_gil.cpp"); + cc::Build::new() + .cpp(true) + .file("src/acquire_gil.cpp") + .define(implementation_def, None) + .cpp_link_stdlib("stdc++") + .compile("acquire_gil"); + Ok(()) +} + /// Prepares the PyO3 crate for compilation. /// /// This loads the config from pyo3-build-config and then makes some additional checks to improve UX @@ -218,6 +234,8 @@ fn configure_pyo3() -> Result<()> { // Emit cfgs like `invalid_from_utf8_lint` print_feature_cfgs(); + do_cc(&interpreter_config)?; + Ok(()) } diff --git a/pyo3-ffi/src/acquire_gil.cpp b/pyo3-ffi/src/acquire_gil.cpp new file mode 100644 index 00000000000..5a8200e8b21 --- /dev/null +++ b/pyo3-ffi/src/acquire_gil.cpp @@ -0,0 +1,45 @@ +#if defined(_WIN32) +#include +#include +#else +#include +#endif + +#if defined(PYTHON_IS_PYPY) +#define gil_func_name PyPyGILState_Ensure +#define wrapped_func_name PyPyGILState_Ensure_Safe +#else +#define gil_func_name PyGILState_Ensure +#define wrapped_func_name PyGILState_Ensure_Safe +#endif + +extern "C" { + int wrapped_func_name(void); + int gil_func_name(void); +}; + +#if !defined(_WIN32) +// mark the wrapped function as visibility("hidden") to avoid causing namespace pollution +__attribute__((visibility("hidden"))) +#endif +int wrapped_func_name(void) { + // Do the equivalent of https://github.com/python/cpython/issues/87135 (included + // in Python 3.14) to avoid pthread_exit unwinding the current thread, which tends + // to cause undefined behavior in Rust. + // + // Unfortunately, I don't know of a way to do a catch(...) from Rust. + try { + return gil_func_name(); + } catch(...) { + while(1) { +#if defined(_WIN32) + SleepEx(INFINITE, TRUE); +#elif defined(__wasi__) + sleep(9999999); // WASI doesn't have pause() ?! +#else + pause(); +#endif + } + } +} + diff --git a/pyo3-ffi/src/pystate.rs b/pyo3-ffi/src/pystate.rs index 0c062160ccc..0ad1a20b72e 100644 --- a/pyo3-ffi/src/pystate.rs +++ b/pyo3-ffi/src/pystate.rs @@ -81,7 +81,13 @@ pub enum PyGILState_STATE { } extern "C" { - #[cfg_attr(PyPy, link_name = "PyPyGILState_Ensure")] + // The PyGILState_Ensure function will call pthread_exit during interpreter shutdown, + // which causes undefined behavior. Redirect to the "safe" version that hangs instead, + // as Python 3.14 does. + // + // See https://github.com/rust-lang/rust/issues/135929 + #[cfg_attr(PyPy, link_name = "PyPyGILState_Ensure_Safe")] + #[cfg_attr(not(PyPy), link_name = "PyGILState_Ensure_Safe")] pub fn PyGILState_Ensure() -> PyGILState_STATE; #[cfg_attr(PyPy, link_name = "PyPyGILState_Release")] pub fn PyGILState_Release(arg1: PyGILState_STATE); From b1418bd057ed52744de1d2b02fbfb4c55dd6adba Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 26 Jan 2025 16:15:32 +0000 Subject: [PATCH 02/13] relnotes --- newsfragments/4874.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/4874.changed.md diff --git a/newsfragments/4874.changed.md b/newsfragments/4874.changed.md new file mode 100644 index 00000000000..fd483f43de5 --- /dev/null +++ b/newsfragments/4874.changed.md @@ -0,0 +1 @@ + * PyO3 threads now hang instead of `pthread_exit` trying to acquire the GIL when the interpreter is shutting down. This mimics the [Python 3.14](https://github.com/python/cpython/issues/87135) behavior and avoids undefined behavior and crashes. From 09ee69675bcf2646b0537d39e7b4a961a04a3bc3 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 26 Jan 2025 16:18:02 +0000 Subject: [PATCH 03/13] fix warnings --- pyo3-ffi/build.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pyo3-ffi/build.rs b/pyo3-ffi/build.rs index 078a05c08cd..7f2a1d832fd 100644 --- a/pyo3-ffi/build.rs +++ b/pyo3-ffi/build.rs @@ -182,7 +182,7 @@ fn emit_link_config(interpreter_config: &InterpreterConfig) -> Result<()> { Ok(()) } -fn do_cc(interpreter_config: &InterpreterConfig) -> Result<()> { +fn do_cc(interpreter_config: &InterpreterConfig) { let implementation_def = match interpreter_config.implementation { PythonImplementation::CPython => "PYTHON_IS_CPYTHON", PythonImplementation::PyPy => "PYTHON_IS_PYPY", @@ -195,7 +195,6 @@ fn do_cc(interpreter_config: &InterpreterConfig) -> Result<()> { .define(implementation_def, None) .cpp_link_stdlib("stdc++") .compile("acquire_gil"); - Ok(()) } /// Prepares the PyO3 crate for compilation. @@ -234,7 +233,7 @@ fn configure_pyo3() -> Result<()> { // Emit cfgs like `invalid_from_utf8_lint` print_feature_cfgs(); - do_cc(&interpreter_config)?; + do_cc(&interpreter_config); Ok(()) } From 93457f7331b947a80d869e737f43529f95a2fc99 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 27 Jan 2025 12:53:33 +0000 Subject: [PATCH 04/13] version using pthread_cleanup_push --- pyo3-ffi/build.rs | 4 +- .../src/{acquire_gil.cpp => acquire_gil.c} | 40 +++++++++++++------ 2 files changed, 28 insertions(+), 16 deletions(-) rename pyo3-ffi/src/{acquire_gil.cpp => acquire_gil.c} (56%) diff --git a/pyo3-ffi/build.rs b/pyo3-ffi/build.rs index 7f2a1d832fd..19fcae11242 100644 --- a/pyo3-ffi/build.rs +++ b/pyo3-ffi/build.rs @@ -190,10 +190,8 @@ fn do_cc(interpreter_config: &InterpreterConfig) { }; println!("cargo:rerun-if-changed=src/acquire_gil.cpp"); cc::Build::new() - .cpp(true) - .file("src/acquire_gil.cpp") + .file("src/acquire_gil.c") .define(implementation_def, None) - .cpp_link_stdlib("stdc++") .compile("acquire_gil"); } diff --git a/pyo3-ffi/src/acquire_gil.cpp b/pyo3-ffi/src/acquire_gil.c similarity index 56% rename from pyo3-ffi/src/acquire_gil.cpp rename to pyo3-ffi/src/acquire_gil.c index 5a8200e8b21..52732694828 100644 --- a/pyo3-ffi/src/acquire_gil.cpp +++ b/pyo3-ffi/src/acquire_gil.c @@ -2,6 +2,7 @@ #include #include #else +#include #include #endif @@ -13,33 +14,46 @@ #define wrapped_func_name PyGILState_Ensure_Safe #endif -extern "C" { - int wrapped_func_name(void); - int gil_func_name(void); -}; +int wrapped_func_name(void); +int gil_func_name(void); -#if !defined(_WIN32) -// mark the wrapped function as visibility("hidden") to avoid causing namespace pollution -__attribute__((visibility("hidden"))) -#endif +#if defined(_WIN32) int wrapped_func_name(void) { // Do the equivalent of https://github.com/python/cpython/issues/87135 (included // in Python 3.14) to avoid pthread_exit unwinding the current thread, which tends // to cause undefined behavior in Rust. // // Unfortunately, I don't know of a way to do a catch(...) from Rust. - try { + __try { return gil_func_name(); - } catch(...) { + } __catch(void) { while(1) { -#if defined(_WIN32) SleepEx(INFINITE, TRUE); -#elif defined(__wasi__) + } + } +} +#else +static void hang_thread(void *ignore) { + (void)ignore; + while(1) { +#if defined(__wasi__) sleep(9999999); // WASI doesn't have pause() ?! #else pause(); #endif - } } } +int wrapped_func_name(void) { + // Do the equivalent of https://github.com/python/cpython/issues/87135 (included + // in Python 3.14) to avoid pthread_exit unwinding the current thread, which tends + // to cause undefined behavior in Rust. + // + // Unfortunately, I don't know of a way to do a catch(...) from Rust. + int ret; + pthread_cleanup_push(hang_thread, NULL); + ret = gil_func_name(); + pthread_cleanup_pop(0); + return ret; +} +#endif \ No newline at end of file From 36394c83f1994da91b3060343efd3c4923b322f1 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 27 Jan 2025 13:21:11 +0000 Subject: [PATCH 05/13] add tests --- pyo3-ffi/src/acquire_gil.c | 17 +++-------------- pytests/src/misc.rs | 20 ++++++++++++++++++++ pytests/tests/test_hammer_gil_in_thread.py | 20 ++++++++++++++++++++ 3 files changed, 43 insertions(+), 14 deletions(-) create mode 100644 pytests/tests/test_hammer_gil_in_thread.py diff --git a/pyo3-ffi/src/acquire_gil.c b/pyo3-ffi/src/acquire_gil.c index 52732694828..3becd0bc70b 100644 --- a/pyo3-ffi/src/acquire_gil.c +++ b/pyo3-ffi/src/acquire_gil.c @@ -1,6 +1,4 @@ #if defined(_WIN32) -#include -#include #else #include #include @@ -19,18 +17,9 @@ int gil_func_name(void); #if defined(_WIN32) int wrapped_func_name(void) { - // Do the equivalent of https://github.com/python/cpython/issues/87135 (included - // in Python 3.14) to avoid pthread_exit unwinding the current thread, which tends - // to cause undefined behavior in Rust. - // - // Unfortunately, I don't know of a way to do a catch(...) from Rust. - __try { - return gil_func_name(); - } __catch(void) { - while(1) { - SleepEx(INFINITE, TRUE); - } - } + // In MSVC, PyThread_exit_thread calls _endthreadex(0), which does not use SEH. This can + // cause Rust-level UB if there is pinned memory, but AFAICT there's not much we can do about it. + return gil_func_name(); } #else static void hang_thread(void *ignore) { diff --git a/pytests/src/misc.rs b/pytests/src/misc.rs index e44d1aa0ecf..2fa43bea971 100644 --- a/pytests/src/misc.rs +++ b/pytests/src/misc.rs @@ -9,6 +9,25 @@ fn issue_219() { Python::with_gil(|_| {}); } +#[pyclass] +struct LockHolder { + #[allow(unused)] + sender: std::sync::mpsc::Sender<()>, +} + +// This will hammer the GIL once the retyrbed LockHolder is dropped. +#[pyfunction] +fn hammer_gil_in_thread() -> PyResult { + let (sender, receiver) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + receiver.recv().ok(); + loop { + Python::with_gil(|_py| ()); + } + }); + Ok(LockHolder { sender }) +} + #[pyfunction] fn get_type_fully_qualified_name<'py>(obj: &Bound<'py, PyAny>) -> PyResult> { obj.get_type().fully_qualified_name() @@ -35,6 +54,7 @@ fn get_item_and_run_callback(dict: Bound<'_, PyDict>, callback: Bound<'_, PyAny> #[pymodule(gil_used = false)] pub fn misc(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(issue_219, m)?)?; + m.add_function(wrap_pyfunction!(hammer_gil_in_thread, m)?)?; m.add_function(wrap_pyfunction!(get_type_fully_qualified_name, m)?)?; m.add_function(wrap_pyfunction!(accepts_bool, m)?)?; m.add_function(wrap_pyfunction!(get_item_and_run_callback, m)?)?; diff --git a/pytests/tests/test_hammer_gil_in_thread.py b/pytests/tests/test_hammer_gil_in_thread.py new file mode 100644 index 00000000000..cee1bb6b828 --- /dev/null +++ b/pytests/tests/test_hammer_gil_in_thread.py @@ -0,0 +1,20 @@ +from pyo3_pytests import misc + + +def make_loop(): + # create a reference loop that will only be destroyed when the GC is called at the end + # of execution + start = [] + cur = [start] + for _ in range(1000 * 1000 * 10): + cur = [cur] + start.append(cur) + return start + + +# set a bomb that will explode when modules are cleaned up +loopy = [make_loop()] + + +def test_hammer_gil(): + loopy.append(misc.hammer_gil_in_thread()) From 5ba9a7b7eddf879a23a0ba29ac513cd17a2e3a86 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 27 Jan 2025 21:48:49 +0000 Subject: [PATCH 06/13] new attempt --- pyo3-ffi/Cargo.toml | 2 +- pyo3-ffi/build.rs | 15 ----------- pyo3-ffi/src/acquire_gil.c | 48 ----------------------------------- pyo3-ffi/src/pystate.rs | 51 ++++++++++++++++++++++++++++++++------ 4 files changed, 44 insertions(+), 72 deletions(-) delete mode 100644 pyo3-ffi/src/acquire_gil.c diff --git a/pyo3-ffi/Cargo.toml b/pyo3-ffi/Cargo.toml index 8d6fdbdb2d1..ff36a98824d 100644 --- a/pyo3-ffi/Cargo.toml +++ b/pyo3-ffi/Cargo.toml @@ -14,6 +14,7 @@ rust-version = "1.63" [dependencies] libc = "0.2.62" +rustversion = "1" [features] @@ -43,7 +44,6 @@ paste = "1" [build-dependencies] pyo3-build-config = { path = "../pyo3-build-config", version = "=0.23.5", features = ["resolve-config"] } -cc = "1.2" [lints] workspace = true diff --git a/pyo3-ffi/build.rs b/pyo3-ffi/build.rs index 19fcae11242..096614c7961 100644 --- a/pyo3-ffi/build.rs +++ b/pyo3-ffi/build.rs @@ -182,19 +182,6 @@ fn emit_link_config(interpreter_config: &InterpreterConfig) -> Result<()> { Ok(()) } -fn do_cc(interpreter_config: &InterpreterConfig) { - let implementation_def = match interpreter_config.implementation { - PythonImplementation::CPython => "PYTHON_IS_CPYTHON", - PythonImplementation::PyPy => "PYTHON_IS_PYPY", - PythonImplementation::GraalPy => "PYTHON_IS_GRAALPY", - }; - println!("cargo:rerun-if-changed=src/acquire_gil.cpp"); - cc::Build::new() - .file("src/acquire_gil.c") - .define(implementation_def, None) - .compile("acquire_gil"); -} - /// Prepares the PyO3 crate for compilation. /// /// This loads the config from pyo3-build-config and then makes some additional checks to improve UX @@ -231,8 +218,6 @@ fn configure_pyo3() -> Result<()> { // Emit cfgs like `invalid_from_utf8_lint` print_feature_cfgs(); - do_cc(&interpreter_config); - Ok(()) } diff --git a/pyo3-ffi/src/acquire_gil.c b/pyo3-ffi/src/acquire_gil.c deleted file mode 100644 index 3becd0bc70b..00000000000 --- a/pyo3-ffi/src/acquire_gil.c +++ /dev/null @@ -1,48 +0,0 @@ -#if defined(_WIN32) -#else -#include -#include -#endif - -#if defined(PYTHON_IS_PYPY) -#define gil_func_name PyPyGILState_Ensure -#define wrapped_func_name PyPyGILState_Ensure_Safe -#else -#define gil_func_name PyGILState_Ensure -#define wrapped_func_name PyGILState_Ensure_Safe -#endif - -int wrapped_func_name(void); -int gil_func_name(void); - -#if defined(_WIN32) -int wrapped_func_name(void) { - // In MSVC, PyThread_exit_thread calls _endthreadex(0), which does not use SEH. This can - // cause Rust-level UB if there is pinned memory, but AFAICT there's not much we can do about it. - return gil_func_name(); -} -#else -static void hang_thread(void *ignore) { - (void)ignore; - while(1) { -#if defined(__wasi__) - sleep(9999999); // WASI doesn't have pause() ?! -#else - pause(); -#endif - } -} - -int wrapped_func_name(void) { - // Do the equivalent of https://github.com/python/cpython/issues/87135 (included - // in Python 3.14) to avoid pthread_exit unwinding the current thread, which tends - // to cause undefined behavior in Rust. - // - // Unfortunately, I don't know of a way to do a catch(...) from Rust. - int ret; - pthread_cleanup_push(hang_thread, NULL); - ret = gil_func_name(); - pthread_cleanup_pop(0); - return ret; -} -#endif \ No newline at end of file diff --git a/pyo3-ffi/src/pystate.rs b/pyo3-ffi/src/pystate.rs index 0ad1a20b72e..47c92bd14b6 100644 --- a/pyo3-ffi/src/pystate.rs +++ b/pyo3-ffi/src/pystate.rs @@ -80,15 +80,50 @@ pub enum PyGILState_STATE { PyGILState_UNLOCKED, } +struct HangThread; + +impl Drop for HangThread { + fn drop(&mut self) { + loop { + #[cfg(target_family = "unix")] + unsafe { + libc::pause(); + } + #[cfg(not(target_family = "unix"))] + std::thread::sleep(std::time::Duration::from_secs(9_999_999)); + } + } +} + +// The PyGILState_Ensure function will call pthread_exit during interpreter shutdown, +// which causes undefined behavior. Redirect to the "safe" version that hangs instead, +// as Python 3.14 does. +// +// See https://github.com/rust-lang/rust/issues/135929 + +// C-unwind only supported (and necessary) since 1.71 +mod raw { + #[rustversion::since(1.71)] + extern "C-unwind" { + #[cfg_attr(PyPy, link_name = "PyPyGILState_Ensure")] + pub fn PyGILState_Ensure() -> super::PyGILState_STATE; + } + + #[rustversion::before(1.71)] + extern "C" { + #[cfg_attr(PyPy, link_name = "PyPyGILState_Ensure")] + pub fn PyGILState_Ensure() -> super::PyGILState_STATE; + } +} + +pub unsafe extern "C" fn PyGILState_Ensure() -> PyGILState_STATE { + let guard = HangThread; + let ret: PyGILState_STATE = raw::PyGILState_Ensure(); + std::mem::forget(guard); + ret +} + extern "C" { - // The PyGILState_Ensure function will call pthread_exit during interpreter shutdown, - // which causes undefined behavior. Redirect to the "safe" version that hangs instead, - // as Python 3.14 does. - // - // See https://github.com/rust-lang/rust/issues/135929 - #[cfg_attr(PyPy, link_name = "PyPyGILState_Ensure_Safe")] - #[cfg_attr(not(PyPy), link_name = "PyGILState_Ensure_Safe")] - pub fn PyGILState_Ensure() -> PyGILState_STATE; #[cfg_attr(PyPy, link_name = "PyPyGILState_Release")] pub fn PyGILState_Release(arg1: PyGILState_STATE); #[cfg(not(PyPy))] From d37d1e10a11661e8fc409423b256779e9f5f0cca Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 27 Jan 2025 22:14:28 +0000 Subject: [PATCH 07/13] clippy --- pytests/src/misc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pytests/src/misc.rs b/pytests/src/misc.rs index 2fa43bea971..3b6c8cff949 100644 --- a/pytests/src/misc.rs +++ b/pytests/src/misc.rs @@ -17,7 +17,7 @@ struct LockHolder { // This will hammer the GIL once the retyrbed LockHolder is dropped. #[pyfunction] -fn hammer_gil_in_thread() -> PyResult { +fn hammer_gil_in_thread() -> LockHolder { let (sender, receiver) = std::sync::mpsc::channel(); std::thread::spawn(move || { receiver.recv().ok(); @@ -25,7 +25,7 @@ fn hammer_gil_in_thread() -> PyResult { Python::with_gil(|_py| ()); } }); - Ok(LockHolder { sender }) + LockHolder { sender } } #[pyfunction] From 4b5153abe044a664801dce71897f1207e9413672 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 27 Jan 2025 22:16:13 +0000 Subject: [PATCH 08/13] comment --- pytests/src/misc.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pytests/src/misc.rs b/pytests/src/misc.rs index 3b6c8cff949..7beafe78020 100644 --- a/pytests/src/misc.rs +++ b/pytests/src/misc.rs @@ -15,12 +15,14 @@ struct LockHolder { sender: std::sync::mpsc::Sender<()>, } -// This will hammer the GIL once the retyrbed LockHolder is dropped. +// This will hammer the GIL once the LockHolder is dropped. #[pyfunction] fn hammer_gil_in_thread() -> LockHolder { let (sender, receiver) = std::sync::mpsc::channel(); std::thread::spawn(move || { receiver.recv().ok(); + // now the interpreter has shut down, so hammer the GIL. In buggy + // versions of PyO3 this will cause a crash. loop { Python::with_gil(|_py| ()); } From 3fc09730396fbfb5022b52601e98d1fa632810d9 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 27 Jan 2025 22:20:39 +0000 Subject: [PATCH 09/13] msrv --- pytests/src/misc.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pytests/src/misc.rs b/pytests/src/misc.rs index 7beafe78020..b3ef5ee283e 100644 --- a/pytests/src/misc.rs +++ b/pytests/src/misc.rs @@ -12,7 +12,8 @@ fn issue_219() { #[pyclass] struct LockHolder { #[allow(unused)] - sender: std::sync::mpsc::Sender<()>, + // Mutex needed for the MSRV + sender: std::sync::Mutex>, } // This will hammer the GIL once the LockHolder is dropped. @@ -27,7 +28,9 @@ fn hammer_gil_in_thread() -> LockHolder { Python::with_gil(|_py| ()); } }); - LockHolder { sender } + LockHolder { + sender: std::sync::Mutex::new(sender), + } } #[pyfunction] From 41757bab97c164e2e3ab07adde365dafcfce3d70 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 28 Jan 2025 10:51:31 +0000 Subject: [PATCH 10/13] address review comments --- pyo3-build-config/src/lib.rs | 8 +++++++- pyo3-ffi/Cargo.toml | 1 - pyo3-ffi/src/pystate.rs | 8 ++++++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 9070f6d7401..a908fe88068 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -180,6 +180,10 @@ pub fn print_feature_cfgs() { println!("cargo:rustc-cfg=rustc_has_once_lock"); } + if rustc_minor_version >= 71 { + println!("cargo:rustc-cfg=rustc_has_extern_c_unwind"); + } + // invalid_from_utf8 lint was added in Rust 1.74 if rustc_minor_version >= 74 { println!("cargo:rustc-cfg=invalid_from_utf8_lint"); @@ -226,12 +230,14 @@ pub fn print_expected_cfgs() { println!("cargo:rustc-check-cfg=cfg(diagnostic_namespace)"); println!("cargo:rustc-check-cfg=cfg(c_str_lit)"); println!("cargo:rustc-check-cfg=cfg(rustc_has_once_lock)"); + println!("cargo:rustc-check-cfg=cfg(rustc_has_extern_c_unwind)"); println!("cargo:rustc-check-cfg=cfg(io_error_more)"); println!("cargo:rustc-check-cfg=cfg(fn_ptr_eq)"); // allow `Py_3_*` cfgs from the minimum supported version up to the // maximum minor version (+1 for development for the next) - for i in impl_::MINIMUM_SUPPORTED_VERSION.minor..=impl_::ABI3_MAX_MINOR + 1 { + // FIXME: support cfg(Py_3_14) as well due to PyGILState_Ensure + for i in impl_::MINIMUM_SUPPORTED_VERSION.minor..=std::cmp::max(14, impl_::ABI3_MAX_MINOR + 1) { println!("cargo:rustc-check-cfg=cfg(Py_3_{i})"); } } diff --git a/pyo3-ffi/Cargo.toml b/pyo3-ffi/Cargo.toml index ff36a98824d..656146e12aa 100644 --- a/pyo3-ffi/Cargo.toml +++ b/pyo3-ffi/Cargo.toml @@ -14,7 +14,6 @@ rust-version = "1.63" [dependencies] libc = "0.2.62" -rustversion = "1" [features] diff --git a/pyo3-ffi/src/pystate.rs b/pyo3-ffi/src/pystate.rs index 47c92bd14b6..28331947d75 100644 --- a/pyo3-ffi/src/pystate.rs +++ b/pyo3-ffi/src/pystate.rs @@ -103,19 +103,20 @@ impl Drop for HangThread { // C-unwind only supported (and necessary) since 1.71 mod raw { - #[rustversion::since(1.71)] + #[cfg(all(not(Py_3_14), rustc_has_extern_c_unwind))] extern "C-unwind" { #[cfg_attr(PyPy, link_name = "PyPyGILState_Ensure")] pub fn PyGILState_Ensure() -> super::PyGILState_STATE; } - #[rustversion::before(1.71)] + #[cfg(not(all(not(Py_3_14), rustc_has_extern_c_unwind)))] extern "C" { #[cfg_attr(PyPy, link_name = "PyPyGILState_Ensure")] pub fn PyGILState_Ensure() -> super::PyGILState_STATE; } } +#[cfg(not(Py_3_14))] pub unsafe extern "C" fn PyGILState_Ensure() -> PyGILState_STATE { let guard = HangThread; let ret: PyGILState_STATE = raw::PyGILState_Ensure(); @@ -123,6 +124,9 @@ pub unsafe extern "C" fn PyGILState_Ensure() -> PyGILState_STATE { ret } +#[cfg(Py_3_14)] +pub use self::raw::PyGILState_Ensure; + extern "C" { #[cfg_attr(PyPy, link_name = "PyPyGILState_Release")] pub fn PyGILState_Release(arg1: PyGILState_STATE); From b406cd967ee8c5e57b2f6717a4ad9b9f174eaf94 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 28 Jan 2025 10:57:35 +0000 Subject: [PATCH 11/13] update comment --- pyo3-ffi/src/pystate.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pyo3-ffi/src/pystate.rs b/pyo3-ffi/src/pystate.rs index 28331947d75..02f8a758754 100644 --- a/pyo3-ffi/src/pystate.rs +++ b/pyo3-ffi/src/pystate.rs @@ -101,7 +101,8 @@ impl Drop for HangThread { // // See https://github.com/rust-lang/rust/issues/135929 -// C-unwind only supported (and necessary) since 1.71 +// C-unwind only supported (and necessary) since 1.71. Python 3.14+ does not do +// pthread_exit from PyGILState_Ensure (https://github.com/python/cpython/issues/87135). mod raw { #[cfg(all(not(Py_3_14), rustc_has_extern_c_unwind))] extern "C-unwind" { From 5f38e0ebaf98b915a28247cdf61c8a9014eab0ec Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 28 Jan 2025 23:24:15 +0000 Subject: [PATCH 12/13] add comment --- pyo3-ffi/src/pystate.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pyo3-ffi/src/pystate.rs b/pyo3-ffi/src/pystate.rs index 02f8a758754..a6caf421ff6 100644 --- a/pyo3-ffi/src/pystate.rs +++ b/pyo3-ffi/src/pystate.rs @@ -120,6 +120,24 @@ mod raw { #[cfg(not(Py_3_14))] pub unsafe extern "C" fn PyGILState_Ensure() -> PyGILState_STATE { let guard = HangThread; + // If `PyGILState_Ensure` calls `pthread_exit`, which it does on Python < 3.14 + // when the interpreter is shutting down, this will cause a forced unwind. + // doing a forced unwind through a function with a Rust destructor is unspecified + // behavior. + // + // However, currently it runs the destructor, which will cause the thread to + // hang as it should. + // + // And if we don't catch the unwinding here, then one of our callers probably has a destructor, + // so it's unspecified behavior anyway, and on many configurations causes the process to abort. + // + // The alternative is for pyo3 to contain custom C or C++ code that catches the `pthread_exit`, + // but that's also annoying from a portability point of view. + // + // On Windows, `PyGILState_Ensure` calls `_endthreadex` instead, which AFAICT can't be caught + // and therefore will cause unsafety if there are pinned objects on the stack. AFAICT there's + // nothing we can do it other than waiting for Python 3.14 or not using Windows. At least, + // if there is nothing pinned on the stack, it won't cause the process to crash. let ret: PyGILState_STATE = raw::PyGILState_Ensure(); std::mem::forget(guard); ret From b86c1df0b96252a93a0521f0076afb72ce811187 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 9 Mar 2025 19:15:14 +0000 Subject: [PATCH 13/13] try to skip test on debug builds --- pytests/tests/test_hammer_gil_in_thread.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pytests/tests/test_hammer_gil_in_thread.py b/pytests/tests/test_hammer_gil_in_thread.py index cee1bb6b828..9eed640f65c 100644 --- a/pytests/tests/test_hammer_gil_in_thread.py +++ b/pytests/tests/test_hammer_gil_in_thread.py @@ -1,3 +1,7 @@ +import sysconfig + +import pytest + from pyo3_pytests import misc @@ -16,5 +20,9 @@ def make_loop(): loopy = [make_loop()] +@pytest.mark.skipif( + sysconfig.get_config_var("Py_DEBUG"), + reason="causes a crash on debug builds, see discussion in https://github.com/PyO3/pyo3/pull/4874", +) def test_hammer_gil(): loopy.append(misc.hammer_gil_in_thread())