From 486528916695ed25dd10e13714497f39a213d4b2 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 3 Sep 2024 14:05:08 -0600 Subject: [PATCH 01/15] define public pyo3_ffi::PyMutex::new() --- pyo3-ffi/src/cpython/lock.rs | 9 +++++++++ pyo3-ffi/src/object.rs | 9 ++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pyo3-ffi/src/cpython/lock.rs b/pyo3-ffi/src/cpython/lock.rs index 6c80b00d3c1..c451666e762 100644 --- a/pyo3-ffi/src/cpython/lock.rs +++ b/pyo3-ffi/src/cpython/lock.rs @@ -8,6 +8,15 @@ pub struct PyMutex { pub(crate) _pin: PhantomPinned, } +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); diff --git a/pyo3-ffi/src/object.rs b/pyo3-ffi/src/object.rs index fc3484be102..e775a4625aa 100644 --- a/pyo3-ffi/src/object.rs +++ b/pyo3-ffi/src/object.rs @@ -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); @@ -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)] From 874d608e9bf0b5da0069f55ee6aba33e7a09708b Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 3 Sep 2024 14:06:49 -0600 Subject: [PATCH 02/15] add pyo3::types::mutex for PyMutex wrappers --- src/types/mod.rs | 3 ++ src/types/mutex.rs | 86 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 src/types/mutex.rs diff --git a/src/types/mod.rs b/src/types/mod.rs index d1020931d76..a721c79aa60 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -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)] @@ -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; diff --git a/src/types/mutex.rs b/src/types/mutex.rs new file mode 100644 index 00000000000..c0c139159ea --- /dev/null +++ b/src/types/mutex.rs @@ -0,0 +1,86 @@ +use std::ops::Deref; + +/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex) exposing an RAII interface. +#[derive(Debug)] +pub struct PyMutex { + _mutex: crate::ffi::PyMutex, + data: T, +} + +/// RAII guard to handle releasing a PyMutex lock. +#[derive(Debug)] +pub struct PyMutexGuard<'a, T> { + _mutex: &'a mut crate::ffi::PyMutex, + data: &'a T, +} + +impl PyMutex { + /// 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(&mut self._mutex) }; + PyMutexGuard { + _mutex: &mut self._mutex, + data: &self.data, + } + } + + /// Create a new mutex in an unlocked state ready for use. + pub fn new(value: T) -> Self { + Self { + _mutex: crate::ffi::PyMutex::new(), + data: value, + } + } +} + +impl<'a, T> Drop for PyMutexGuard<'a, T> { + fn drop(&mut self) { + unsafe { crate::ffi::PyMutex_Unlock(self._mutex) }; + } +} + +impl<'a, T> Deref for PyMutexGuard<'a, T> { + type Target = T; + + fn deref(&self) -> &T { + self.data + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::types::{PyAnyMethods, PyDict, PyDictMethods, PyList, PyNone}; + use crate::Python; + + #[test] + fn test_pymutex() { + Python::with_gil(|py| { + let d = PyDict::new(py); + let mut mutex = PyMutex::new(&d); + + let list = Python::with_gil(|py| PyList::new(py, vec!["foo", "bar"]).unbind()); + let dict_guard = mutex.lock(); + + py.allow_threads(|| { + std::thread::spawn(move || { + drop(list); + }) + .join() + .unwrap(); + }); + + dict_guard + .set_item(PyNone::get(py), PyNone::get(py)) + .unwrap(); + drop(dict_guard); + + assert!(d + .get_item(PyNone::get(py)) + .unwrap() + .unwrap() + .eq(PyNone::get(py)) + .unwrap()); + }); + } +} From b1afb8bcb49be3b27568e3a5f670f90fed01a1a0 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 3 Sep 2024 14:16:20 -0600 Subject: [PATCH 03/15] add changelog entry --- newsfragments/4523.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/4523.added.md diff --git a/newsfragments/4523.added.md b/newsfragments/4523.added.md new file mode 100644 index 00000000000..6dd1bcf1cdd --- /dev/null +++ b/newsfragments/4523.added.md @@ -0,0 +1 @@ +* Added a Rust wrapper for `PyMutex`, available on Python 3.13 and newer. From b3d193b17b7905f788d337d3a888d9dc774fa6d7 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 17 Sep 2024 15:22:48 -0600 Subject: [PATCH 04/15] update rust API for PyMutex bindings following code review --- src/types/mutex.rs | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index c0c139159ea..1e649fc7fd1 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -1,41 +1,38 @@ -use std::ops::Deref; +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 interface. +/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex), exposing an RAII guard interface similar to `std::sync::Mutex`. #[derive(Debug)] pub struct PyMutex { - _mutex: crate::ffi::PyMutex, - data: T, + _mutex: UnsafeCell, + data: UnsafeCell, } /// RAII guard to handle releasing a PyMutex lock. #[derive(Debug)] pub struct PyMutexGuard<'a, T> { - _mutex: &'a mut crate::ffi::PyMutex, - data: &'a T, + mutex: &'a mut PyMutex, } impl PyMutex { /// 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(&mut self._mutex) }; - PyMutexGuard { - _mutex: &mut self._mutex, - data: &self.data, - } + 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 { Self { - _mutex: crate::ffi::PyMutex::new(), - data: value, + _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) }; + unsafe { crate::ffi::PyMutex_Unlock(self.mutex._mutex.get_mut()) }; } } @@ -43,7 +40,17 @@ impl<'a, T> Deref for PyMutexGuard<'a, T> { type Target = T; fn deref(&self) -> &T { - self.data + // 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() } } From 12296343c88cc0e854a5bd9e6b8f65e3305219df Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 17 Sep 2024 15:22:58 -0600 Subject: [PATCH 05/15] make the test do something more interesting --- src/types/mutex.rs | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index 1e649fc7fd1..1b792c21f75 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -57,30 +57,35 @@ impl<'a, T> DerefMut for PyMutexGuard<'a, T> { #[cfg(test)] mod tests { use super::*; - use crate::types::{PyAnyMethods, PyDict, PyDictMethods, PyList, PyNone}; + use crate::sync::GILOnceCell; + use crate::types::{PyAnyMethods, PyDict, PyDictMethods, PyNone}; + use crate::Py; use crate::Python; #[test] fn test_pymutex() { - Python::with_gil(|py| { + let mut mutex = Python::with_gil(|py| -> PyMutex> { let d = PyDict::new(py); - let mut mutex = PyMutex::new(&d); - - let list = Python::with_gil(|py| PyList::new(py, vec!["foo", "bar"]).unbind()); - let dict_guard = mutex.lock(); + PyMutex::new(d.unbind()) + }); - py.allow_threads(|| { - std::thread::spawn(move || { - drop(list); + Python::with_gil(|py| { + let mut mutex = py.allow_threads(|| -> PyMutex> { + 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(); + .unwrap() }); - dict_guard - .set_item(PyNone::get(py), PyNone::get(py)) - .unwrap(); - drop(dict_guard); + let dict_guard = mutex.lock(); + let d = dict_guard.bind(py); assert!(d .get_item(PyNone::get(py)) From 307e8cf5c7e678806aa8195f18c334ecc7734a99 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 17 Sep 2024 16:26:33 -0600 Subject: [PATCH 06/15] add Sync impl and make lock accept an immutable reference --- src/types/mutex.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index 1b792c21f75..c4d49abc57c 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -11,14 +11,14 @@ pub struct PyMutex { /// RAII guard to handle releasing a PyMutex lock. #[derive(Debug)] pub struct PyMutexGuard<'a, T> { - mutex: &'a mut PyMutex, + mutex: &'a PyMutex, } impl PyMutex { /// 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 } + pub fn lock(&self) -> PyMutexGuard<'_, T> { + unsafe { crate::ffi::PyMutex_Lock(UnsafeCell::raw_get(&self._mutex)) }; + PyMutexGuard { mutex: self } } /// Create a new mutex in an unlocked state ready for use. @@ -30,9 +30,11 @@ impl PyMutex { } } +unsafe impl Sync for PyMutex {} + impl<'a, T> Drop for PyMutexGuard<'a, T> { fn drop(&mut self) { - unsafe { crate::ffi::PyMutex_Unlock(self.mutex._mutex.get_mut()) }; + unsafe { crate::ffi::PyMutex_Unlock(UnsafeCell::raw_get(&self.mutex._mutex)) }; } } @@ -50,27 +52,26 @@ 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() + unsafe { &mut *self.mutex.data.get() } } } #[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> { + let mutex = Python::with_gil(|py| -> PyMutex> { let d = PyDict::new(py); PyMutex::new(d.unbind()) }); Python::with_gil(|py| { - let mut mutex = py.allow_threads(|| -> PyMutex> { + let mutex = py.allow_threads(|| -> PyMutex> { std::thread::spawn(|| { let dict_guard = mutex.lock(); Python::with_gil(|py| { From f0ead33e1f2834b768f9dea8d69528c87f99d35a Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 11:23:55 -0600 Subject: [PATCH 07/15] respond to review comments --- pyo3-ffi/src/cpython/lock.rs | 9 ++++++--- src/types/mod.rs | 1 + src/types/mutex.rs | 23 +++++++++++------------ 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/pyo3-ffi/src/cpython/lock.rs b/pyo3-ffi/src/cpython/lock.rs index c451666e762..2b45e9995b0 100644 --- a/pyo3-ffi/src/cpython/lock.rs +++ b/pyo3-ffi/src/cpython/lock.rs @@ -1,22 +1,25 @@ -use std::marker::PhantomPinned; use std::sync::atomic::AtomicU8; #[repr(transparent)] #[derive(Debug)] pub struct PyMutex { pub(crate) _bits: AtomicU8, - pub(crate) _pin: PhantomPinned, } impl PyMutex { pub const fn new() -> PyMutex { PyMutex { _bits: AtomicU8::new(0), - _pin: PhantomPinned, } } } +impl Default for PyMutex { + fn default() -> Self { + Self::new() + } +} + extern "C" { pub fn PyMutex_Lock(m: *mut PyMutex); pub fn PyMutex_Unlock(m: *mut PyMutex); diff --git a/src/types/mod.rs b/src/types/mod.rs index a721c79aa60..ab264e87c33 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -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}; +#[cfg(all(not(Py_LIMITED_API), Py_3_13))] pub use self::mutex::{PyMutex, PyMutexGuard}; pub use self::none::PyNone; pub use self::notimplemented::PyNotImplemented; diff --git a/src/types/mutex.rs b/src/types/mutex.rs index c4d49abc57c..f690bfcd48e 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -2,39 +2,38 @@ 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`. -#[derive(Debug)] -pub struct PyMutex { - _mutex: UnsafeCell, +pub struct PyMutex { + mutex: UnsafeCell, data: UnsafeCell, } /// RAII guard to handle releasing a PyMutex lock. -#[derive(Debug)] pub struct PyMutexGuard<'a, T> { - mutex: &'a PyMutex, + inner: &'a PyMutex, } impl PyMutex { /// Acquire the mutex, blocking the current thread until it is able to do so. pub fn lock(&self) -> PyMutexGuard<'_, T> { - unsafe { crate::ffi::PyMutex_Lock(UnsafeCell::raw_get(&self._mutex)) }; - PyMutexGuard { mutex: self } + unsafe { crate::ffi::PyMutex_Lock(UnsafeCell::raw_get(&self.mutex)) }; + PyMutexGuard { inner: self } } /// Create a new mutex in an unlocked state ready for use. pub fn new(value: T) -> Self { Self { - _mutex: UnsafeCell::new(crate::ffi::PyMutex::new()), + mutex: UnsafeCell::new(crate::ffi::PyMutex::new()), data: UnsafeCell::new(value), } } } +// safety: PyMutex serializes access unsafe impl Sync for PyMutex {} impl<'a, T> Drop for PyMutexGuard<'a, T> { fn drop(&mut self) { - unsafe { crate::ffi::PyMutex_Unlock(UnsafeCell::raw_get(&self.mutex._mutex)) }; + unsafe { crate::ffi::PyMutex_Unlock(UnsafeCell::raw_get(&self.inner.mutex)) }; } } @@ -42,15 +41,15 @@ impl<'a, T> Deref for PyMutexGuard<'a, T> { type Target = T; fn deref(&self) -> &T { - // safety: cannot be null pointer because PyMutexGuard::new always + // safety: cannot be null pointer because PyMutex::new always // creates a valid PyMutex pointer - unsafe { &*self.mutex.data.get() } + unsafe { &*self.inner.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 + // safety: cannot be null pointer because PyMutex::new always // creates a valid PyMutex pointer unsafe { &mut *self.mutex.data.get() } } From 4a0bf3b91a4d7f2e5aec209be6e93ec3c8aef006 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 11:24:10 -0600 Subject: [PATCH 08/15] add test for PyMutex blocking behavior --- src/types/mutex.rs | 47 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index f690bfcd48e..b0d11aaab14 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -51,12 +51,14 @@ impl<'a, T> DerefMut for PyMutexGuard<'a, T> { fn deref_mut(&mut self) -> &mut T { // safety: cannot be null pointer because PyMutex::new always // creates a valid PyMutex pointer - unsafe { &mut *self.mutex.data.get() } + unsafe { &mut *self.inner.data.get() } } } #[cfg(test)] mod tests { + use std::sync::{mpsc::sync_channel, OnceLock}; + use super::*; use crate::types::{PyAnyMethods, PyDict, PyDictMethods, PyNone}; use crate::Py; @@ -95,4 +97,47 @@ mod tests { .unwrap()); }); } + + #[test] + fn test_pymutex_blocks() { + let mutex = OnceLock::>::new(); + let first_thread_locked_once = OnceLock::::new(); + let second_thread_locked_once = OnceLock::::new(); + let finished = OnceLock::::new(); + let (sender, receiver) = sync_channel::(0); + + mutex.get_or_init(|| PyMutex::new(())); + + std::thread::scope(|s| { + s.spawn(|| { + let guard = mutex.get().unwrap().lock(); + first_thread_locked_once.set(true).unwrap(); + while finished.get().is_none() { + if second_thread_locked_once.get().is_some() { + // Wait a little to guard against the unlikely event that + // the other thread isn't blocked on acquiring the mutex yet. + // If PyMutex had a try_lock implementation this would be + // unnecessary + std::thread::sleep(std::time::Duration::from_millis(10)); + // block (and hold the mutex) until the receiver actually receives something + sender.send(true).unwrap(); + finished.set(true).unwrap(); + } + } + drop(guard); + }); + + s.spawn(|| { + while first_thread_locked_once.get().is_none() {} + let mutex = mutex.get().unwrap(); + second_thread_locked_once.set(true).unwrap(); + let guard = mutex.lock(); + assert!(finished.get().unwrap()); + drop(guard); + }); + + // threads are blocked until we receive + receiver.recv().unwrap(); + }); + } } From 17263cc7b13e6f86b986d490153f68d0ba504600 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 13:24:35 -0600 Subject: [PATCH 09/15] expand docstrings --- src/types/mutex.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index b0d11aaab14..629ea9a94b6 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -1,13 +1,25 @@ 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`. +/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex), exposing an RAII guard interface. +/// +/// Comapred with `std::sync::Mutex` or `parking_lot::Mutex`, this is a very +/// stripped-down locking primitive that only supports blocking lock and unlock +/// operations. + +/// `PyMutex` is hooked into CPython's garbage collector and the GIL in GIL-enabled +/// builds. If a thread is blocked on aquiring the mutex and holds the GIL or would +/// prevent Python from entering garbage collection, then Python will release the +/// thread state, allowing garbage collection or other threads blocked by the GIL to +/// proceed. This means it is impossible for PyMutex to deadlock with the GIL. pub struct PyMutex { mutex: UnsafeCell, data: UnsafeCell, } /// RAII guard to handle releasing a PyMutex lock. +/// +/// The lock is released when `PyMutexGuard` is dropped. pub struct PyMutexGuard<'a, T> { inner: &'a PyMutex, } From 37ffa3ab2757c7038661bb1bc9a1a96babd33408 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 13:43:41 -0600 Subject: [PATCH 10/15] fix clippy under abi3 --- src/types/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/mod.rs b/src/types/mod.rs index ab264e87c33..f44d3673990 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -253,7 +253,7 @@ pub(crate) mod list; pub(crate) mod mapping; mod memoryview; pub(crate) mod module; -#[cfg(Py_3_13)] +#[cfg(all(not(Py_LIMITED_API), Py_3_13))] mod mutex; mod none; mod notimplemented; From 10f5faf33dc685ecbb85a12fefca24a5d038a819 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 13:50:41 -0600 Subject: [PATCH 11/15] simplify test --- src/types/mutex.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index 629ea9a94b6..89be2cba909 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -112,17 +112,15 @@ mod tests { #[test] fn test_pymutex_blocks() { - let mutex = OnceLock::>::new(); + let mutex = PyMutex::new(()); let first_thread_locked_once = OnceLock::::new(); let second_thread_locked_once = OnceLock::::new(); let finished = OnceLock::::new(); let (sender, receiver) = sync_channel::(0); - mutex.get_or_init(|| PyMutex::new(())); - std::thread::scope(|s| { s.spawn(|| { - let guard = mutex.get().unwrap().lock(); + let guard = mutex.lock(); first_thread_locked_once.set(true).unwrap(); while finished.get().is_none() { if second_thread_locked_once.get().is_some() { @@ -141,7 +139,6 @@ mod tests { s.spawn(|| { while first_thread_locked_once.get().is_none() {} - let mutex = mutex.get().unwrap(); second_thread_locked_once.set(true).unwrap(); let guard = mutex.lock(); assert!(finished.get().unwrap()); From 7df69a05ed8c6e550c81f1e1ae6b92e9e4c3f5f6 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 14:18:35 -0600 Subject: [PATCH 12/15] use AtomicBool instead of OnceLock --- src/types/mutex.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index 89be2cba909..a082adf904e 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -69,7 +69,10 @@ impl<'a, T> DerefMut for PyMutexGuard<'a, T> { #[cfg(test)] mod tests { - use std::sync::{mpsc::sync_channel, OnceLock}; + use std::sync::{ + atomic::{AtomicBool, Ordering}, + mpsc::sync_channel, + }; use super::*; use crate::types::{PyAnyMethods, PyDict, PyDictMethods, PyNone}; @@ -113,17 +116,17 @@ mod tests { #[test] fn test_pymutex_blocks() { let mutex = PyMutex::new(()); - let first_thread_locked_once = OnceLock::::new(); - let second_thread_locked_once = OnceLock::::new(); - let finished = OnceLock::::new(); + let first_thread_locked_once = AtomicBool::new(false); + let second_thread_locked_once = AtomicBool::new(false); + let finished = AtomicBool::new(false); let (sender, receiver) = sync_channel::(0); std::thread::scope(|s| { s.spawn(|| { let guard = mutex.lock(); - first_thread_locked_once.set(true).unwrap(); - while finished.get().is_none() { - if second_thread_locked_once.get().is_some() { + first_thread_locked_once.store(true, Ordering::SeqCst); + while !finished.load(Ordering::SeqCst) { + if second_thread_locked_once.load(Ordering::SeqCst) { // Wait a little to guard against the unlikely event that // the other thread isn't blocked on acquiring the mutex yet. // If PyMutex had a try_lock implementation this would be @@ -131,17 +134,17 @@ mod tests { std::thread::sleep(std::time::Duration::from_millis(10)); // block (and hold the mutex) until the receiver actually receives something sender.send(true).unwrap(); - finished.set(true).unwrap(); + finished.store(true, Ordering::SeqCst); } } drop(guard); }); s.spawn(|| { - while first_thread_locked_once.get().is_none() {} - second_thread_locked_once.set(true).unwrap(); + while !first_thread_locked_once.load(Ordering::SeqCst) {} + second_thread_locked_once.store(true, Ordering::SeqCst); let guard = mutex.lock(); - assert!(finished.get().unwrap()); + assert!(finished.load(Ordering::SeqCst)); drop(guard); }); From e35ea76652eea6b2dc74c56958c2dd745360f51c Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 14:38:05 -0600 Subject: [PATCH 13/15] use std::hint::spin_loop() per clippy lint --- src/types/mutex.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index a082adf904e..88ac2266380 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -141,7 +141,9 @@ mod tests { }); s.spawn(|| { - while !first_thread_locked_once.load(Ordering::SeqCst) {} + while !first_thread_locked_once.load(Ordering::SeqCst) { + std::hint::spin_loop(); + } second_thread_locked_once.store(true, Ordering::SeqCst); let guard = mutex.lock(); assert!(finished.load(Ordering::SeqCst)); From 604b9bb6d4fb2a1869d2692a70188577891194c2 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 19 Sep 2024 15:00:41 -0600 Subject: [PATCH 14/15] make PyMutex::new() const --- src/types/mutex.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index 88ac2266380..13aec60ddda 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -32,7 +32,7 @@ impl PyMutex { } /// Create a new mutex in an unlocked state ready for use. - 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), From 22ee3517edbbbbe6580a6d6469407e41ffa56334 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Fri, 20 Sep 2024 08:25:31 -0600 Subject: [PATCH 15/15] fix docstring formatting --- src/types/mutex.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/mutex.rs b/src/types/mutex.rs index 13aec60ddda..2d9b146c760 100644 --- a/src/types/mutex.rs +++ b/src/types/mutex.rs @@ -6,7 +6,7 @@ use std::ops::{Deref, DerefMut}; /// Comapred with `std::sync::Mutex` or `parking_lot::Mutex`, this is a very /// stripped-down locking primitive that only supports blocking lock and unlock /// operations. - +/// /// `PyMutex` is hooked into CPython's garbage collector and the GIL in GIL-enabled /// builds. If a thread is blocked on aquiring the mutex and holds the GIL or would /// prevent Python from entering garbage collection, then Python will release the