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 PyMutex wrappers #4523

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
1 change: 1 addition & 0 deletions newsfragments/4523.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Added a Rust wrapper for `PyMutex`, available on Python 3.13 and newer.
9 changes: 9 additions & 0 deletions pyo3-ffi/src/cpython/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ pub struct PyMutex {
pub(crate) _pin: PhantomPinned,
Copy link
Member

Choose a reason for hiding this comment

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

This not part of the PR but since colesbury's clarification I believe this is unnecessary, so it can be removed.

}

impl PyMutex {
pub const fn new() -> PyMutex {
PyMutex {
_bits: AtomicU8::new(0),
_pin: PhantomPinned,
}
}
}

extern "C" {
pub fn PyMutex_Lock(m: *mut PyMutex);
pub fn PyMutex_Unlock(m: *mut PyMutex);
Expand Down
9 changes: 2 additions & 7 deletions pyo3-ffi/src/object.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
use crate::pyport::{Py_hash_t, Py_ssize_t};
#[cfg(Py_GIL_DISABLED)]
use crate::PyMutex;
#[cfg(Py_GIL_DISABLED)]
use std::marker::PhantomPinned;
use std::mem;
use std::os::raw::{c_char, c_int, c_uint, c_ulong, c_void};
use std::ptr;
#[cfg(Py_GIL_DISABLED)]
use std::sync::atomic::{AtomicIsize, AtomicU32, AtomicU8, Ordering::Relaxed};
use std::sync::atomic::{AtomicIsize, AtomicU32, Ordering::Relaxed};

#[cfg(Py_LIMITED_API)]
opaque_struct!(PyTypeObject);
Expand Down Expand Up @@ -39,10 +37,7 @@ pub const PyObject_HEAD_INIT: PyObject = PyObject {
#[cfg(Py_GIL_DISABLED)]
_padding: 0,
#[cfg(Py_GIL_DISABLED)]
ob_mutex: PyMutex {
_bits: AtomicU8::new(0),
_pin: PhantomPinned,
},
ob_mutex: PyMutex::new(),
#[cfg(Py_GIL_DISABLED)]
ob_gc_bits: 0,
#[cfg(Py_GIL_DISABLED)]
Expand Down
3 changes: 3 additions & 0 deletions src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub use self::list::{PyList, PyListMethods};
pub use self::mapping::{PyMapping, PyMappingMethods};
pub use self::memoryview::PyMemoryView;
pub use self::module::{PyModule, PyModuleMethods};
pub use self::mutex::{PyMutex, PyMutexGuard};
pub use self::none::PyNone;
pub use self::notimplemented::PyNotImplemented;
#[allow(deprecated)]
Expand Down Expand Up @@ -251,6 +252,8 @@ pub(crate) mod list;
pub(crate) mod mapping;
mod memoryview;
pub(crate) mod module;
#[cfg(Py_3_13)]
mod mutex;
mod none;
mod notimplemented;
mod num;
Expand Down
98 changes: 98 additions & 0 deletions src/types/mutex.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
use std::cell::UnsafeCell;
use std::ops::{Deref, DerefMut};

/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex), exposing an RAII guard interface similar to `std::sync::Mutex`.
Copy link
Member

Choose a reason for hiding this comment

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

This should go into depth about how it's different from std's Mutex as well.

#[derive(Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

I think a derived Debug implementation here is unsound as it could allow reading while another thread is writing. We should check what the debug implementation for std::sync::Mutex prints.

Copy link
Member

Choose a reason for hiding this comment

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

Std's Mutex' Debug impl uses try_lock, which PyMutex doesn't have. We can have a Debug impl, it just can't print any of its contents.

pub struct PyMutex<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub struct PyMutex<T> {
pub struct PyMutex<T: ?Sized> {

We should be able to support unsized types, I think.

_mutex: UnsafeCell<crate::ffi::PyMutex>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_mutex: UnsafeCell<crate::ffi::PyMutex>,
inner: UnsafeCell<crate::ffi::PyMutex>,

To avoid a bunch of self.mutex_mutex or self.mutex.mutex (if the underscore were to be removed)

data: UnsafeCell<T>,
}

/// RAII guard to handle releasing a PyMutex lock.
#[derive(Debug)]
pub struct PyMutexGuard<'a, T> {
mutex: &'a mut PyMutex<T>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mutex: &'a mut PyMutex<T>,
mutex: &'a PyMutex<T>,

}

impl<T> PyMutex<T> {
/// Acquire the mutex, blocking the current thread until it is able to do so.
pub fn lock(&mut self) -> PyMutexGuard<'_, T> {
unsafe { crate::ffi::PyMutex_Lock(self._mutex.get_mut()) };
PyMutexGuard { mutex: &mut *self }
}

/// Create a new mutex in an unlocked state ready for use.
pub fn new(value: T) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

This can be const

Suggested change
pub fn new(value: T) -> Self {
pub const fn new(value: T) -> Self {

Self {
_mutex: UnsafeCell::new(crate::ffi::PyMutex::new()),
data: UnsafeCell::new(value),
}
}
}

impl<'a, T> Drop for PyMutexGuard<'a, T> {
fn drop(&mut self) {
unsafe { crate::ffi::PyMutex_Unlock(self.mutex._mutex.get_mut()) };
}
}

impl<'a, T> Deref for PyMutexGuard<'a, T> {
ngoldbaum marked this conversation as resolved.
Show resolved Hide resolved
type Target = T;

fn deref(&self) -> &T {
// safety: cannot be null pointer because PyMutexGuard::new always
// creates a valid PyMutex pointer
unsafe { &*self.mutex.data.get() }
}
}

impl<'a, T> DerefMut for PyMutexGuard<'a, T> {
fn deref_mut(&mut self) -> &mut T {
// safety: cannot be null pointer because PyMutexGuard::new always
// creates a valid PyMutex pointer
self.mutex.data.get_mut()
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::sync::GILOnceCell;
use crate::types::{PyAnyMethods, PyDict, PyDictMethods, PyNone};
use crate::Py;
use crate::Python;

#[test]
fn test_pymutex() {
let mut mutex = Python::with_gil(|py| -> PyMutex<Py<PyDict>> {
let d = PyDict::new(py);
PyMutex::new(d.unbind())
});

Python::with_gil(|py| {
let mut mutex = py.allow_threads(|| -> PyMutex<Py<PyDict>> {
std::thread::spawn(|| {
let dict_guard = mutex.lock();
Python::with_gil(|py| {
let dict = dict_guard.bind(py);
dict.set_item(PyNone::get(py), PyNone::get(py)).unwrap();
});
drop(dict_guard);
mutex
})
.join()
.unwrap()
});

let dict_guard = mutex.lock();
let d = dict_guard.bind(py);

assert!(d
.get_item(PyNone::get(py))
.unwrap()
.unwrap()
.eq(PyNone::get(py))
.unwrap());
});
}
}
Loading