-
Notifications
You must be signed in to change notification settings - Fork 782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add sync::OnceExt
and sync::OnceLockExt
traits
#4676
Changes from 7 commits
9582419
c4a736f
4ac16a3
6955bf8
c3ede2a
f0eb7e0
7e3d648
a45974a
2d1e397
9e10588
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add `pyo3::sync::OnceExt` and `pyo3::sync::OnceLockExt` traits. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,135 @@ where | |
} | ||
} | ||
|
||
#[cfg(rustc_has_once_lock)] | ||
mod once_lock_ext_sealed { | ||
pub trait Sealed {} | ||
impl<T> Sealed for std::sync::OnceLock<T> {} | ||
} | ||
|
||
/// 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)); | ||
} | ||
|
||
// 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<T>: 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<F>(&self, py: Python<'_>, f: F) -> &T | ||
where | ||
F: FnOnce() -> T; | ||
} | ||
|
||
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) }; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wrote a test for this trait based on the standard library example for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the guard fix your problem? That'd be surprising to me, because I reviewed the previous implementation and believed it was sound. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh nevermind; I read the Once documentation and if it is poisoned it will panic before the closure is called. |
||
} | ||
|
||
impl OnceExt for Once { | ||
fn call_once_py_attached(&self, py: Python<'_>, f: impl FnOnce()) { | ||
if self.is_completed() { | ||
return; | ||
} | ||
|
||
init_once_py_attached(self, py, f) | ||
} | ||
|
||
fn call_once_force_py_attached(&self, py: Python<'_>, f: impl FnOnce(&OnceState)) { | ||
if self.is_completed() { | ||
return; | ||
} | ||
|
||
init_once_force_py_attached(self, py, f); | ||
} | ||
} | ||
|
||
#[cfg(rustc_has_once_lock)] | ||
impl<T> OnceLockExt<T> for std::sync::OnceLock<T> { | ||
fn get_or_init_py_attached<F>(&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<F, T>(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 ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); | ||
|
||
once.call_once(|| { | ||
drop(ts_guard); | ||
f(); | ||
}); | ||
} | ||
|
||
#[cold] | ||
fn init_once_force_py_attached<F, T>(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 ts_guard = Guard(Some(unsafe { ffi::PyEval_SaveThread() })); | ||
|
||
once.call_once_force(|state| { | ||
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<T>, | ||
_py: Python<'_>, | ||
f: F, | ||
) -> &'a T | ||
where | ||
F: FnOnce() -> T, | ||
{ | ||
// SAFETY: we are currently attached to a Python thread | ||
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 || { | ||
drop(ts_guard); | ||
f() | ||
}); | ||
|
||
value | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
@@ -589,4 +725,54 @@ mod tests { | |
}); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn test_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, || {}); | ||
}); | ||
}); | ||
} | ||
|
||
#[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)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestions welcome for a more interesting test |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably want to rewrite this example to use OnceLock