Skip to content
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 critical section API wrappers #4587

Merged
merged 8 commits into from
Oct 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions newsfragments/4587.added.md
Original file line number Diff line number Diff line change
@@ -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.
34 changes: 34 additions & 0 deletions pyo3-ffi/src/cpython/critical_section.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,47 @@ 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()
}
}
davidhewitt marked this conversation as resolved.
Show resolved Hide resolved

#[repr(C)]
#[cfg(Py_GIL_DISABLED)]
pub struct PyCriticalSection2 {
_cs_base: PyCriticalSection,
_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);

Expand Down
96 changes: 94 additions & 2 deletions src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -330,11 +330,67 @@ impl Interned {
}
}

/// 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.
///
/// This is structurally equivalent to the use of the paired
/// Py_BEGIN_CRITICAL_SECTION and Py_END_CRITICAL_SECTION macros.
davidhewitt marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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<F, R>(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)
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not be moving this

Suggested change
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.section, object.as_ptr()) };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add PhantomPinned to PyCriticalSection?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add PhantomPinned to PyCriticalSection?

It doesn't really matter in this case.

Implementing a type that "needs pinning" requires unsafe code, and writing code that needs pinned types also requires unsafe code. Pin is just a way to let safe code carry that promise between two pieces of unsafe code. The latter doesn't happen here.

f()
}
#[cfg(not(Py_GIL_DISABLED))]
{
f()
}
}

#[cfg(test)]
mod tests {
use super::*;

use crate::types::{dict::PyDictMethods, PyDict};
use crate::types::{PyDict, PyDictMethods};

#[cfg(feature = "macros")]
use std::sync::{
atomic::{AtomicBool, Ordering},
Barrier,
};

#[test]
fn test_intern() {
Expand Down Expand Up @@ -381,4 +437,40 @@ mod tests {
assert!(cell_py.clone_ref(py).get(py).unwrap().is_none(py));
})
}

#[cfg(feature = "macros")]
#[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<BoolWrapper> {
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));
});
});
});
});
}
}
Loading