From e195f32b2ef029083cb09a3f1a36b3b74b9e484c Mon Sep 17 00:00:00 2001 From: person93 Date: Fri, 7 Aug 2026 03:17:34 -0400 Subject: [PATCH 01/10] add safety comments in sync.rs Just copied existing safety comment to more places --- src/sync.rs | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index 0ec45ff9c2b..ac06a279a03 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -1,6 +1,3 @@ -// TODO https://github.com/PyO3/pyo3/issues/5487 -#![allow(clippy::undocumented_unsafe_blocks)] - //! Synchronization mechanisms which are aware of the existence of the Python interpreter. //! //! The Python interpreter has multiple "stop the world" situations which may block threads, such as @@ -367,6 +364,9 @@ impl OnceExt for parking_lot::Once { return; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; self.call_once(move || { @@ -384,6 +384,9 @@ impl OnceExt for parking_lot::Once { return; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; self.call_once_force(move |state| { @@ -447,6 +450,9 @@ impl MutexExt for lock_api::Mutex { return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.lock(); drop(ts_guard); @@ -469,6 +475,9 @@ where return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.lock_arc(); drop(ts_guard); @@ -492,6 +501,9 @@ where return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.lock(); drop(ts_guard); @@ -515,6 +527,9 @@ where return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.lock_arc(); drop(ts_guard); @@ -597,6 +612,9 @@ impl RwLockExt for lock_api::RwLock { return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.read(); drop(ts_guard); @@ -608,6 +626,9 @@ impl RwLockExt for lock_api::RwLock { return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.write(); drop(ts_guard); @@ -635,6 +656,9 @@ where return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.read_arc(); drop(ts_guard); @@ -646,6 +670,9 @@ where return guard; } + // SAFETY: detach from the runtime right before a possibly blocking call + // then reattach when the blocking call completes and before calling + // into the C API. let ts_guard = unsafe { SuspendAttach::new() }; let res = self.write_arc(); drop(ts_guard); From 01aeff1ae4b5882ca5f38ae3a0dc0b0bbf9aaab1 Mon Sep 17 00:00:00 2001 From: person93 Date: Fri, 7 Aug 2026 03:26:35 -0400 Subject: [PATCH 02/10] safety comments for type_object.rs --- src/type_object.rs | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/type_object.rs b/src/type_object.rs index 017c9c2028f..a8df18ff90a 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -1,6 +1,3 @@ -// TODO https://github.com/PyO3/pyo3/issues/5487 -#![allow(clippy::undocumented_unsafe_blocks)] - //! Python type object information use crate::ffi_ptr_ext::FfiPtrExt; @@ -74,30 +71,27 @@ pub unsafe trait PyTypeInfo: Sized { // the type object to be freed. // // By making `Bound` we assume ownership which is then safe against races. - unsafe { - Self::type_object_raw(py) - .cast::() - .assume_borrowed_unchecked(py) - .to_owned() - .cast_into_unchecked() - } + let tp = Self::type_object_raw(py).cast::(); + // SAFETY: the pointer is known to be a borrowed type object and we immeditely make a new reference + unsafe { tp.assume_borrowed_unchecked(py).cast_unchecked() }.to_owned() } /// Checks if `object` is an instance of this type or a subclass of this type. #[inline] fn is_type_of(object: &Bound<'_, PyAny>) -> bool { - unsafe { ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object_raw(object.py())) != 0 } + let tp = Self::type_object_raw(object.py()); + // SAFETY: pointers are known to be correct types and borrowed + (unsafe { ffi::PyObject_TypeCheck(object.as_ptr(), tp) }) != 0 } /// Checks if `object` is an instance of this type. #[inline] fn is_exact_type_of(object: &Bound<'_, PyAny>) -> bool { - unsafe { - ptr::eq( - ffi::Py_TYPE(object.as_ptr()), - Self::type_object_raw(object.py()), - ) - } + ptr::eq( + // SAFETY: no additional requirements + unsafe { ffi::Py_TYPE(object.as_ptr()) }, + Self::type_object_raw(object.py()), + ) } } @@ -124,6 +118,7 @@ pub unsafe trait PyTypeCheck { fn classinfo_object(py: Python<'_>) -> Bound<'_, PyAny>; } +// SAFETY: requirements upheld by impl of PyTypeInfo unsafe impl PyTypeCheck for T where T: PyTypeInfo, From d40f168de6dc6a5e8f26a765096ef71c34f8c5fc Mon Sep 17 00:00:00 2001 From: person93 Date: Fri, 7 Aug 2026 04:00:24 -0400 Subject: [PATCH 03/10] add safety comments to internal/state.rs --- src/internal/state.rs | 123 ++++++++++++++++++++++++++++++------------ 1 file changed, 89 insertions(+), 34 deletions(-) diff --git a/src/internal/state.rs b/src/internal/state.rs index 4a711c23bcd..41038f5df5b 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -1,6 +1,3 @@ -// TODO https://github.com/PyO3/pyo3/issues/5487 -#![allow(clippy::undocumented_unsafe_blocks)] - //! Interaction with attachment of the current thread to the Python interpreter. #[cfg(pyo3_disable_reference_pool)] @@ -76,6 +73,7 @@ impl AttachGuard { Err(AttachError::NotInitialized) => { // try to initialize the interpreter and try again crate::interpreter_lifecycle::ensure_initialized(); + // SAFETY: just initialized the interpreter unsafe { Self::do_attach_unchecked() } } #[cfg(Py_3_13)] @@ -138,13 +136,17 @@ impl AttachGuard { /// for a thread to be able to attach to it. pub(crate) unsafe fn attach_unchecked() -> Self { if thread_is_attached() { + // SAFETY: just confirmed that current thread is attached return unsafe { Self::assume() }; } + // SAFETY: requirements upheld by caller unsafe { Self::do_attach_unchecked() } } /// Attach to the interpreter, without a fast-path to check if the thread is already attached. + /// # Safety + /// The interpreter must be sufficiently initialized ot attach a thread. #[cold] unsafe fn do_attach_unchecked() -> Self { // SAFETY: interpreter is sufficiently initialized to attach a thread. @@ -157,6 +159,9 @@ impl AttachGuard { /// Acquires the `AttachGuard` while assuming that the thread is already attached /// to the interpreter. + /// + /// # Safety + /// Current thread must already be attached to the interpreter. pub(crate) unsafe fn assume() -> Self { increment_attach_count(); // SAFETY: invariant of calling this function @@ -177,17 +182,57 @@ impl Drop for AttachGuard { fn drop(&mut self) { match self { AttachGuard::Assumed => {} - AttachGuard::Ensured { gstate } => unsafe { - // Drop the objects in the pool before attempting to release the thread state - ffi::PyGILState_Release(*gstate); - }, + AttachGuard::Ensured { gstate } => { + // SAFETY: matching call to ensure in constructor + unsafe { + // Drop the objects in the pool before attempting to release the thread state + ffi::PyGILState_Release(*gstate); + } + } } decrement_attach_count(); } } #[cfg(not(pyo3_disable_reference_pool))] -type PyObjVec = Vec>; +use self::pending_decref::PendingDecref; + +// NOTE: this is its own mod so that it can fully contain its unsafe assumptions +#[cfg(not(pyo3_disable_reference_pool))] +mod pending_decref { + use crate::ffi; + use crate::marker::Python; + use core::ptr::NonNull; + + #[repr(transparent)] + pub(super) struct PendingDecref(NonNull); + + // SAFETY: it's a python object + unsafe impl Send for PendingDecref {} + // SAFETY: it's a python object + unsafe impl Sync for PendingDecref {} + + impl PendingDecref { + /// # Safety + /// `obj` must point to a valid [`ffi::PyObject`] and it must not be used again after this call. + pub(super) unsafe fn new(obj: NonNull) -> Self { + Self(obj) + } + + pub(super) fn decref(self, _py: Python<'_>) { + // SAFETY: requirements upheld by constructor + unsafe { ffi::Py_DECREF(self.0.as_ptr()) }; + } + + #[cfg(test)] + pub(super) fn as_raw(&self) -> NonNull { + self.0 + } + } +} + +#[cfg(not(pyo3_disable_reference_pool))] +type PyObjVec = Vec; #[cfg(not(pyo3_disable_reference_pool))] /// Thread-safe storage for objects which were dec_ref while not attached. @@ -208,9 +253,12 @@ impl ReferencePool { } } - fn register_decref(&self, obj: NonNull) { - self.pending_decrefs.lock().unwrap().push(obj); - self.dirty.store(true, Ordering::Relaxed); + /// # Safety + /// `obj` must be a valid python object and it must not be used again after this call + unsafe fn register_decref(&self, obj: NonNull) { + // SAFETY: requirements upheld by caller + let pending = unsafe { PendingDecref::new(obj) }; + self.pending_decrefs.lock().unwrap().push(pending); } fn drop_deferred_references(&self, py: Python<'_>) { @@ -226,7 +274,7 @@ impl ReferencePool { } #[cold] - fn drop_deferred_references_slow(&self, _py: Python<'_>) { + fn drop_deferred_references_slow(&self, py: Python<'_>) { // Compare and swap the dirty flag to false avoids multiple threads from having // contention on the mutex. if self @@ -249,17 +297,11 @@ impl ReferencePool { drop(pending_decrefs); for ptr in decrefs { - unsafe { ffi::Py_DECREF(ptr.as_ptr()) }; + ptr.decref(py); } } } -#[cfg(not(pyo3_disable_reference_pool))] -unsafe impl Send for ReferencePool {} - -#[cfg(not(pyo3_disable_reference_pool))] -unsafe impl Sync for ReferencePool {} - #[cfg(not(pyo3_disable_reference_pool))] static POOL: OnceLock = OnceLock::new(); @@ -284,8 +326,11 @@ pub(crate) struct SuspendAttach { } impl SuspendAttach { + /// # Safety + /// Current thread must be attached pub(crate) unsafe fn new() -> Self { let count = ATTACH_COUNT.with(|c| c.replace(0)); + // SAFETY: caller uphold requirements let tstate = unsafe { ffi::PyEval_SaveThread() }; Self { count, tstate } @@ -295,14 +340,14 @@ impl SuspendAttach { impl Drop for SuspendAttach { fn drop(&mut self) { ATTACH_COUNT.with(|c| c.set(self.count)); - unsafe { - ffi::PyEval_RestoreThread(self.tstate); - - // Update counts of `Py` that were dropped while not attached. - #[cfg(not(pyo3_disable_reference_pool))] - if let Some(pool) = POOL.get() { - pool.drop_deferred_references(Python::assume_attached()); - } + // SAFETY: tstate come from call to PyEval_SaveThread and it was not re-attached yet + unsafe { ffi::PyEval_RestoreThread(self.tstate) }; + // Update counts of `Py` that were dropped while not attached. + #[cfg(not(pyo3_disable_reference_pool))] + if let Some(pool) = POOL.get() { + // SAFETY: just re-attached + let py = unsafe { Python::assume_attached() }; + pool.drop_deferred_references(py); } } } @@ -354,7 +399,8 @@ impl Drop for ForbidAttaching { pub unsafe fn register_decref(obj: NonNull) { #[cfg(not(pyo3_disable_reference_pool))] { - get_pool().register_decref(obj); + // SAFETY: caller upholds requirements + unsafe { get_pool().register_decref(obj) }; } #[cfg(all( pyo3_disable_reference_pool, @@ -401,6 +447,7 @@ fn decrement_attach_count() { }); } +#[allow(clippy::undocumented_unsafe_blocks, reason = "tests")] #[cfg(test)] mod tests { use super::*; @@ -418,7 +465,10 @@ mod tests { .pending_decrefs .lock() .unwrap() - .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) + .iter() + .any(|pending| { + pending.as_raw() == (unsafe { NonNull::new_unchecked(obj.as_ptr()) }) + }) { return true; } @@ -439,7 +489,8 @@ mod tests { .pending_decrefs .lock() .unwrap() - .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) + .iter() + .any(|pending| pending.as_raw() == unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } #[test] @@ -590,7 +641,7 @@ mod tests { let ptr = Python::attach(|py| obj.clone_ref(py).into_ptr()); // A decref registered while detached applies once an attach drains the pool. - get_pool().register_decref(NonNull::new(ptr).unwrap()); + unsafe { get_pool().register_decref(NonNull::new(ptr).unwrap()) }; Python::attach(|_| { assert!(pool_dec_refs_does_not_contain(&obj)); @@ -629,7 +680,7 @@ mod tests { let capsule = unsafe { ffi::PyCapsule_New(ptr as _, core::ptr::null(), Some(capsule_drop)) }; - get_pool().register_decref(NonNull::new(capsule).unwrap()); + unsafe { get_pool().register_decref(NonNull::new(capsule).unwrap()) }; // Updating the counts will call decref on the capsule, which calls capsule_drop get_pool().drop_deferred_references(py); @@ -644,7 +695,9 @@ mod tests { // For AttachGuard::attach - get_pool().register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()); + unsafe { + get_pool().register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) + }; #[cfg(not(Py_GIL_DISABLED))] assert!(pool_dec_refs_contains(&obj)); let _guard = AttachGuard::attach(); @@ -652,7 +705,9 @@ mod tests { // For AttachGuard::assume - get_pool().register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()); + unsafe { + get_pool().register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) + }; #[cfg(not(Py_GIL_DISABLED))] assert!(pool_dec_refs_contains(&obj)); let _guard2 = unsafe { AttachGuard::assume() }; From 322dd735584f0622d7ab85d0ef007e598c1ccd4b Mon Sep 17 00:00:00 2001 From: person93 Date: Fri, 7 Aug 2026 04:00:53 -0400 Subject: [PATCH 04/10] remove OnceLock that was wrapping a Mutex --- src/internal/state.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/internal/state.rs b/src/internal/state.rs index 41038f5df5b..40cd7664368 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -11,7 +11,7 @@ use core::sync::atomic::{AtomicBool, Ordering}; #[cfg_attr(pyo3_disable_reference_pool, allow(unused_imports))] use core::{mem, ptr::NonNull}; #[cfg(not(pyo3_disable_reference_pool))] -use std::sync::{Mutex, OnceLock}; +use std::sync::Mutex; std::thread_local! { /// This is an internal counter in pyo3 monitoring whether this thread is attached to the interpreter. @@ -303,19 +303,19 @@ impl ReferencePool { } #[cfg(not(pyo3_disable_reference_pool))] -static POOL: OnceLock = OnceLock::new(); +static POOL: ReferencePool = ReferencePool::new(); #[cfg(not(pyo3_disable_reference_pool))] fn get_pool() -> &'static ReferencePool { - POOL.get_or_init(ReferencePool::new) + &POOL } #[cfg_attr(pyo3_disable_reference_pool, inline(always))] #[cfg_attr(pyo3_disable_reference_pool, allow(unused_variables))] fn drop_deferred_references(py: Python<'_>) { #[cfg(not(pyo3_disable_reference_pool))] - if let Some(pool) = POOL.get() { - pool.drop_deferred_references(py); + { + POOL.drop_deferred_references(py); } } @@ -344,10 +344,10 @@ impl Drop for SuspendAttach { unsafe { ffi::PyEval_RestoreThread(self.tstate) }; // Update counts of `Py` that were dropped while not attached. #[cfg(not(pyo3_disable_reference_pool))] - if let Some(pool) = POOL.get() { + { // SAFETY: just re-attached let py = unsafe { Python::assume_attached() }; - pool.drop_deferred_references(py); + POOL.drop_deferred_references(py); } } } From 7c9fbb3e806a1d3dd6ac3c9cd0b78f0a9f44b66e Mon Sep 17 00:00:00 2001 From: person93 Date: Fri, 7 Aug 2026 04:10:24 -0400 Subject: [PATCH 05/10] fix typos --- src/internal/state.rs | 2 +- src/type_object.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/state.rs b/src/internal/state.rs index 40cd7664368..baa38263759 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -146,7 +146,7 @@ impl AttachGuard { /// Attach to the interpreter, without a fast-path to check if the thread is already attached. /// # Safety - /// The interpreter must be sufficiently initialized ot attach a thread. + /// The interpreter must be sufficiently initialized to attach a thread. #[cold] unsafe fn do_attach_unchecked() -> Self { // SAFETY: interpreter is sufficiently initialized to attach a thread. diff --git a/src/type_object.rs b/src/type_object.rs index a8df18ff90a..5211e07ac22 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -72,7 +72,7 @@ pub unsafe trait PyTypeInfo: Sized { // // By making `Bound` we assume ownership which is then safe against races. let tp = Self::type_object_raw(py).cast::(); - // SAFETY: the pointer is known to be a borrowed type object and we immeditely make a new reference + // SAFETY: the pointer is known to be a borrowed type object and we immediately make a new reference unsafe { tp.assume_borrowed_unchecked(py).cast_unchecked() }.to_owned() } From 9a7e158e364b30d5f22e8accaadcabb857fdedfc Mon Sep 17 00:00:00 2001 From: person93 Date: Wed, 12 Aug 2026 21:32:26 -0400 Subject: [PATCH 06/10] remove `get_pool` function in `internal/state.rs` --- src/internal/state.rs | 36 ++++++++++-------------------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/src/internal/state.rs b/src/internal/state.rs index baa38263759..5b176dead3c 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -305,11 +305,6 @@ impl ReferencePool { #[cfg(not(pyo3_disable_reference_pool))] static POOL: ReferencePool = ReferencePool::new(); -#[cfg(not(pyo3_disable_reference_pool))] -fn get_pool() -> &'static ReferencePool { - &POOL -} - #[cfg_attr(pyo3_disable_reference_pool, inline(always))] #[cfg_attr(pyo3_disable_reference_pool, allow(unused_variables))] fn drop_deferred_references(py: Python<'_>) { @@ -400,7 +395,7 @@ pub unsafe fn register_decref(obj: NonNull) { #[cfg(not(pyo3_disable_reference_pool))] { // SAFETY: caller upholds requirements - unsafe { get_pool().register_decref(obj) }; + unsafe { POOL.register_decref(obj) }; } #[cfg(all( pyo3_disable_reference_pool, @@ -461,15 +456,9 @@ mod tests { #[cfg(not(pyo3_disable_reference_pool))] fn pool_dec_refs_does_not_contain(obj: &Py) -> bool { for _ in 0..100 { - if !get_pool() - .pending_decrefs - .lock() - .unwrap() - .iter() - .any(|pending| { - pending.as_raw() == (unsafe { NonNull::new_unchecked(obj.as_ptr()) }) - }) - { + if !POOL.pending_decrefs.lock().unwrap().iter().any(|pending| { + pending.as_raw() == (unsafe { NonNull::new_unchecked(obj.as_ptr()) }) + }) { return true; } @@ -485,8 +474,7 @@ mod tests { // function does not test anything meaningful #[cfg(not(any(pyo3_disable_reference_pool, Py_GIL_DISABLED)))] fn pool_dec_refs_contains(obj: &Py) -> bool { - get_pool() - .pending_decrefs + POOL.pending_decrefs .lock() .unwrap() .iter() @@ -641,7 +629,7 @@ mod tests { let ptr = Python::attach(|py| obj.clone_ref(py).into_ptr()); // A decref registered while detached applies once an attach drains the pool. - unsafe { get_pool().register_decref(NonNull::new(ptr).unwrap()) }; + unsafe { POOL.register_decref(NonNull::new(ptr).unwrap()) }; Python::attach(|_| { assert!(pool_dec_refs_does_not_contain(&obj)); @@ -680,10 +668,10 @@ mod tests { let capsule = unsafe { ffi::PyCapsule_New(ptr as _, core::ptr::null(), Some(capsule_drop)) }; - unsafe { get_pool().register_decref(NonNull::new(capsule).unwrap()) }; + unsafe { POOL.register_decref(NonNull::new(capsule).unwrap()) }; // Updating the counts will call decref on the capsule, which calls capsule_drop - get_pool().drop_deferred_references(py); + POOL.drop_deferred_references(py); }) } @@ -695,9 +683,7 @@ mod tests { // For AttachGuard::attach - unsafe { - get_pool().register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) - }; + unsafe { POOL.register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) }; #[cfg(not(Py_GIL_DISABLED))] assert!(pool_dec_refs_contains(&obj)); let _guard = AttachGuard::attach(); @@ -705,9 +691,7 @@ mod tests { // For AttachGuard::assume - unsafe { - get_pool().register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) - }; + unsafe { POOL.register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) }; #[cfg(not(Py_GIL_DISABLED))] assert!(pool_dec_refs_contains(&obj)); let _guard2 = unsafe { AttachGuard::assume() }; From 4a7e8e327992d0c76fd0f4495d5424ade3d10788 Mon Sep 17 00:00:00 2001 From: person93 Date: Wed, 12 Aug 2026 21:58:13 -0400 Subject: [PATCH 07/10] fix bug introduced in rebase --- src/internal/state.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/internal/state.rs b/src/internal/state.rs index 5b176dead3c..bee4c58e6b9 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -258,6 +258,7 @@ impl ReferencePool { unsafe fn register_decref(&self, obj: NonNull) { // SAFETY: requirements upheld by caller let pending = unsafe { PendingDecref::new(obj) }; + self.dirty.store(true, Ordering::Relaxed); self.pending_decrefs.lock().unwrap().push(pending); } From 94a72b80fdd33b777e50a3a02680364ef9ac0218 Mon Sep 17 00:00:00 2001 From: person93 Date: Sun, 16 Aug 2026 02:02:16 -0400 Subject: [PATCH 08/10] remove outdated and misleading doc comment --- src/internal/state.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/internal/state.rs b/src/internal/state.rs index bee4c58e6b9..cd42f29d716 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -385,9 +385,6 @@ impl Drop for ForbidAttaching { /// Registers a Python object pointer inside the release pool, to have its reference count decreased /// the next time the thread is attached in pyo3. /// -/// If the thread is attached, the reference count will be decreased immediately instead of being queued -/// for later. -/// /// # Safety /// - The object must be an owned Python reference. /// - The reference must not be used after calling this function. From 3b1913d13e6969622a82108f0edae8dcc4f7f0fe Mon Sep 17 00:00:00 2001 From: person93 Date: Sun, 16 Aug 2026 02:03:20 -0400 Subject: [PATCH 09/10] use Py for pending decrefs so register decref can be safe --- src/instance.rs | 7 ++- src/internal/state.rs | 105 +++++++++++------------------------------- 2 files changed, 30 insertions(+), 82 deletions(-) diff --git a/src/instance.rs b/src/instance.rs index b57e5dd07ee..244a3df6f46 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -2298,10 +2298,9 @@ impl Drop for Py { #[cold] fn drop_slow(obj: NonNull) { - // SAFETY: handing ownership of the reference to `register_decref`. - unsafe { - state::register_decref(obj); - } + // SAFETY: the Py instance being dropped will not use the pointer any more + let obj = unsafe { Py::from_non_null(obj) }; + state::register_decref(obj); } inner(self.0) diff --git a/src/internal/state.rs b/src/internal/state.rs index cd42f29d716..84a19caff6d 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -3,13 +3,13 @@ #[cfg(pyo3_disable_reference_pool)] use crate::impl_::panic::PanicTrap; use crate::platform::prelude::*; -use crate::{ffi, Python}; +use crate::{ffi, Py, PyAny, Python}; use core::cell::Cell; +#[cfg_attr(pyo3_disable_reference_pool, allow(unused_imports))] +use core::mem; #[cfg(not(pyo3_disable_reference_pool))] use core::sync::atomic::{AtomicBool, Ordering}; -#[cfg_attr(pyo3_disable_reference_pool, allow(unused_imports))] -use core::{mem, ptr::NonNull}; #[cfg(not(pyo3_disable_reference_pool))] use std::sync::Mutex; @@ -195,44 +195,7 @@ impl Drop for AttachGuard { } #[cfg(not(pyo3_disable_reference_pool))] -use self::pending_decref::PendingDecref; - -// NOTE: this is its own mod so that it can fully contain its unsafe assumptions -#[cfg(not(pyo3_disable_reference_pool))] -mod pending_decref { - use crate::ffi; - use crate::marker::Python; - use core::ptr::NonNull; - - #[repr(transparent)] - pub(super) struct PendingDecref(NonNull); - - // SAFETY: it's a python object - unsafe impl Send for PendingDecref {} - // SAFETY: it's a python object - unsafe impl Sync for PendingDecref {} - - impl PendingDecref { - /// # Safety - /// `obj` must point to a valid [`ffi::PyObject`] and it must not be used again after this call. - pub(super) unsafe fn new(obj: NonNull) -> Self { - Self(obj) - } - - pub(super) fn decref(self, _py: Python<'_>) { - // SAFETY: requirements upheld by constructor - unsafe { ffi::Py_DECREF(self.0.as_ptr()) }; - } - - #[cfg(test)] - pub(super) fn as_raw(&self) -> NonNull { - self.0 - } - } -} - -#[cfg(not(pyo3_disable_reference_pool))] -type PyObjVec = Vec; +type PyObjVec = Vec>; #[cfg(not(pyo3_disable_reference_pool))] /// Thread-safe storage for objects which were dec_ref while not attached. @@ -253,13 +216,9 @@ impl ReferencePool { } } - /// # Safety - /// `obj` must be a valid python object and it must not be used again after this call - unsafe fn register_decref(&self, obj: NonNull) { - // SAFETY: requirements upheld by caller - let pending = unsafe { PendingDecref::new(obj) }; + fn register_decref(&self, obj: Py) { self.dirty.store(true, Ordering::Relaxed); - self.pending_decrefs.lock().unwrap().push(pending); + self.pending_decrefs.lock().unwrap().push(obj); } fn drop_deferred_references(&self, py: Python<'_>) { @@ -297,8 +256,8 @@ impl ReferencePool { let decrefs = mem::take(&mut *pending_decrefs); drop(pending_decrefs); - for ptr in decrefs { - ptr.decref(py); + for obj in decrefs { + obj.drop_ref(py); } } } @@ -384,16 +343,11 @@ impl Drop for ForbidAttaching { /// Registers a Python object pointer inside the release pool, to have its reference count decreased /// the next time the thread is attached in pyo3. -/// -/// # Safety -/// - The object must be an owned Python reference. -/// - The reference must not be used after calling this function. #[inline] -pub unsafe fn register_decref(obj: NonNull) { +pub fn register_decref(obj: Py) { #[cfg(not(pyo3_disable_reference_pool))] { - // SAFETY: caller upholds requirements - unsafe { POOL.register_decref(obj) }; + POOL.register_decref(obj); } #[cfg(all( pyo3_disable_reference_pool, @@ -445,6 +399,7 @@ fn decrement_attach_count() { mod tests { use super::*; + use crate::ffi_ptr_ext::FfiPtrExt; use crate::{Py, PyAny, Python}; fn get_object(py: Python<'_>) -> Py { @@ -453,19 +408,12 @@ mod tests { #[cfg(not(pyo3_disable_reference_pool))] fn pool_dec_refs_does_not_contain(obj: &Py) -> bool { - for _ in 0..100 { - if !POOL.pending_decrefs.lock().unwrap().iter().any(|pending| { - pending.as_raw() == (unsafe { NonNull::new_unchecked(obj.as_ptr()) }) - }) { - return true; - } - - // It is possible for another thread to be about to remove the decref - // from the pool having already cleared the dirty flag, wait a bit and re-check. - std::thread::sleep(core::time::Duration::from_millis(5)); - } - - false + !POOL + .pending_decrefs + .lock() + .unwrap() + .iter() + .any(|pending| pending.is(obj)) } // With free-threading, threads can empty the POOL at any time, so this @@ -476,7 +424,7 @@ mod tests { .lock() .unwrap() .iter() - .any(|pending| pending.as_raw() == unsafe { NonNull::new_unchecked(obj.as_ptr()) }) + .any(|pending| pending.is(obj)) } #[test] @@ -624,10 +572,9 @@ mod tests { #[cfg(not(pyo3_disable_reference_pool))] fn test_detached_drop_is_collected_on_next_attach() { let obj = Python::attach(get_object); - let ptr = Python::attach(|py| obj.clone_ref(py).into_ptr()); + let obj2 = Python::attach(|py| obj.clone_ref(py)); - // A decref registered while detached applies once an attach drains the pool. - unsafe { POOL.register_decref(NonNull::new(ptr).unwrap()) }; + POOL.register_decref(obj2); Python::attach(|_| { assert!(pool_dec_refs_does_not_contain(&obj)); @@ -663,10 +610,12 @@ mod tests { let ptr = obj.into_ptr(); - let capsule = - unsafe { ffi::PyCapsule_New(ptr as _, core::ptr::null(), Some(capsule_drop)) }; + let capsule = unsafe { + ffi::PyCapsule_New(ptr as _, core::ptr::null(), Some(capsule_drop)).assume_owned(py) + } + .unbind(); - unsafe { POOL.register_decref(NonNull::new(capsule).unwrap()) }; + POOL.register_decref(capsule); // Updating the counts will call decref on the capsule, which calls capsule_drop POOL.drop_deferred_references(py); @@ -681,7 +630,7 @@ mod tests { // For AttachGuard::attach - unsafe { POOL.register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) }; + POOL.register_decref(obj.clone_ref(py)); #[cfg(not(Py_GIL_DISABLED))] assert!(pool_dec_refs_contains(&obj)); let _guard = AttachGuard::attach(); @@ -689,7 +638,7 @@ mod tests { // For AttachGuard::assume - unsafe { POOL.register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()) }; + POOL.register_decref(obj.clone_ref(py)); #[cfg(not(Py_GIL_DISABLED))] assert!(pool_dec_refs_contains(&obj)); let _guard2 = unsafe { AttachGuard::assume() }; From 07e61ef9e2c4c651509f56a2672411c1d14df767 Mon Sep 17 00:00:00 2001 From: person93 Date: Sun, 16 Aug 2026 02:31:19 -0400 Subject: [PATCH 10/10] undo change accidentally reverted during rebase --- src/internal/state.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/internal/state.rs b/src/internal/state.rs index 84a19caff6d..9b63526561a 100644 --- a/src/internal/state.rs +++ b/src/internal/state.rs @@ -408,12 +408,22 @@ mod tests { #[cfg(not(pyo3_disable_reference_pool))] fn pool_dec_refs_does_not_contain(obj: &Py) -> bool { - !POOL - .pending_decrefs - .lock() - .unwrap() - .iter() - .any(|pending| pending.is(obj)) + for _ in 0..100 { + if !POOL + .pending_decrefs + .lock() + .unwrap() + .iter() + .any(|pending| pending.is(obj)) + { + return true; + } + + // It is possible for another thread to be about to remove the decref + // from the pool having already cleared the dirty flag, wait a bit and re-check. + std::thread::sleep(core::time::Duration::from_millis(5)); + } + false } // With free-threading, threads can empty the POOL at any time, so this