From 9582419723a999642ec741fa4e3df07dcc7ea900 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 31 Oct 2024 21:56:56 +0000 Subject: [PATCH 01/10] add `sync::OnceExt` trait --- src/sealed.rs | 2 ++ src/sync.rs | 61 +++++++++++++++++++++++++++++++- tests/test_declarative_module.rs | 18 +++++++--- 3 files changed, 76 insertions(+), 5 deletions(-) diff --git a/src/sealed.rs b/src/sealed.rs index cc835bee3b8..0a2846b134a 100644 --- a/src/sealed.rs +++ b/src/sealed.rs @@ -53,3 +53,5 @@ impl Sealed for ModuleDef {} impl Sealed for PyNativeTypeInitializer {} impl Sealed for PyClassInitializer {} + +impl Sealed for std::sync::Once {} diff --git a/src/sync.rs b/src/sync.rs index 65a81d06bd5..161327470cc 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -5,10 +5,17 @@ //! //! [PEP 703]: https://peps.python.org/pep-703/ use crate::{ + ffi, + sealed::Sealed, types::{any::PyAnyMethods, PyAny, PyString}, Bound, Py, PyResult, PyTypeCheck, Python, }; -use std::{cell::UnsafeCell, marker::PhantomData, mem::MaybeUninit, sync::Once}; +use std::{ + cell::UnsafeCell, + marker::PhantomData, + mem::MaybeUninit, + sync::{Once, OnceState}, +}; #[cfg(not(Py_GIL_DISABLED))] use crate::PyVisit; @@ -473,6 +480,58 @@ where } } +/// Helper trait for `Once` to help avoid deadlocking when using a `Once` when attached to a +/// Python thread. +pub trait OnceExt: Sealed { + /// Similar to [`call_once`][Once::call_once], but releases the Python GIL temporarily + /// if blocking on another thread currently calling this `Once`. + fn call_once_py_attached(&self, py: Python<'_>, f: impl FnOnce()); + + /// Similar to [`call_once_force`][Once::call_once_force], but releases the Python GIL + /// temporarily if blocking on another thread currently calling this `Once`. + fn call_once_force_py_attached(&self, py: Python<'_>, f: impl FnOnce(&OnceState)); +} + +impl OnceExt for Once { + fn call_once_py_attached(&self, _py: Python<'_>, f: impl FnOnce()) { + if self.is_completed() { + return; + } + + // Safety: we are currently attached to the GIL, and we expect to block. We will save + // the current thread state and restore it as soon as we are done blocking. + let mut ts = Some(unsafe { ffi::PyEval_SaveThread() }); + + self.call_once(|| { + unsafe { ffi::PyEval_RestoreThread(ts.take().unwrap()) }; + f(); + }); + if let Some(ts) = ts { + // Some other thread filled this Once, so we need to restore the GIL state. + unsafe { ffi::PyEval_RestoreThread(ts) }; + } + } + + fn call_once_force_py_attached(&self, _py: Python<'_>, f: impl FnOnce(&OnceState)) { + if self.is_completed() { + return; + } + + // Safety: we are currently attached to the GIL, and we expect to block. We will save + // the current thread state and restore it as soon as we are done blocking. + let mut ts = Some(unsafe { ffi::PyEval_SaveThread() }); + + self.call_once_force(|state| { + unsafe { ffi::PyEval_RestoreThread(ts.take().unwrap()) }; + f(state); + }); + if let Some(ts) = ts { + // Some other thread filled this Once, so we need to restore the GIL state. + unsafe { ffi::PyEval_RestoreThread(ts) }; + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/test_declarative_module.rs b/tests/test_declarative_module.rs index a911702ce20..93e0e1366f0 100644 --- a/tests/test_declarative_module.rs +++ b/tests/test_declarative_module.rs @@ -1,9 +1,11 @@ #![cfg(feature = "macros")] +use std::sync::Once; + use pyo3::create_exception; use pyo3::exceptions::PyException; use pyo3::prelude::*; -use pyo3::sync::GILOnceCell; +use pyo3::sync::{GILOnceCell, OnceExt}; #[path = "../src/tests/common.rs"] mod common; @@ -149,9 +151,17 @@ mod declarative_module2 { fn declarative_module(py: Python<'_>) -> &Bound<'_, PyModule> { static MODULE: GILOnceCell> = GILOnceCell::new(); - MODULE - .get_or_init(py, || pyo3::wrap_pymodule!(declarative_module)(py)) - .bind(py) + static ONCE: Once = Once::new(); + + // Guarantee that the module is only ever initialized once; GILOnceCell can race. + // TODO: use OnceLock when MSRV >= 1.70 + ONCE.call_once_py_attached(py, || { + MODULE + .set(py, pyo3::wrap_pymodule!(declarative_module)(py)) + .expect("only ever set once"); + }); + + MODULE.get(py).expect("once is completed").bind(py) } #[test] From c4a736f70c766406971d1b819a8a59589a292467 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 31 Oct 2024 22:05:13 +0000 Subject: [PATCH 02/10] newsfragment --- newsfragments/4676.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/4676.added.md diff --git a/newsfragments/4676.added.md b/newsfragments/4676.added.md new file mode 100644 index 00000000000..01b650487d8 --- /dev/null +++ b/newsfragments/4676.added.md @@ -0,0 +1 @@ +Add `pyo3::sync::OnceExt` trait. From 4ac16a30d8546d78f5f9618f1ff77419fd06713e Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Fri, 1 Nov 2024 12:45:46 -0600 Subject: [PATCH 03/10] refactor to use RAII guard --- src/sync.rs | 60 ++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index 161327470cc..e1162fd77ba 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -492,6 +492,16 @@ pub trait OnceExt: Sealed { fn call_once_force_py_attached(&self, py: Python<'_>, f: impl FnOnce(&OnceState)); } +struct Guard(Option<*mut crate::ffi::PyThreadState>); + +impl Drop for Guard { + fn drop(&mut self) { + if let Some(ts) = self.0 { + unsafe { ffi::PyEval_RestoreThread(ts) }; + } + } +} + impl OnceExt for Once { fn call_once_py_attached(&self, _py: Python<'_>, f: impl FnOnce()) { if self.is_completed() { @@ -500,16 +510,12 @@ impl OnceExt for Once { // Safety: we are currently attached to the GIL, and we expect to block. We will save // the current thread state and restore it as soon as we are done blocking. - let mut ts = Some(unsafe { ffi::PyEval_SaveThread() }); + let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); self.call_once(|| { - unsafe { ffi::PyEval_RestoreThread(ts.take().unwrap()) }; + unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; f(); }); - if let Some(ts) = ts { - // Some other thread filled this Once, so we need to restore the GIL state. - unsafe { ffi::PyEval_RestoreThread(ts) }; - } } fn call_once_force_py_attached(&self, _py: Python<'_>, f: impl FnOnce(&OnceState)) { @@ -519,16 +525,12 @@ impl OnceExt for Once { // Safety: we are currently attached to the GIL, and we expect to block. We will save // the current thread state and restore it as soon as we are done blocking. - let mut ts = Some(unsafe { ffi::PyEval_SaveThread() }); + let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); self.call_once_force(|state| { - unsafe { ffi::PyEval_RestoreThread(ts.take().unwrap()) }; + unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; f(state); }); - if let Some(ts) = ts { - // Some other thread filled this Once, so we need to restore the GIL state. - unsafe { ffi::PyEval_RestoreThread(ts) }; - } } } @@ -648,4 +650,38 @@ mod tests { }); }); } + + #[test] + fn test_call_once_ext() { + // adapted from the example in the docs for Once::try_once_force + let init = Once::new(); + std::thread::scope(|s| { + // poison the once + let handle = s.spawn(|| { + Python::with_gil(|py| { + init.call_once_py_attached(py, || panic!()); + }) + }); + assert!(handle.join().is_err()); + + // poisoning propagates + let handle = s.spawn(|| { + Python::with_gil(|py| { + init.call_once_py_attached(py, || {}); + }); + }); + + assert!(handle.join().is_err()); + + // call_once_force will still run and reset the poisoned state + Python::with_gil(|py| { + init.call_once_force_py_attached(py, |state| { + assert!(state.is_poisoned()); + }); + + // once any success happens, we stop propagating the poison + init.call_once_py_attached(py, || {}); + }); + }); + } } From 6955bf8a32296473a8dff573d9fa929b72a671c5 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Fri, 1 Nov 2024 15:31:54 -0600 Subject: [PATCH 04/10] Add docs for single-initialization --- guide/src/faq.md | 4 ++- guide/src/free-threading.md | 52 +++++++++++++++++++++++++++++++++++++ guide/src/migration.md | 7 ++++- 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/guide/src/faq.md b/guide/src/faq.md index 5752e14adbd..f6ccdd4da59 100644 --- a/guide/src/faq.md +++ b/guide/src/faq.md @@ -13,9 +13,11 @@ Sorry that you're having trouble using PyO3. If you can't find the answer to you 5. Thread A is blocked, because it waits to re-acquire the GIL which thread B still holds. 6. Deadlock. -PyO3 provides a struct [`GILOnceCell`] which works similarly to these types but avoids risk of deadlocking with the Python GIL. This means it can be used in place of other choices when you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] for further details and an example how to use it. +PyO3 provides a struct [`GILOnceCell`] and an extension trait [`OnceExt`] that +enable functionality similar to these types but avoid the risk of deadlocking with the Python GIL. This means they can be used in place of other choices when you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] and [`OnceExt`] for further details and an example how to use them. [`GILOnceCell`]: {{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html +[`OnceExt`]: {{#PYO3_DOCS_URL}}/pyo3/sync/trait.OnceExt.html ## I can't run `cargo test`; or I can't build in a Cargo workspace: I'm having linker issues like "Symbol not found" or "Undefined reference to _PyExc_SystemError"! diff --git a/guide/src/free-threading.md b/guide/src/free-threading.md index 77b2ff327a2..6f630a1c7f7 100644 --- a/guide/src/free-threading.md +++ b/guide/src/free-threading.md @@ -152,6 +152,58 @@ We plan to allow user-selectable semantics for mutable pyclass definitions in PyO3 0.24, allowing some form of opt-in locking to emulate the GIL if that is needed. +## Thread-safe single initialization + +Until version 0.23, PyO3 provided only `GILOnceCell` to enable deadlock-free +single initialization of data in contexts that might execute arbitrary Python +code. While we have updated `GILOnceCell` to avoid thread safety issues +triggered only under the free-threaded build, the design of `GILOnceCell` is +inherently thread-unsafe, in a manner that can be problematic even in the +GIL-enabled build. + +If, for example, the function executed by `GILOnceCell` releases the GIL or +calls code that releases the GIL, then it is possible for multiple threads to +try to race to initialize the cell. While the cell will only ever be intialized +once, it can be problematic in some contexts that `GILOnceCell` does not block +like the standard library `OnceLock`. + +In cases where the initialization function must run exactly once, you can bring +the `OnceExt` trait into scope. This trait adds `OnceExt::call_once_py_attached` +and `OnceExt::call_once_force_py_attached` functions to the api of +`std::sync::Once`, enabling use of `Once` in contexts where the GIL is +held. These functions are analogous to `Once::call_once` and +`Once::call_once_force` except they both accept a `Python<'py>` token in +addition to an `FnOnce`. Both functions release the GIL and re-acquire it before +executing the function, avoiding deadlocks with the GIL that are possible +without using these functions. Here is an example of how to use this function to +enable single-initialization of a runtime cache: + +```rust +# fn main() { +# use pyo3::prelude::*; +use std::sync::Once; +use pyo3::sync::OnceExt; +use pyo3::types::PyDict; + +struct RuntimeCache { + once: Once, + cache: Option> +} + +let mut cache = RuntimeCache { + once: Once::new(), + cache: None +}; + +Python::with_gil(|py| { + // guaranteed to be called once and only once + cache.once.call_once_py_attached(py, || { + cache.cache = Some(PyDict::new(py).unbind()); + }); +}); +# } +``` + ## `GILProtected` is not exposed `GILProtected` is a PyO3 type that allows mutable access to static data by diff --git a/guide/src/migration.md b/guide/src/migration.md index 0d76d220dc9..0f56498043b 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -230,7 +230,12 @@ PyO3 0.23 introduces preliminary support for the new free-threaded build of CPython 3.13. PyO3 features that implicitly assumed the existence of the GIL are not exposed in the free-threaded build, since they are no longer safe. Other features, such as `GILOnceCell`, have been internally rewritten to be threadsafe -without the GIL. +without the GIL, although note that `GILOnceCell` is inherently racey. You can +also use `OnceExt::call_once_py_attached` or +`OnceExt::call_once_force_py_attached` to enable use of `std::sync::Once` in +code that has the GIL acquired without risking a dealock with the GIL. We plan +We plan to expose more extension traits in the future that make it easier to +write code for the GIL-enabled and free-threaded builds of Python. If you make use of these features then you will need to account for the unavailability of this API in the free-threaded build. One way to handle it is From c3ede2aec8058c496e6a79b98647520a943e309c Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Fri, 1 Nov 2024 16:06:15 -0600 Subject: [PATCH 05/10] mark init logic with #[cold] --- src/sync.rs | 54 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index e1162fd77ba..6f3c6537ce0 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -503,37 +503,53 @@ impl Drop for Guard { } impl OnceExt for Once { - fn call_once_py_attached(&self, _py: Python<'_>, f: impl FnOnce()) { + fn call_once_py_attached(&self, py: Python<'_>, f: impl FnOnce()) { if self.is_completed() { return; } - // Safety: we are currently attached to the GIL, and we expect to block. We will save - // the current thread state and restore it as soon as we are done blocking. - let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); - - self.call_once(|| { - unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; - f(); - }); + init_once_py_attached(self, py, f) } - fn call_once_force_py_attached(&self, _py: Python<'_>, f: impl FnOnce(&OnceState)) { + fn call_once_force_py_attached(&self, py: Python<'_>, f: impl FnOnce(&OnceState)) { if self.is_completed() { return; } - // Safety: we are currently attached to the GIL, and we expect to block. We will save - // the current thread state and restore it as soon as we are done blocking. - let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); - - self.call_once_force(|state| { - unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; - f(state); - }); + init_once_force_py_attached(self, py, f); } } +#[cold] +fn init_once_py_attached(once: &Once, _py: Python<'_>, f: F) +where + F: FnOnce() -> T, +{ + // Safety: we are currently attached to the GIL, and we expect to block. We will save + // the current thread state and restore it as soon as we are done blocking. + let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + + once.call_once(|| { + unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; + f(); + }); +} + +#[cold] +fn init_once_force_py_attached(once: &Once, _py: Python<'_>, f: F) +where + F: FnOnce(&OnceState) -> T, +{ + // Safety: we are currently attached to the GIL, and we expect to block. We will save + // the current thread state and restore it as soon as we are done blocking. + let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + + once.call_once_force(|state| { + unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; + f(state); + }); +} + #[cfg(test)] mod tests { use super::*; @@ -652,7 +668,7 @@ mod tests { } #[test] - fn test_call_once_ext() { + fn test_once_ext() { // adapted from the example in the docs for Once::try_once_force let init = Once::new(); std::thread::scope(|s| { From f0eb7e07f2aa3598e6f18a8a26703fd32d9a1df3 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Fri, 1 Nov 2024 16:15:53 -0600 Subject: [PATCH 06/10] attempt to include OnceLockExt as well --- src/sync.rs | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/sync.rs b/src/sync.rs index 6f3c6537ce0..44a4919f64f 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -480,6 +480,11 @@ where } } +mod once_lock_ext_sealed { + pub trait Sealed {} + impl Sealed for std::sync::OnceLock {} +} + /// Helper trait for `Once` to help avoid deadlocking when using a `Once` when attached to a /// Python thread. pub trait OnceExt: Sealed { @@ -492,6 +497,23 @@ pub trait OnceExt: Sealed { fn call_once_force_py_attached(&self, py: Python<'_>, f: impl FnOnce(&OnceState)); } +// Extension trait for [`std::sync::OnceLock`] which helps avoid deadlocks between the Python +/// interpreter and initialization with the `OnceLock`. +pub trait OnceLockExt: once_lock_ext_sealed::Sealed { + /// Initializes this `OnceLock` with the given closure if it has not been initialized yet. + /// + /// If this function would block, this function detaches from the Python interpreter and + /// reattaches before calling `f`. This avoids deadlocks between the Python interpreter and + /// the `OnceLock` in cases where `f` can call arbitrary Python code, as calling arbitrary + /// Python code can lead to `f` itself blocking on the Python interpreter. + /// + /// By detaching from the Python interpreter before blocking, this ensures that if `f` blocks + /// then the Python interpreter cannot be blocked by `f` itself. + fn get_or_init_py_attached(&self, py: Python<'_>, f: F) -> &T + where + F: FnOnce() -> T; +} + struct Guard(Option<*mut crate::ffi::PyThreadState>); impl Drop for Guard { @@ -520,6 +542,17 @@ impl OnceExt for Once { } } +impl OnceLockExt for std::sync::OnceLock { + fn get_or_init_py_attached(&self, py: Python<'_>, f: F) -> &T + where + F: FnOnce() -> T, + { + // Use self.get() first to create a fast path when initialized + self.get() + .unwrap_or_else(|| init_once_lock_py_attached(self, py, f)) + } +} + #[cold] fn init_once_py_attached(once: &Once, _py: Python<'_>, f: F) where @@ -550,6 +583,30 @@ where }); } +#[cold] +fn init_once_lock_py_attached<'a, F, T>( + lock: &'a std::sync::OnceLock, + _py: Python<'_>, + f: F, +) -> &'a T +where + F: FnOnce() -> T, +{ + // SAFETY: we are currently attached to a Python thread + let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + + // By having detached here, we guarantee that `.get_or_init` cannot deadlock with + // the Python interpreter + let value = lock.get_or_init(move || { + let ts = ts.0.take().expect("ts is set to Some above"); + // SAFETY: ts is a valid thread state and needs restoring + unsafe { ffi::PyEval_RestoreThread(ts) }; + f() + }); + + value +} + #[cfg(test)] mod tests { use super::*; From 7e3d648e1c4f2e7037473acad49164414f998146 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Fri, 1 Nov 2024 17:13:17 -0600 Subject: [PATCH 07/10] Add OnceLockExt --- guide/src/faq.md | 3 +-- guide/src/free-threading.md | 20 +++++++++++--------- newsfragments/4676.added.md | 2 +- pyo3-build-config/src/lib.rs | 5 +++++ src/sync.rs | 34 ++++++++++++++++++++++++++-------- 5 files changed, 44 insertions(+), 20 deletions(-) diff --git a/guide/src/faq.md b/guide/src/faq.md index f6ccdd4da59..75ea043c3a5 100644 --- a/guide/src/faq.md +++ b/guide/src/faq.md @@ -13,8 +13,7 @@ Sorry that you're having trouble using PyO3. If you can't find the answer to you 5. Thread A is blocked, because it waits to re-acquire the GIL which thread B still holds. 6. Deadlock. -PyO3 provides a struct [`GILOnceCell`] and an extension trait [`OnceExt`] that -enable functionality similar to these types but avoid the risk of deadlocking with the Python GIL. This means they can be used in place of other choices when you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] and [`OnceExt`] for further details and an example how to use them. +PyO3 provides a struct [`GILOnceCell`] which implements a single-initialization API based on these types that relies on the GIL for locking. If the GIL is released or there is no GIL, then this type allows the initialization function to race but ensures that the data is only ever initialized once. If you need ot ensure that the initialization function is called once and only once, you can make use of the [`OnceExt`] and [`OnceLockExt`] extension traits that enable using the standard library types for this purpose but provide new methods for these types that avoid the risk of deadlocking with the Python GIL. This means they can be used in place of other choices when you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] and [`OnceExt`] for further details and an example how to use them. [`GILOnceCell`]: {{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html [`OnceExt`]: {{#PYO3_DOCS_URL}}/pyo3/sync/trait.OnceExt.html diff --git a/guide/src/free-threading.md b/guide/src/free-threading.md index 6f630a1c7f7..8100a3d45ef 100644 --- a/guide/src/free-threading.md +++ b/guide/src/free-threading.md @@ -168,15 +168,17 @@ once, it can be problematic in some contexts that `GILOnceCell` does not block like the standard library `OnceLock`. In cases where the initialization function must run exactly once, you can bring -the `OnceExt` trait into scope. This trait adds `OnceExt::call_once_py_attached` -and `OnceExt::call_once_force_py_attached` functions to the api of -`std::sync::Once`, enabling use of `Once` in contexts where the GIL is -held. These functions are analogous to `Once::call_once` and -`Once::call_once_force` except they both accept a `Python<'py>` token in -addition to an `FnOnce`. Both functions release the GIL and re-acquire it before -executing the function, avoiding deadlocks with the GIL that are possible -without using these functions. Here is an example of how to use this function to -enable single-initialization of a runtime cache: +the `OnceExt` or `OnceLockExt` traits into scope. The `OnceExt` trait adds +`OnceExt::call_once_py_attached` and `OnceExt::call_once_force_py_attached` +functions to the api of `std::sync::Once`, enabling use of `Once` in contexts +where the GIL is held. Similarly, `OnceLockExt` adds +`OnceLockExt::get_or_init_py_attached`. These functions are analogous to +`Once::call_once`, `Once::call_once_force`, and `OnceLock::get_or_init` except +they accept a `Python<'py>` token in addition to an `FnOnce`. All of these +functions release the GIL and re-acquire it before executing the function, +avoiding deadlocks with the GIL that are possible without using the PyO3 +extension traits. Here is an example of how to use `OnceExt` to +enable single-initialization of a runtime cache holding a `Py`. ```rust # fn main() { diff --git a/newsfragments/4676.added.md b/newsfragments/4676.added.md index 01b650487d8..730b2297d91 100644 --- a/newsfragments/4676.added.md +++ b/newsfragments/4676.added.md @@ -1 +1 @@ -Add `pyo3::sync::OnceExt` trait. +Add `pyo3::sync::OnceExt` and `pyo3::sync::OnceLockExt` traits. diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 033e7b46540..642fdf1659f 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -138,6 +138,10 @@ fn resolve_cross_compile_config_path() -> Option { pub fn print_feature_cfgs() { let rustc_minor_version = rustc_minor_version().unwrap_or(0); + if rustc_minor_version >= 70 { + println!("cargo:rustc-cfg=rustc_has_once_lock"); + } + // invalid_from_utf8 lint was added in Rust 1.74 if rustc_minor_version >= 74 { println!("cargo:rustc-cfg=invalid_from_utf8_lint"); @@ -175,6 +179,7 @@ pub fn print_expected_cfgs() { println!("cargo:rustc-check-cfg=cfg(pyo3_leak_on_drop_without_reference_pool)"); 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)"); // allow `Py_3_*` cfgs from the minimum supported version up to the // maximum minor version (+1 for development for the next) diff --git a/src/sync.rs b/src/sync.rs index 44a4919f64f..26e9bbd4c6d 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -480,6 +480,7 @@ where } } +#[cfg(rustc_has_once_lock)] mod once_lock_ext_sealed { pub trait Sealed {} impl Sealed for std::sync::OnceLock {} @@ -499,6 +500,7 @@ pub trait OnceExt: Sealed { // Extension trait for [`std::sync::OnceLock`] which helps avoid deadlocks between the Python /// interpreter and initialization with the `OnceLock`. +#[cfg(rustc_has_once_lock)] pub trait OnceLockExt: once_lock_ext_sealed::Sealed { /// Initializes this `OnceLock` with the given closure if it has not been initialized yet. /// @@ -542,6 +544,7 @@ impl OnceExt for Once { } } +#[cfg(rustc_has_once_lock)] impl OnceLockExt for std::sync::OnceLock { fn get_or_init_py_attached(&self, py: Python<'_>, f: F) -> &T where @@ -560,10 +563,10 @@ where { // Safety: we are currently attached to the GIL, and we expect to block. We will save // the current thread state and restore it as soon as we are done blocking. - let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + let ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); once.call_once(|| { - unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; + drop(ts_guard); f(); }); } @@ -575,14 +578,15 @@ where { // Safety: we are currently attached to the GIL, and we expect to block. We will save // the current thread state and restore it as soon as we are done blocking. - let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + let ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); once.call_once_force(|state| { - unsafe { ffi::PyEval_RestoreThread(ts.0.take().unwrap()) }; + drop(ts_guard); f(state); }); } +#[cfg(rustc_has_once_lock)] #[cold] fn init_once_lock_py_attached<'a, F, T>( lock: &'a std::sync::OnceLock, @@ -593,14 +597,12 @@ where F: FnOnce() -> T, { // SAFETY: we are currently attached to a Python thread - let mut ts = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + let ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); // By having detached here, we guarantee that `.get_or_init` cannot deadlock with // the Python interpreter let value = lock.get_or_init(move || { - let ts = ts.0.take().expect("ts is set to Some above"); - // SAFETY: ts is a valid thread state and needs restoring - unsafe { ffi::PyEval_RestoreThread(ts) }; + drop(ts_guard); f() }); @@ -757,4 +759,20 @@ mod tests { }); }); } + + #[cfg(rustc_has_once_lock)] + #[test] + fn test_once_lock_ext() { + let cell = std::sync::OnceLock::new(); + std::thread::scope(|s| { + assert!(cell.get().is_none()); + + s.spawn(|| { + Python::with_gil(|py| { + assert_eq!(*cell.get_or_init_py_attached(py, || 12345), 12345); + }); + }); + }); + assert_eq!(cell.get(), Some(&12345)); + } } From a45974a1818c4b3a4b278f783837abba28fe4622 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Fri, 1 Nov 2024 19:21:12 -0600 Subject: [PATCH 08/10] ignore clippy MSRV lint --- src/sync.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/sync.rs b/src/sync.rs index 26e9bbd4c6d..7c5c7b033d5 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -550,6 +550,9 @@ impl OnceLockExt for std::sync::OnceLock { where F: FnOnce() -> T, { + // this trait is guarded by a rustc version config + // so clippy's MSRV check is wrong + #[allow(clippy::incompatible_msrv)] // Use self.get() first to create a fast path when initialized self.get() .unwrap_or_else(|| init_once_lock_py_attached(self, py, f)) @@ -599,6 +602,9 @@ where // SAFETY: we are currently attached to a Python thread let ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + // this trait is guarded by a rustc version config + // so clippy's MSRV check is wrong + #[allow(clippy::incompatible_msrv)] // By having detached here, we guarantee that `.get_or_init` cannot deadlock with // the Python interpreter let value = lock.get_or_init(move || { From 2d1e397ac90a9d54a4f39f63fd527a9a979fbb24 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 2 Nov 2024 10:29:14 +0000 Subject: [PATCH 09/10] simplify --- guide/src/faq.md | 3 ++- src/sync.rs | 16 +++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/guide/src/faq.md b/guide/src/faq.md index 75ea043c3a5..83089cf395e 100644 --- a/guide/src/faq.md +++ b/guide/src/faq.md @@ -13,10 +13,11 @@ Sorry that you're having trouble using PyO3. If you can't find the answer to you 5. Thread A is blocked, because it waits to re-acquire the GIL which thread B still holds. 6. Deadlock. -PyO3 provides a struct [`GILOnceCell`] which implements a single-initialization API based on these types that relies on the GIL for locking. If the GIL is released or there is no GIL, then this type allows the initialization function to race but ensures that the data is only ever initialized once. If you need ot ensure that the initialization function is called once and only once, you can make use of the [`OnceExt`] and [`OnceLockExt`] extension traits that enable using the standard library types for this purpose but provide new methods for these types that avoid the risk of deadlocking with the Python GIL. This means they can be used in place of other choices when you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] and [`OnceExt`] for further details and an example how to use them. +PyO3 provides a struct [`GILOnceCell`] which implements a single-initialization API based on these types that relies on the GIL for locking. If the GIL is released or there is no GIL, then this type allows the initialization function to race but ensures that the data is only ever initialized once. If you need to ensure that the initialization function is called once and only once, you can make use of the [`OnceExt`] and [`OnceLockExt`] extension traits that enable using the standard library types for this purpose but provide new methods for these types that avoid the risk of deadlocking with the Python GIL. This means they can be used in place of other choices when you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] and [`OnceExt`] for further details and an example how to use them. [`GILOnceCell`]: {{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html [`OnceExt`]: {{#PYO3_DOCS_URL}}/pyo3/sync/trait.OnceExt.html +[`OnceLockExt`]: {{#PYO3_DOCS_URL}}/pyo3/sync/trait.OnceLockExt.html ## I can't run `cargo test`; or I can't build in a Cargo workspace: I'm having linker issues like "Symbol not found" or "Undefined reference to _PyExc_SystemError"! diff --git a/src/sync.rs b/src/sync.rs index 7c5c7b033d5..230aeaba57d 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -516,13 +516,11 @@ pub trait OnceLockExt: once_lock_ext_sealed::Sealed { F: FnOnce() -> T; } -struct Guard(Option<*mut crate::ffi::PyThreadState>); +struct Guard(*mut crate::ffi::PyThreadState); impl Drop for Guard { fn drop(&mut self) { - if let Some(ts) = self.0 { - unsafe { ffi::PyEval_RestoreThread(ts) }; - } + unsafe { ffi::PyEval_RestoreThread(self.0) }; } } @@ -566,9 +564,9 @@ where { // Safety: we are currently attached to the GIL, and we expect to block. We will save // the current thread state and restore it as soon as we are done blocking. - let ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + let ts_guard = Guard(unsafe { ffi::PyEval_SaveThread() }); - once.call_once(|| { + once.call_once(move || { drop(ts_guard); f(); }); @@ -581,9 +579,9 @@ where { // Safety: we are currently attached to the GIL, and we expect to block. We will save // the current thread state and restore it as soon as we are done blocking. - let ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + let ts_guard = Guard(unsafe { ffi::PyEval_SaveThread() }); - once.call_once_force(|state| { + once.call_once_force(move |state| { drop(ts_guard); f(state); }); @@ -600,7 +598,7 @@ where F: FnOnce() -> T, { // SAFETY: we are currently attached to a Python thread - let ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); + let ts_guard = Guard(unsafe { ffi::PyEval_SaveThread() }); // this trait is guarded by a rustc version config // so clippy's MSRV check is wrong From 9e10588d2ec286673c1e79d8fcbc4db139744997 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Sat, 2 Nov 2024 08:26:56 -0600 Subject: [PATCH 10/10] fix wasm tests --- src/sync.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sync.rs b/src/sync.rs index 230aeaba57d..0845eaf8cec 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -731,6 +731,7 @@ mod tests { } #[test] + #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled fn test_once_ext() { // adapted from the example in the docs for Once::try_once_force let init = Once::new(); @@ -765,6 +766,7 @@ mod tests { } #[cfg(rustc_has_once_lock)] + #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled #[test] fn test_once_lock_ext() { let cell = std::sync::OnceLock::new();