From 2b364d319059923f0d94cc705f35e77680c55ccb Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 22 Nov 2024 16:31:22 +0000 Subject: [PATCH 01/11] support 3.13t free-threaded python --- .github/workflows/ci.yml | 3 ++- CHANGELOG.md | 1 + src/datetime.rs | 12 ++++++------ src/strings.rs | 11 +++++------ 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f4e801413..f6c84b891 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,7 @@ jobs: "3.11", "3.12", "3.13", + "3.13t", "pypy-3.9", "pypy-3.10", ] @@ -108,7 +109,7 @@ jobs: steps: - uses: actions/checkout@v4 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v5 + uses: Quansight-Labs/setup-python@v5 with: python-version: ${{ matrix.python-version }} architecture: ${{ matrix.platform.python-architecture }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b748cf6f..65fd124a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - removed the `gil-refs` feature - reintroduced function names without `_bound` suffix + deprecating the old names - switched to `IntoPyObject` as trait bound + - Support Python 3.13t "free-threaded" Python. ([#471](https://github.com/PyO3/rust-numpy/pull/471) - v0.22.1 - Fix building on 32-bit Windows. ([#463](https://github.com/PyO3/rust-numpy/pull/463)) diff --git a/src/datetime.rs b/src/datetime.rs index 3eef346d8..b43a2e062 100644 --- a/src/datetime.rs +++ b/src/datetime.rs @@ -54,13 +54,13 @@ //! [scalars-datetime64]: https://numpy.org/doc/stable/reference/arrays.scalars.html#numpy.datetime64 //! [scalars-timedelta64]: https://numpy.org/doc/stable/reference/arrays.scalars.html#numpy.timedelta64 -use std::cell::RefCell; use std::collections::hash_map::Entry; use std::fmt; use std::hash::Hash; use std::marker::PhantomData; +use std::sync::Mutex; -use pyo3::{sync::GILProtected, Bound, Py, Python}; +use pyo3::{Bound, Py, Python}; use rustc_hash::FxHashMap; use crate::dtype::{clone_methods_impl, Element, PyArrayDescr, PyArrayDescrMethods}; @@ -209,8 +209,7 @@ impl fmt::Debug for Timedelta { struct TypeDescriptors { npy_type: NPY_TYPES, - #[allow(clippy::type_complexity)] - dtypes: GILProtected>>>>, + dtypes: Mutex>>>, } impl TypeDescriptors { @@ -218,13 +217,14 @@ impl TypeDescriptors { const unsafe fn new(npy_type: NPY_TYPES) -> Self { Self { npy_type, - dtypes: GILProtected::new(RefCell::new(None)), + dtypes: Mutex::new(None), } } #[allow(clippy::wrong_self_convention)] fn from_unit<'py>(&self, py: Python<'py>, unit: NPY_DATETIMEUNIT) -> Bound<'py, PyArrayDescr> { - let mut dtypes = self.dtypes.get(py).borrow_mut(); + // FIXME probably a deadlock risk here due to the GIL? Might need MutexExt trait in PyO3 + let mut dtypes = self.dtypes.lock().expect("dtype cache poisoned"); let dtype = match dtypes.get_or_insert_with(Default::default).entry(unit) { Entry::Occupied(entry) => entry.into_mut(), diff --git a/src/strings.rs b/src/strings.rs index 067327865..e2a05b95e 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -3,16 +3,15 @@ //! [ascii]: https://numpy.org/doc/stable/reference/c-api/dtype.html#c.NPY_STRING //! [ucs4]: https://numpy.org/doc/stable/reference/c-api/dtype.html#c.NPY_UNICODE -use std::cell::RefCell; use std::collections::hash_map::Entry; use std::fmt; use std::mem::size_of; use std::os::raw::c_char; use std::str; +use std::sync::Mutex; use pyo3::{ ffi::{Py_UCS1, Py_UCS4}, - sync::GILProtected, Bound, Py, Python, }; use rustc_hash::FxHashMap; @@ -160,14 +159,13 @@ unsafe impl Element for PyFixedUnicode { } struct TypeDescriptors { - #[allow(clippy::type_complexity)] - dtypes: GILProtected>>>>, + dtypes: Mutex>>>, } impl TypeDescriptors { const fn new() -> Self { Self { - dtypes: GILProtected::new(RefCell::new(None)), + dtypes: Mutex::new(None), } } @@ -180,7 +178,8 @@ impl TypeDescriptors { byteorder: c_char, size: usize, ) -> Bound<'py, PyArrayDescr> { - let mut dtypes = self.dtypes.get(py).borrow_mut(); + // FIXME probably a deadlock risk here due to the GIL? Might need MutexExt trait in PyO3 + let mut dtypes = self.dtypes.lock().expect("dtype cache poisoned"); let dtype = match dtypes.get_or_insert_with(Default::default).entry(size) { Entry::Occupied(entry) => entry.into_mut(), From 28d1eddcd41fec1c778136f9d6cf9996e11fde2f Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Sun, 24 Nov 2024 15:54:29 +0100 Subject: [PATCH 02/11] make dynamic borrow-checking threadsafe --- Cargo.toml | 4 +++ build.rs | 3 +++ src/borrow/shared.rs | 59 +++++++++++++++++++++++++------------------- 3 files changed, 41 insertions(+), 25 deletions(-) create mode 100644 build.rs diff --git a/Cargo.toml b/Cargo.toml index 90c74f452..085f7ef3b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,10 +24,14 @@ num-traits = "0.2" ndarray = ">= 0.15, < 0.17" pyo3 = { version = "0.23.0", default-features = false, features = ["macros"] } rustc-hash = "1.1" +parking_lot = "0.12.3" [dev-dependencies] pyo3 = { version = "0.23.0", default-features = false, features = ["auto-initialize"] } nalgebra = { version = ">=0.30, <0.34", default-features = false, features = ["std"] } +[build-dependencies] +pyo3-build-config = { version = "0.23.1", features = ["resolve-config"] } + [package.metadata.docs.rs] all-features = true diff --git a/build.rs b/build.rs new file mode 100644 index 000000000..6b746f1a1 --- /dev/null +++ b/build.rs @@ -0,0 +1,3 @@ +fn main() { + pyo3_build_config::use_pyo3_cfgs(); +} diff --git a/src/borrow/shared.rs b/src/borrow/shared.rs index 36860b275..d93632709 100644 --- a/src/borrow/shared.rs +++ b/src/borrow/shared.rs @@ -13,6 +13,7 @@ use crate::array::get_array_module; use crate::cold; use crate::error::BorrowError; use crate::npyffi::{PyArrayObject, PyArray_Check, PyDataType_ELSIZE, NPY_ARRAY_WRITEABLE}; +use parking_lot::Mutex; /// Defines the shared C API used for borrow checking /// @@ -45,7 +46,7 @@ unsafe impl Send for Shared {} unsafe extern "C" fn acquire_shared(flags: *mut c_void, array: *mut PyArrayObject) -> c_int { // SAFETY: GIL must be held when calling `acquire_shared`. let py = Python::assume_gil_acquired(); - let flags = &mut *(flags as *mut BorrowFlags); + let flags = &*(flags as *mut BorrowFlags); let address = base_address(py, array); let key = borrow_key(py, array); @@ -63,7 +64,7 @@ unsafe extern "C" fn acquire_mut_shared(flags: *mut c_void, array: *mut PyArrayO // SAFETY: GIL must be held when calling `acquire_shared`. let py = Python::assume_gil_acquired(); - let flags = &mut *(flags as *mut BorrowFlags); + let flags = &*(flags as *mut BorrowFlags); let address = base_address(py, array); let key = borrow_key(py, array); @@ -77,8 +78,7 @@ unsafe extern "C" fn acquire_mut_shared(flags: *mut c_void, array: *mut PyArrayO unsafe extern "C" fn release_shared(flags: *mut c_void, array: *mut PyArrayObject) { // SAFETY: GIL must be held when calling `acquire_shared`. let py = Python::assume_gil_acquired(); - let flags = &mut *(flags as *mut BorrowFlags); - + let flags = &*(flags as *mut BorrowFlags); let address = base_address(py, array); let key = borrow_key(py, array); @@ -88,7 +88,7 @@ unsafe extern "C" fn release_shared(flags: *mut c_void, array: *mut PyArrayObjec unsafe extern "C" fn release_mut_shared(flags: *mut c_void, array: *mut PyArrayObject) { // SAFETY: GIL must be held when calling `acquire_shared`. let py = Python::assume_gil_acquired(); - let flags = &mut *(flags as *mut BorrowFlags); + let flags = &*(flags as *mut BorrowFlags); let address = base_address(py, array); let key = borrow_key(py, array); @@ -250,14 +250,14 @@ impl BorrowKey { } } -type BorrowFlagsInner = FxHashMap<*mut c_void, FxHashMap>; +type BorrowFlagsInner = Mutex>>; #[derive(Default)] struct BorrowFlags(BorrowFlagsInner); impl BorrowFlags { - fn acquire(&mut self, address: *mut c_void, key: BorrowKey) -> Result<(), ()> { - let borrow_flags = &mut self.0; + fn acquire(&self, address: *mut c_void, key: BorrowKey) -> Result<(), ()> { + let mut borrow_flags = self.0.lock(); match borrow_flags.entry(address) { Entry::Occupied(entry) => { @@ -298,11 +298,10 @@ impl BorrowFlags { Ok(()) } - fn release(&mut self, address: *mut c_void, key: BorrowKey) { - let borrow_flags = &mut self.0; + fn release(&self, address: *mut c_void, key: BorrowKey) { + let mut borrow_flags = self.0.lock(); let same_base_arrays = borrow_flags.get_mut(&address).unwrap(); - let readers = same_base_arrays.get_mut(&key).unwrap(); *readers -= 1; @@ -316,8 +315,8 @@ impl BorrowFlags { } } - fn acquire_mut(&mut self, address: *mut c_void, key: BorrowKey) -> Result<(), ()> { - let borrow_flags = &mut self.0; + fn acquire_mut(&self, address: *mut c_void, key: BorrowKey) -> Result<(), ()> { + let mut borrow_flags = self.0.lock(); match borrow_flags.entry(address) { Entry::Occupied(entry) => { @@ -352,8 +351,8 @@ impl BorrowFlags { Ok(()) } - fn release_mut(&mut self, address: *mut c_void, key: BorrowKey) { - let borrow_flags = &mut self.0; + fn release_mut(&self, address: *mut c_void, key: BorrowKey) { + let mut borrow_flags = self.0.lock(); let same_base_arrays = borrow_flags.get_mut(&address).unwrap(); @@ -782,7 +781,8 @@ mod tests { let _exclusive1 = array1.readwrite(); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base1]; @@ -796,7 +796,8 @@ mod tests { let _shared2 = array2.readonly(); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 2); let same_base_arrays = &borrow_flags[&base1]; @@ -832,7 +833,8 @@ mod tests { let exclusive1 = view1.readwrite(); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base]; @@ -852,7 +854,8 @@ mod tests { let shared2 = view2.readonly(); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base]; @@ -875,7 +878,8 @@ mod tests { let shared3 = view3.readonly(); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base]; @@ -901,7 +905,8 @@ mod tests { let shared4 = view4.readonly(); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base]; @@ -923,7 +928,8 @@ mod tests { drop(shared2); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base]; @@ -945,7 +951,8 @@ mod tests { drop(shared3); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base]; @@ -965,7 +972,8 @@ mod tests { drop(exclusive1); { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); + #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); let same_base_arrays = &borrow_flags[&base]; @@ -983,8 +991,9 @@ mod tests { drop(shared4); + #[cfg(not(Py_GIL_DISABLED))] { - let borrow_flags = get_borrow_flags(py); + let borrow_flags = get_borrow_flags(py).lock(); assert_eq!(borrow_flags.len(), 0); } }); From 2b9ccf2ebd125495f136068fe0619b43cc17dc18 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Wed, 12 Feb 2025 12:57:45 -0700 Subject: [PATCH 03/11] fix compiler error in benches target --- benches/array.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/benches/array.rs b/benches/array.rs index 3df2321bb..756a352bd 100644 --- a/benches/array.rs +++ b/benches/array.rs @@ -6,7 +6,7 @@ use test::{black_box, Bencher}; use std::ops::Range; use numpy::{PyArray1, PyArray2, PyArray3}; -use pyo3::{types::PyAnyMethods, Bound, Python}; +use pyo3::{types::PyAnyMethods, Bound, IntoPyObjectExt, Python}; #[bench] fn extract_success(bencher: &mut Bencher) { @@ -115,7 +115,11 @@ fn from_slice_large(bencher: &mut Bencher) { } fn from_object_slice(bencher: &mut Bencher, size: usize) { - let vec = Python::with_gil(|py| (0..size).map(|val| val.to_object(py)).collect::>()); + let vec = Python::with_gil(|py| { + (0..size) + .map(|val| val.into_py_any(py).unwrap()) + .collect::>() + }); Python::with_gil(|py| { bencher.iter(|| { From ee9066c69a57844a93cc2735ea7aab6e293715c4 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Wed, 12 Feb 2025 12:57:57 -0700 Subject: [PATCH 04/11] fix warning on nightly rust about extern usage --- src/npyffi/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/npyffi/mod.rs b/src/npyffi/mod.rs index b96e69344..bf846f8e2 100644 --- a/src/npyffi/mod.rs +++ b/src/npyffi/mod.rs @@ -53,7 +53,7 @@ macro_rules! impl_api { [$offset: expr; $fname: ident ($($arg: ident: $t: ty),* $(,)?) $(-> $ret: ty)?] => { #[allow(non_snake_case)] pub unsafe fn $fname<'py>(&self, py: Python<'py>, $($arg : $t), *) $(-> $ret)* { - let fptr = self.get(py, $offset) as *const extern fn ($($arg : $t), *) $(-> $ret)*; + let fptr = self.get(py, $offset) as *const extern "C" fn ($($arg : $t), *) $(-> $ret)*; (*fptr)($($arg), *) } }; @@ -69,7 +69,7 @@ macro_rules! impl_api { API_VERSION_2_0, *API_VERSION.get(py).expect("API_VERSION is initialized"), ); - let fptr = self.get(py, $offset) as *const extern fn ($($arg: $t), *) $(-> $ret)*; + let fptr = self.get(py, $offset) as *const extern "C" fn ($($arg: $t), *) $(-> $ret)*; (*fptr)($($arg), *) } @@ -84,7 +84,7 @@ macro_rules! impl_api { API_VERSION_2_0, *API_VERSION.get(py).expect("API_VERSION is initialized"), ); - let fptr = self.get(py, $offset) as *const extern fn ($($arg: $t), *) $(-> $ret)*; + let fptr = self.get(py, $offset) as *const extern "C" fn ($($arg: $t), *) $(-> $ret)*; (*fptr)($($arg), *) } From 177927c51057e2a647dd7f2a939ba2b9cfcfdf88 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 13 Feb 2025 14:55:21 -0700 Subject: [PATCH 05/11] remove parking_lot dependency --- Cargo.toml | 1 - src/borrow/shared.rs | 33 +++++++++++++++------------------ 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8269ce2d4..5054b0082 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,6 @@ num-traits = "0.2" ndarray = ">= 0.15, < 0.17" pyo3 = { version = "0.23.4", default-features = false, features = ["macros"] } rustc-hash = "2.0" -parking_lot = "0.12.3" [dev-dependencies] pyo3 = { version = "0.23.3", default-features = false, features = ["auto-initialize"] } diff --git a/src/borrow/shared.rs b/src/borrow/shared.rs index d93632709..52b3c70f5 100644 --- a/src/borrow/shared.rs +++ b/src/borrow/shared.rs @@ -3,6 +3,7 @@ use std::ffi::{c_void, CString}; use std::mem::forget; use std::os::raw::{c_char, c_int}; use std::slice::from_raw_parts; +use std::sync::Mutex; use num_integer::gcd; use pyo3::types::{PyAnyMethods, PyCapsuleMethods}; @@ -13,16 +14,12 @@ use crate::array::get_array_module; use crate::cold; use crate::error::BorrowError; use crate::npyffi::{PyArrayObject, PyArray_Check, PyDataType_ELSIZE, NPY_ARRAY_WRITEABLE}; -use parking_lot::Mutex; /// Defines the shared C API used for borrow checking /// /// This structure will be placed into a capsule at /// `numpy.core.multiarray._RUST_NUMPY_BORROW_CHECKING_API`. /// -/// All functions exposed here assume the GIL is held -/// while they are called. -/// /// Versions are assumed to be backwards-compatible, i.e. /// an extension which knows version N will work using /// any API version M as long as M >= N holds. @@ -257,7 +254,7 @@ struct BorrowFlags(BorrowFlagsInner); impl BorrowFlags { fn acquire(&self, address: *mut c_void, key: BorrowKey) -> Result<(), ()> { - let mut borrow_flags = self.0.lock(); + let mut borrow_flags = self.0.lock().unwrap(); match borrow_flags.entry(address) { Entry::Occupied(entry) => { @@ -299,7 +296,7 @@ impl BorrowFlags { } fn release(&self, address: *mut c_void, key: BorrowKey) { - let mut borrow_flags = self.0.lock(); + let mut borrow_flags = self.0.lock().unwrap(); let same_base_arrays = borrow_flags.get_mut(&address).unwrap(); let readers = same_base_arrays.get_mut(&key).unwrap(); @@ -316,7 +313,7 @@ impl BorrowFlags { } fn acquire_mut(&self, address: *mut c_void, key: BorrowKey) -> Result<(), ()> { - let mut borrow_flags = self.0.lock(); + let mut borrow_flags = self.0.lock().unwrap(); match borrow_flags.entry(address) { Entry::Occupied(entry) => { @@ -352,7 +349,7 @@ impl BorrowFlags { } fn release_mut(&self, address: *mut c_void, key: BorrowKey) { - let mut borrow_flags = self.0.lock(); + let mut borrow_flags = self.0.lock().unwrap(); let same_base_arrays = borrow_flags.get_mut(&address).unwrap(); @@ -781,7 +778,7 @@ mod tests { let _exclusive1 = array1.readwrite(); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -796,7 +793,7 @@ mod tests { let _shared2 = array2.readonly(); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 2); @@ -833,7 +830,7 @@ mod tests { let exclusive1 = view1.readwrite(); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -854,7 +851,7 @@ mod tests { let shared2 = view2.readonly(); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -878,7 +875,7 @@ mod tests { let shared3 = view3.readonly(); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -905,7 +902,7 @@ mod tests { let shared4 = view4.readonly(); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -928,7 +925,7 @@ mod tests { drop(shared2); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -951,7 +948,7 @@ mod tests { drop(shared3); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -972,7 +969,7 @@ mod tests { drop(exclusive1); { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); #[cfg(not(Py_GIL_DISABLED))] assert_eq!(borrow_flags.len(), 1); @@ -993,7 +990,7 @@ mod tests { #[cfg(not(Py_GIL_DISABLED))] { - let borrow_flags = get_borrow_flags(py).lock(); + let borrow_flags = get_borrow_flags(py).lock().unwrap(); assert_eq!(borrow_flags.len(), 0); } }); From cc109bfa71f2fd7b2034894f888bbb48ad425d68 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 13 Feb 2025 15:08:21 -0700 Subject: [PATCH 06/11] Add deadlock-avoidance using direct FFI calls --- src/strings.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/strings.rs b/src/strings.rs index e2a05b95e..844c3f7b1 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -178,9 +178,24 @@ impl TypeDescriptors { byteorder: c_char, size: usize, ) -> Bound<'py, PyArrayDescr> { - // FIXME probably a deadlock risk here due to the GIL? Might need MutexExt trait in PyO3 + // FIXME create a proper MutexExt trait that handles poisoning and upstream to PyO3 + struct Guard(*mut pyo3::ffi::PyThreadState); + + impl Drop for Guard { + fn drop(&mut self) { + unsafe { pyo3::ffi::PyEval_RestoreThread(self.0) }; + } + } + + // we are currently attached to the python runtime and we might block trying to acquire + // the dtype cache mutex, so detach to avoid a possible deadlock. + let ts_guard = Guard(unsafe { pyo3::ffi::PyEval_SaveThread() }); + let mut dtypes = self.dtypes.lock().expect("dtype cache poisoned"); + // we've acquire the dtype cache lock so it's safe to re-attach to the runtime + drop(ts_guard); + let dtype = match dtypes.get_or_insert_with(Default::default).entry(size) { Entry::Occupied(entry) => entry.into_mut(), Entry::Vacant(entry) => { From e58c0afc31a669d6c975a6929fde198d17fef3c8 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 17 Feb 2025 14:51:56 -0700 Subject: [PATCH 07/11] refactor BorrowFlags tests to not hold a lock in the tests --- src/borrow/shared.rs | 209 +++++++++++++++++++++---------------------- 1 file changed, 102 insertions(+), 107 deletions(-) diff --git a/src/borrow/shared.rs b/src/borrow/shared.rs index 52b3c70f5..ffffa0d56 100644 --- a/src/borrow/shared.rs +++ b/src/borrow/shared.rs @@ -247,15 +247,14 @@ impl BorrowKey { } } -type BorrowFlagsInner = Mutex>>; +type BorrowFlagsInner = FxHashMap<*mut c_void, FxHashMap>; #[derive(Default)] -struct BorrowFlags(BorrowFlagsInner); +struct BorrowFlags(Mutex); impl BorrowFlags { fn acquire(&self, address: *mut c_void, key: BorrowKey) -> Result<(), ()> { let mut borrow_flags = self.0.lock().unwrap(); - match borrow_flags.entry(address) { Entry::Occupied(entry) => { let same_base_arrays = entry.into_mut(); @@ -448,10 +447,27 @@ mod tests { use crate::untyped_array::PyUntypedArrayMethods; use pyo3::ffi::c_str; - fn get_borrow_flags<'py>(py: Python<'py>) -> &'py BorrowFlagsInner { + struct BorrowFlagsState(usize, usize, Option); + + fn get_borrow_flags_state<'py>( + py: Python<'py>, + base: *mut c_void, + key: &BorrowKey, + ) -> BorrowFlagsState { let shared = get_or_insert_shared(py).unwrap(); assert_eq!(shared.version, 1); - unsafe { &(*(shared.flags as *mut BorrowFlags)).0 } + let inner = unsafe { &(*(shared.flags as *mut BorrowFlags)).0 } + .lock() + .unwrap(); + if let Some(base_arrays) = inner.get(&base) { + BorrowFlagsState( + inner.len(), + base_arrays.len(), + base_arrays.get(key).map(|x| *x), + ) + } else { + BorrowFlagsState(0, 0, None) + } } #[test] @@ -778,36 +794,30 @@ mod tests { let _exclusive1 = array1.readwrite(); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base1, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); - - let same_base_arrays = &borrow_flags[&base1]; - assert_eq!(same_base_arrays.len(), 1); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); + assert_eq!(state.1, 1); + assert_eq!(state.2, Some(-1)); } let key2 = borrow_key(py, array2.as_array_ptr()); let _shared2 = array2.readonly(); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base1, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 2); - - let same_base_arrays = &borrow_flags[&base1]; - assert_eq!(same_base_arrays.len(), 1); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 2); - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); + assert_eq!(state.1, 1); + assert_eq!(state.2, Some(-1)); - let same_base_arrays = &borrow_flags[&base2]; - assert_eq!(same_base_arrays.len(), 1); - - let flag = same_base_arrays[&key2]; - assert_eq!(flag, 1); + let state = get_borrow_flags_state(py, base2, &key2); + assert_eq!(state.1, 1); + assert_eq!(state.2, Some(1)); } }); } @@ -830,15 +840,13 @@ mod tests { let exclusive1 = view1.readwrite(); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); - #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); - - let same_base_arrays = &borrow_flags[&base]; - assert_eq!(same_base_arrays.len(), 1); + let state = get_borrow_flags_state(py, base, &key1); - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); + #[cfg(not(Py_GIL_DISABLED))] + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); + assert_eq!(state.1, 1); + assert_eq!(state.2, Some(-1)); } let view2 = py @@ -851,18 +859,15 @@ mod tests { let shared2 = view2.readonly(); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); - - let same_base_arrays = &borrow_flags[&base]; - assert_eq!(same_base_arrays.len(), 2); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); + assert_eq!(state.1, 2); + assert_eq!(state.2, Some(-1)); - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); - - let flag = same_base_arrays[&key2]; - assert_eq!(flag, 1); + let state = get_borrow_flags_state(py, base, &key2); + assert_eq!(state.2, Some(1)); } let view3 = py @@ -875,21 +880,18 @@ mod tests { let shared3 = view3.readonly(); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); - - let same_base_arrays = &borrow_flags[&base]; - assert_eq!(same_base_arrays.len(), 2); - - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); + assert_eq!(state.1, 2); + assert_eq!(state.2, Some(-1)); - let flag = same_base_arrays[&key2]; - assert_eq!(flag, 2); + let state = get_borrow_flags_state(py, base, &key2); + assert_eq!(state.2, Some(2)); - let flag = same_base_arrays[&key3]; - assert_eq!(flag, 2); + let state = get_borrow_flags_state(py, base, &key3); + assert_eq!(state.2, Some(2)); } let view4 = py @@ -902,96 +904,89 @@ mod tests { let shared4 = view4.readonly(); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); + assert_eq!(state.1, 3); + assert_eq!(state.2, Some(-1)); - let same_base_arrays = &borrow_flags[&base]; - assert_eq!(same_base_arrays.len(), 3); + let state = get_borrow_flags_state(py, base, &key2); + assert_eq!(state.2, Some(2)); - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); + let state = get_borrow_flags_state(py, base, &key3); + assert_eq!(state.2, Some(2)); - let flag = same_base_arrays[&key2]; - assert_eq!(flag, 2); - - let flag = same_base_arrays[&key3]; - assert_eq!(flag, 2); - - let flag = same_base_arrays[&key4]; - assert_eq!(flag, 1); + let state = get_borrow_flags_state(py, base, &key4); + assert_eq!(state.2, Some(1)); } drop(shared2); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); - - let same_base_arrays = &borrow_flags[&base]; - assert_eq!(same_base_arrays.len(), 3); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); + assert_eq!(state.1, 3); + assert_eq!(state.2, Some(-1)); - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); + let state = get_borrow_flags_state(py, base, &key2); + assert_eq!(state.2, Some(1)); - let flag = same_base_arrays[&key2]; - assert_eq!(flag, 1); + let state = get_borrow_flags_state(py, base, &key3); + assert_eq!(state.2, Some(1)); - let flag = same_base_arrays[&key3]; - assert_eq!(flag, 1); - - let flag = same_base_arrays[&key4]; - assert_eq!(flag, 1); + let state = get_borrow_flags_state(py, base, &key4); + assert_eq!(state.2, Some(1)); } drop(shared3); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); - - let same_base_arrays = &borrow_flags[&base]; - assert_eq!(same_base_arrays.len(), 2); - - let flag = same_base_arrays[&key1]; - assert_eq!(flag, -1); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); + assert_eq!(state.1, 2); + assert_eq!(state.2, Some(-1)); - assert!(!same_base_arrays.contains_key(&key2)); + let state = get_borrow_flags_state(py, base, &key2); + assert_eq!(state.2, None); - assert!(!same_base_arrays.contains_key(&key3)); + let state = get_borrow_flags_state(py, base, &key3); + assert_eq!(state.2, None); - let flag = same_base_arrays[&key4]; - assert_eq!(flag, 1); + let state = get_borrow_flags_state(py, base, &key4); + assert_eq!(state.2, Some(1)); } drop(exclusive1); { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); + let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] - assert_eq!(borrow_flags.len(), 1); - - let same_base_arrays = &borrow_flags[&base]; - assert_eq!(same_base_arrays.len(), 1); - - assert!(!same_base_arrays.contains_key(&key1)); + // borrow checking state is shared and other tests might have registered a borrow + assert_eq!(state.0, 1); + assert_eq!(state.1, 1); + assert_eq!(state.2, None); - assert!(!same_base_arrays.contains_key(&key2)); + let state = get_borrow_flags_state(py, base, &key2); + assert_eq!(state.2, None); - assert!(!same_base_arrays.contains_key(&key3)); + let state = get_borrow_flags_state(py, base, &key3); + assert_eq!(state.2, None); - let flag = same_base_arrays[&key4]; - assert_eq!(flag, 1); + let state = get_borrow_flags_state(py, base, &key4); + assert_eq!(state.2, Some(1)); } drop(shared4); #[cfg(not(Py_GIL_DISABLED))] + // borrow checking state is shared and other tests might have registered a borrow { - let borrow_flags = get_borrow_flags(py).lock().unwrap(); - assert_eq!(borrow_flags.len(), 0); + assert_eq!(get_borrow_flags_state(py, base, &key1).0, 0); } }); } From e4ca231903c844b6856828ff2d27f5957120b7e6 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Mon, 17 Feb 2025 15:58:38 -0700 Subject: [PATCH 08/11] fix clippy --- src/borrow/shared.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/borrow/shared.rs b/src/borrow/shared.rs index ffffa0d56..8d74ae141 100644 --- a/src/borrow/shared.rs +++ b/src/borrow/shared.rs @@ -463,7 +463,7 @@ mod tests { BorrowFlagsState( inner.len(), base_arrays.len(), - base_arrays.get(key).map(|x| *x), + base_arrays.get(key).copied(), ) } else { BorrowFlagsState(0, 0, None) From 74bc5dd1fce61d76b5deac1c9848b14eabe5ff3c Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Tue, 18 Feb 2025 15:38:57 -0700 Subject: [PATCH 09/11] give BorrowFlagsState fields descriptive names --- src/borrow/shared.rs | 115 ++++++++++++++++++++++++------------------- 1 file changed, 63 insertions(+), 52 deletions(-) diff --git a/src/borrow/shared.rs b/src/borrow/shared.rs index 8d74ae141..939098c64 100644 --- a/src/borrow/shared.rs +++ b/src/borrow/shared.rs @@ -447,7 +447,12 @@ mod tests { use crate::untyped_array::PyUntypedArrayMethods; use pyo3::ffi::c_str; - struct BorrowFlagsState(usize, usize, Option); + struct BorrowFlagsState { + #[cfg(not(Py_GIL_DISABLED))] + n_flags: usize, + n_arrays: usize, + flag: Option, + } fn get_borrow_flags_state<'py>( py: Python<'py>, @@ -460,13 +465,19 @@ mod tests { .lock() .unwrap(); if let Some(base_arrays) = inner.get(&base) { - BorrowFlagsState( - inner.len(), - base_arrays.len(), - base_arrays.get(key).copied(), - ) + BorrowFlagsState { + #[cfg(not(Py_GIL_DISABLED))] + n_flags: inner.len(), + n_arrays: base_arrays.len(), + flag: base_arrays.get(key).copied(), + } } else { - BorrowFlagsState(0, 0, None) + BorrowFlagsState { + #[cfg(not(Py_GIL_DISABLED))] + n_flags: 0, + n_arrays: 0, + flag: None, + } } } @@ -797,10 +808,10 @@ mod tests { let state = get_borrow_flags_state(py, base1, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); + assert_eq!(state.n_flags, 1); - assert_eq!(state.1, 1); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_arrays, 1); + assert_eq!(state.flag, Some(-1)); } let key2 = borrow_key(py, array2.as_array_ptr()); @@ -810,14 +821,14 @@ mod tests { let state = get_borrow_flags_state(py, base1, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 2); + assert_eq!(state.n_flags, 2); - assert_eq!(state.1, 1); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_arrays, 1); + assert_eq!(state.flag, Some(-1)); let state = get_borrow_flags_state(py, base2, &key2); - assert_eq!(state.1, 1); - assert_eq!(state.2, Some(1)); + assert_eq!(state.n_arrays, 1); + assert_eq!(state.flag, Some(1)); } }); } @@ -844,9 +855,9 @@ mod tests { #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); - assert_eq!(state.1, 1); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_flags, 1); + assert_eq!(state.n_arrays, 1); + assert_eq!(state.flag, Some(-1)); } let view2 = py @@ -862,12 +873,12 @@ mod tests { let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); - assert_eq!(state.1, 2); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_flags, 1); + assert_eq!(state.n_arrays, 2); + assert_eq!(state.flag, Some(-1)); let state = get_borrow_flags_state(py, base, &key2); - assert_eq!(state.2, Some(1)); + assert_eq!(state.flag, Some(1)); } let view3 = py @@ -883,15 +894,15 @@ mod tests { let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); - assert_eq!(state.1, 2); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_flags, 1); + assert_eq!(state.n_arrays, 2); + assert_eq!(state.flag, Some(-1)); let state = get_borrow_flags_state(py, base, &key2); - assert_eq!(state.2, Some(2)); + assert_eq!(state.flag, Some(2)); let state = get_borrow_flags_state(py, base, &key3); - assert_eq!(state.2, Some(2)); + assert_eq!(state.flag, Some(2)); } let view4 = py @@ -907,18 +918,18 @@ mod tests { let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); - assert_eq!(state.1, 3); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_flags, 1); + assert_eq!(state.n_arrays, 3); + assert_eq!(state.flag, Some(-1)); let state = get_borrow_flags_state(py, base, &key2); - assert_eq!(state.2, Some(2)); + assert_eq!(state.flag, Some(2)); let state = get_borrow_flags_state(py, base, &key3); - assert_eq!(state.2, Some(2)); + assert_eq!(state.flag, Some(2)); let state = get_borrow_flags_state(py, base, &key4); - assert_eq!(state.2, Some(1)); + assert_eq!(state.flag, Some(1)); } drop(shared2); @@ -927,18 +938,18 @@ mod tests { let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); - assert_eq!(state.1, 3); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_flags, 1); + assert_eq!(state.n_arrays, 3); + assert_eq!(state.flag, Some(-1)); let state = get_borrow_flags_state(py, base, &key2); - assert_eq!(state.2, Some(1)); + assert_eq!(state.flag, Some(1)); let state = get_borrow_flags_state(py, base, &key3); - assert_eq!(state.2, Some(1)); + assert_eq!(state.flag, Some(1)); let state = get_borrow_flags_state(py, base, &key4); - assert_eq!(state.2, Some(1)); + assert_eq!(state.flag, Some(1)); } drop(shared3); @@ -947,18 +958,18 @@ mod tests { let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); - assert_eq!(state.1, 2); - assert_eq!(state.2, Some(-1)); + assert_eq!(state.n_flags, 1); + assert_eq!(state.n_arrays, 2); + assert_eq!(state.flag, Some(-1)); let state = get_borrow_flags_state(py, base, &key2); - assert_eq!(state.2, None); + assert_eq!(state.flag, None); let state = get_borrow_flags_state(py, base, &key3); - assert_eq!(state.2, None); + assert_eq!(state.flag, None); let state = get_borrow_flags_state(py, base, &key4); - assert_eq!(state.2, Some(1)); + assert_eq!(state.flag, Some(1)); } drop(exclusive1); @@ -967,18 +978,18 @@ mod tests { let state = get_borrow_flags_state(py, base, &key1); #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow - assert_eq!(state.0, 1); - assert_eq!(state.1, 1); - assert_eq!(state.2, None); + assert_eq!(state.n_flags, 1); + assert_eq!(state.n_arrays, 1); + assert_eq!(state.flag, None); let state = get_borrow_flags_state(py, base, &key2); - assert_eq!(state.2, None); + assert_eq!(state.flag, None); let state = get_borrow_flags_state(py, base, &key3); - assert_eq!(state.2, None); + assert_eq!(state.flag, None); let state = get_borrow_flags_state(py, base, &key4); - assert_eq!(state.2, Some(1)); + assert_eq!(state.flag, Some(1)); } drop(shared4); @@ -986,7 +997,7 @@ mod tests { #[cfg(not(Py_GIL_DISABLED))] // borrow checking state is shared and other tests might have registered a borrow { - assert_eq!(get_borrow_flags_state(py, base, &key1).0, 0); + assert_eq!(get_borrow_flags_state(py, base, &key1).n_flags, 0); } }); } From 5f4c90a74912837a55958b7c489243025752fbc4 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Wed, 19 Feb 2025 09:55:45 -0700 Subject: [PATCH 10/11] move thread state guard into the crate root --- src/lib.rs | 17 +++++++++++++++++ src/strings.rs | 17 ++++------------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e147a9c18..c5d608ffa 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -149,6 +149,23 @@ mod doctest { #[inline(always)] fn cold() {} +/// An RAII guard for avoiding deadlocks with the GIL or other global +/// synchronization events in the Python runtime +// FIXME create a proper MutexExt trait that handles poisoning and upstream to PyO3 +struct ThreadStateGuard(*mut pyo3::ffi::PyThreadState); + +impl ThreadStateGuard { + fn new() -> ThreadStateGuard { + ThreadStateGuard(unsafe { pyo3::ffi::PyEval_SaveThread() }) + } +} + +impl Drop for ThreadStateGuard { + fn drop(&mut self) { + unsafe { pyo3::ffi::PyEval_RestoreThread(self.0) }; + } +} + /// Create a [`PyArray`] with one, two or three dimensions. /// /// This macro is backed by [`ndarray::array`]. diff --git a/src/strings.rs b/src/strings.rs index 844c3f7b1..8826e674a 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -19,6 +19,7 @@ use rustc_hash::FxHashMap; use crate::dtype::{clone_methods_impl, Element, PyArrayDescr, PyArrayDescrMethods}; use crate::npyffi::PyDataType_SET_ELSIZE; use crate::npyffi::NPY_TYPES; +use crate::ThreadStateGuard; /// A newtype wrapper around [`[u8; N]`][Py_UCS1] to handle [`byte` scalars][numpy-bytes] while satisfying coherence. /// @@ -178,22 +179,12 @@ impl TypeDescriptors { byteorder: c_char, size: usize, ) -> Bound<'py, PyArrayDescr> { - // FIXME create a proper MutexExt trait that handles poisoning and upstream to PyO3 - struct Guard(*mut pyo3::ffi::PyThreadState); - - impl Drop for Guard { - fn drop(&mut self) { - unsafe { pyo3::ffi::PyEval_RestoreThread(self.0) }; - } - } - - // we are currently attached to the python runtime and we might block trying to acquire - // the dtype cache mutex, so detach to avoid a possible deadlock. - let ts_guard = Guard(unsafe { pyo3::ffi::PyEval_SaveThread() }); + // Detach from the runtime to avoid deadlocking on acquiring the mutex. + let ts_guard = ThreadStateGuard::new(); let mut dtypes = self.dtypes.lock().expect("dtype cache poisoned"); - // we've acquire the dtype cache lock so it's safe to re-attach to the runtime + // Now we hold the mutex so it's safe to re-attach to the runtime. drop(ts_guard); let dtype = match dtypes.get_or_insert_with(Default::default).entry(size) { From 4ee024a66a1903bb007c227074d49e328e0b3f24 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Wed, 19 Feb 2025 09:56:17 -0700 Subject: [PATCH 11/11] use ThreadStateGuard to avoid deadlocks acquiring the dtype cache --- src/datetime.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/datetime.rs b/src/datetime.rs index b43a2e062..a65c562bb 100644 --- a/src/datetime.rs +++ b/src/datetime.rs @@ -67,6 +67,7 @@ use crate::dtype::{clone_methods_impl, Element, PyArrayDescr, PyArrayDescrMethod use crate::npyffi::{ PyArray_DatetimeDTypeMetaData, PyDataType_C_METADATA, NPY_DATETIMEUNIT, NPY_TYPES, }; +use crate::ThreadStateGuard; /// Represents the [datetime units][datetime-units] supported by NumPy /// @@ -223,9 +224,14 @@ impl TypeDescriptors { #[allow(clippy::wrong_self_convention)] fn from_unit<'py>(&self, py: Python<'py>, unit: NPY_DATETIMEUNIT) -> Bound<'py, PyArrayDescr> { - // FIXME probably a deadlock risk here due to the GIL? Might need MutexExt trait in PyO3 + // Detach from the runtime to avoid deadlocking on acquiring the mutex. + let ts_guard = ThreadStateGuard::new(); + let mut dtypes = self.dtypes.lock().expect("dtype cache poisoned"); + // Now we hold the mutex so it's safe to re-attach to the runtime. + drop(ts_guard); + let dtype = match dtypes.get_or_insert_with(Default::default).entry(unit) { Entry::Occupied(entry) => entry.into_mut(), Entry::Vacant(entry) => {