From 89b69ac72354f4f3681444f076401b5a9595ac0c Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 30 Sep 2024 14:55:50 -0600 Subject: [PATCH 1/8] Add critical section API wrappers --- pyo3-ffi/src/cpython/critical_section.rs | 34 +++++++++ src/sync.rs | 94 +++++++++++++++++++++++- 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/pyo3-ffi/src/cpython/critical_section.rs b/pyo3-ffi/src/cpython/critical_section.rs index 97b2f5e0559..ba785481f42 100644 --- a/pyo3-ffi/src/cpython/critical_section.rs +++ b/pyo3-ffi/src/cpython/critical_section.rs @@ -9,6 +9,23 @@ pub struct PyCriticalSection { _cs_mutex: *mut PyMutex, } +#[cfg(Py_GIL_DISABLED)] +impl PyCriticalSection { + pub const fn new() -> PyCriticalSection { + PyCriticalSection { + _cs_prev: 0, + _cs_mutex: std::ptr::null_mut(), + } + } +} + +#[cfg(Py_GIL_DISABLED)] +impl Default for PyCriticalSection { + fn default() -> Self { + PyCriticalSection::new() + } +} + #[repr(C)] #[cfg(Py_GIL_DISABLED)] pub struct PyCriticalSection2 { @@ -16,6 +33,23 @@ pub struct PyCriticalSection2 { _cs_mutex2: *mut PyMutex, } +#[cfg(Py_GIL_DISABLED)] +impl PyCriticalSection2 { + pub const fn new() -> PyCriticalSection2 { + PyCriticalSection2 { + _cs_base: PyCriticalSection::new(), + _cs_mutex2: std::ptr::null_mut(), + } + } +} + +#[cfg(Py_GIL_DISABLED)] +impl Default for PyCriticalSection2 { + fn default() -> Self { + PyCriticalSection2::new() + } +} + #[cfg(not(Py_GIL_DISABLED))] opaque_struct!(PyCriticalSection); diff --git a/src/sync.rs b/src/sync.rs index b02b21def93..bc9dc047b68 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -5,7 +5,7 @@ //! //! [PEP 703]: https://peps.python.org/pep-703/ use crate::{ - types::{any::PyAnyMethods, PyString}, + types::{any::PyAnyMethods, PyAny, PyString}, Bound, Py, PyResult, PyTypeCheck, Python, }; use std::cell::UnsafeCell; @@ -330,11 +330,66 @@ impl Interned { } } +/// Executes a closure on an object with an active Python critical section. +/// +/// Acquires the per-object lock for the object `op` that is held +/// until the closure `f` is finished. +/// +/// This is structurally equivalent to the use of the paired +/// Py_BEGIN_CRITICAL_SECTION and Py_END_CRITICAL_SECTION macros. +/// +/// A no-op on GIL-enabled builds, where the critical section API is exposed as +/// a no-op by the Python C API. +/// +/// Provides weaker locking guarantees than traditional locks, but can in some +/// cases be used to provide guarantees similar to the GIL without the risk of +/// deadlocks associated with traditional locks. +/// +/// Many CPython C API functions do not acquire the per-object lock on objects +/// passed to Python. You should not expect critical sections applied to +/// built-in types to prevent concurrent modification. This API is most useful +/// for user-defined types with full control over how the internal state for the +/// type is managed. +#[cfg_attr(not(Py_GIL_DISABLED), allow(unused_variables))] +pub fn with_critical_section(object: &Bound<'_, PyAny>, f: F) -> R +where + F: FnOnce() -> R, +{ + #[cfg(Py_GIL_DISABLED)] + { + struct Guard(crate::ffi::PyCriticalSection); + + impl Drop for Guard { + fn drop(&mut self) { + unsafe { + crate::ffi::PyCriticalSection_End(&mut self.0); + } + } + } + + let _guard = unsafe { + let mut section = std::mem::zeroed(); + crate::ffi::PyCriticalSection_Begin(&mut section, object.as_ptr()); + Guard(section) + }; + f() + } + #[cfg(not(Py_GIL_DISABLED))] + { + f() + } +} + #[cfg(test)] mod tests { use super::*; - use crate::types::{dict::PyDictMethods, PyDict}; + use crate::types::{PyDict, PyDictMethods}; + + use std::sync::{ + atomic::{AtomicBool, Ordering}, + Barrier, + }; #[test] fn test_intern() { @@ -381,4 +436,39 @@ mod tests { assert!(cell_py.clone_ref(py).get(py).unwrap().is_none(py)); }) } + + #[test] + fn test_critical_section() { + let barrier = Barrier::new(2); + + #[crate::pyclass(crate = "crate")] + struct BoolWrapper(AtomicBool); + + let bool_wrapper = Python::with_gil(|py| -> Py { + Py::new(py, BoolWrapper(AtomicBool::new(false))).unwrap() + }); + + std::thread::scope(|s| { + s.spawn(|| { + Python::with_gil(|py| { + let b = bool_wrapper.bind(py); + with_critical_section(b, || { + barrier.wait(); + std::thread::sleep(std::time::Duration::from_millis(10)); + b.borrow().0.store(true, Ordering::Release); + }) + }); + }); + s.spawn(|| { + barrier.wait(); + Python::with_gil(|py| { + let b = bool_wrapper.bind(py); + // this blocks until the other thread's critical section finishes + with_critical_section(b, || { + assert!(b.borrow().0.load(Ordering::Acquire)); + }); + }); + }); + }); + } } From 66458f51f47f4e3d4e3b8ca0e8b6d05ab1690bfc Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 30 Sep 2024 16:07:37 -0600 Subject: [PATCH 2/8] add changelog entry --- newsfragments/4587.added.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 newsfragments/4587.added.md diff --git a/newsfragments/4587.added.md b/newsfragments/4587.added.md new file mode 100644 index 00000000000..4ccd4cd1c5e --- /dev/null +++ b/newsfragments/4587.added.md @@ -0,0 +1,2 @@ +* Added `with_critical_section`, a safe wrapper around the Python Critical + Section API added in Python 3.13 for the free-threaded build. From 4f182911e8362a03a58806f53ba8d68e566973c9 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 30 Sep 2024 19:11:20 -0600 Subject: [PATCH 3/8] fix no-default-features build --- src/sync.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sync.rs b/src/sync.rs index bc9dc047b68..801e93c99c1 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -386,6 +386,7 @@ mod tests { use crate::types::{PyDict, PyDictMethods}; + #[cfg(feature = "macros")] use std::sync::{ atomic::{AtomicBool, Ordering}, Barrier, @@ -437,6 +438,7 @@ mod tests { }) } + #[cfg(feature = "macros")] #[test] fn test_critical_section() { let barrier = Barrier::new(2); From a5c5a98040a6b454ad5a2e85c4c4dd05e9a57bb4 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 30 Sep 2024 19:15:52 -0600 Subject: [PATCH 4/8] clarify docstring --- src/sync.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sync.rs b/src/sync.rs index 801e93c99c1..08380178ca4 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -330,7 +330,7 @@ impl Interned { } } -/// Executes a closure on an object with an active Python critical section. +/// Executes a closure with a Python critical section held on an object. /// /// Acquires the per-object lock for the object `op` that is held /// until the closure `f` is finished. From 17f9e16a5efa0d441c43018ea1fccadd522b45ae Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 1 Oct 2024 11:09:03 -0600 Subject: [PATCH 5/8] disable test on WASM --- src/sync.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sync.rs b/src/sync.rs index 08380178ca4..f7f1bf6f8f5 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -439,6 +439,7 @@ mod tests { } #[cfg(feature = "macros")] + #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled #[test] fn test_critical_section() { let barrier = Barrier::new(2); From dba597c3b2d26e8762a146e1f2806d37fc5ce320 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 1 Oct 2024 11:20:10 -0600 Subject: [PATCH 6/8] no need to move the section to initialize it --- src/sync.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index f7f1bf6f8f5..9e349f71abe 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -367,11 +367,8 @@ where } } - let _guard = unsafe { - let mut section = std::mem::zeroed(); - crate::ffi::PyCriticalSection_Begin(&mut section, object.as_ptr()); - Guard(section) - }; + let mut guard = Guard(unsafe { std::mem::zeroed() }); + unsafe { crate::ffi::PyCriticalSection_Begin(&mut guard.0, object.as_ptr()) }; f() } #[cfg(not(Py_GIL_DISABLED))] From 96169693206a5e89fc203a6f4f4da2155c056f99 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 1 Oct 2024 11:51:07 -0600 Subject: [PATCH 7/8] fix wasm clippy --- src/sync.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index 9e349f71abe..1555b8dfd43 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -383,12 +383,6 @@ mod tests { use crate::types::{PyDict, PyDictMethods}; - #[cfg(feature = "macros")] - use std::sync::{ - atomic::{AtomicBool, Ordering}, - Barrier, - }; - #[test] fn test_intern() { Python::with_gil(|py| { @@ -439,6 +433,11 @@ mod tests { #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled #[test] fn test_critical_section() { + use std::sync::{ + atomic::{AtomicBool, Ordering}, + Barrier, + }; + let barrier = Barrier::new(2); #[crate::pyclass(crate = "crate")] From f575502222bfa375ea1417794c7641b61f530d52 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 4 Oct 2024 16:11:46 +0100 Subject: [PATCH 8/8] comments from review --- pyo3-ffi/src/cpython/critical_section.rs | 34 ------------------------ src/sync.rs | 2 +- 2 files changed, 1 insertion(+), 35 deletions(-) diff --git a/pyo3-ffi/src/cpython/critical_section.rs b/pyo3-ffi/src/cpython/critical_section.rs index ba785481f42..97b2f5e0559 100644 --- a/pyo3-ffi/src/cpython/critical_section.rs +++ b/pyo3-ffi/src/cpython/critical_section.rs @@ -9,23 +9,6 @@ pub struct PyCriticalSection { _cs_mutex: *mut PyMutex, } -#[cfg(Py_GIL_DISABLED)] -impl PyCriticalSection { - pub const fn new() -> PyCriticalSection { - PyCriticalSection { - _cs_prev: 0, - _cs_mutex: std::ptr::null_mut(), - } - } -} - -#[cfg(Py_GIL_DISABLED)] -impl Default for PyCriticalSection { - fn default() -> Self { - PyCriticalSection::new() - } -} - #[repr(C)] #[cfg(Py_GIL_DISABLED)] pub struct PyCriticalSection2 { @@ -33,23 +16,6 @@ pub struct PyCriticalSection2 { _cs_mutex2: *mut PyMutex, } -#[cfg(Py_GIL_DISABLED)] -impl PyCriticalSection2 { - pub const fn new() -> PyCriticalSection2 { - PyCriticalSection2 { - _cs_base: PyCriticalSection::new(), - _cs_mutex2: std::ptr::null_mut(), - } - } -} - -#[cfg(Py_GIL_DISABLED)] -impl Default for PyCriticalSection2 { - fn default() -> Self { - PyCriticalSection2::new() - } -} - #[cfg(not(Py_GIL_DISABLED))] opaque_struct!(PyCriticalSection); diff --git a/src/sync.rs b/src/sync.rs index 1555b8dfd43..2320a5ec42a 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -336,7 +336,7 @@ impl Interned { /// until the closure `f` is finished. /// /// This is structurally equivalent to the use of the paired -/// Py_BEGIN_CRITICAL_SECTION and Py_END_CRITICAL_SECTION macros. +/// Py_BEGIN_CRITICAL_SECTION and Py_END_CRITICAL_SECTION C-API macros. /// /// A no-op on GIL-enabled builds, where the critical section API is exposed as /// a no-op by the Python C API.