From e519b3bcca971c54f8e190dbb1815b00881980f9 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 21 May 2024 16:36:00 -0400 Subject: [PATCH 1/8] fix calling POOL.update_counts() when no `gil-refs` feature --- src/gil.rs | 92 +++++++++++++++++++++++++++++++++------------------ src/marker.rs | 4 +++ 2 files changed, 64 insertions(+), 32 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index 6f97011b71c..ff03ed41a85 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -166,7 +166,9 @@ impl GILGuard { /// `GILGuard::Ensured` will be returned. pub(crate) fn acquire() -> Self { if gil_is_acquired() { - increment_gil_count(); + // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition + #[cfg(not(pyo3_disable_reference_pool))] + POOL.update_counts(unsafe { Python::assume_gil_acquired() }); return GILGuard::Assumed; } @@ -215,32 +217,33 @@ impl GILGuard { /// as part of multi-phase interpreter initialization. pub(crate) fn acquire_unchecked() -> Self { if gil_is_acquired() { - increment_gil_count(); + // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition + #[cfg(not(pyo3_disable_reference_pool))] + POOL.update_counts(unsafe { Python::assume_gil_acquired() }); return GILGuard::Assumed; } let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL + increment_gil_count(); + #[cfg(feature = "gil-refs")] #[allow(deprecated)] - { - let pool = unsafe { mem::ManuallyDrop::new(GILPool::new()) }; - increment_gil_count(); - GILGuard::Ensured { gstate, pool } - } - - #[cfg(not(feature = "gil-refs"))] - { - increment_gil_count(); - // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition - #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(unsafe { Python::assume_gil_acquired() }); + let pool = unsafe { mem::ManuallyDrop::new(GILPool::new()) }; - GILGuard::Ensured { gstate } + // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition + #[cfg(not(pyo3_disable_reference_pool))] + POOL.update_counts(unsafe { Python::assume_gil_acquired() }); + GILGuard::Ensured { + gstate, + #[cfg(feature = "gil-refs")] + pool, } } /// Acquires the `GILGuard` while assuming that the GIL is already held. pub(crate) unsafe fn assume() -> Self { - increment_gil_count(); + // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition + #[cfg(not(pyo3_disable_reference_pool))] + POOL.update_counts(unsafe { Python::assume_gil_acquired() }); GILGuard::Assumed } @@ -256,19 +259,19 @@ impl Drop for GILGuard { fn drop(&mut self) { match self { GILGuard::Assumed => {} - #[cfg(feature = "gil-refs")] - GILGuard::Ensured { gstate, pool } => unsafe { + GILGuard::Ensured { + gstate, + #[cfg(feature = "gil-refs")] + pool, + } => unsafe { // Drop the objects in the pool before attempting to release the thread state + #[cfg(feature = "gil-refs")] mem::ManuallyDrop::drop(pool); ffi::PyGILState_Release(*gstate); - }, - #[cfg(not(feature = "gil-refs"))] - GILGuard::Ensured { gstate } => unsafe { - ffi::PyGILState_Release(*gstate); + decrement_gil_count(); }, } - decrement_gil_count(); } } @@ -549,14 +552,15 @@ fn decrement_gil_count() { #[cfg(test)] mod tests { + use super::GIL_COUNT; #[cfg(not(pyo3_disable_reference_pool))] use super::{gil_is_acquired, POOL}; #[cfg(feature = "gil-refs")] #[allow(deprecated)] - use super::{GILPool, GIL_COUNT, OWNED_OBJECTS}; - use crate::types::any::PyAnyMethods; + use super::{GILPool, OWNED_OBJECTS}; #[cfg(feature = "gil-refs")] use crate::{ffi, gil}; + use crate::{gil::GILGuard, types::any::PyAnyMethods}; use crate::{PyObject, Python}; use std::ptr::NonNull; @@ -702,7 +706,6 @@ mod tests { } #[test] - #[cfg(feature = "gil-refs")] #[allow(deprecated)] fn test_gil_counts() { // Check with_gil and GILPool both increase counts correctly @@ -712,10 +715,10 @@ mod tests { Python::with_gil(|_| { assert_eq!(get_gil_count(), 1); - let pool = unsafe { GILPool::new() }; + let pool = unsafe { GILGuard::assume() }; assert_eq!(get_gil_count(), 1); - let pool2 = unsafe { GILPool::new() }; + let pool2 = unsafe { GILGuard::assume() }; assert_eq!(get_gil_count(), 1); drop(pool); @@ -723,7 +726,7 @@ mod tests { Python::with_gil(|_| { // nested with_gil doesn't update gil count - assert_eq!(get_gil_count(), 2); + assert_eq!(get_gil_count(), 1); }); assert_eq!(get_gil_count(), 1); @@ -794,19 +797,20 @@ mod tests { #[test] #[cfg(not(pyo3_disable_reference_pool))] - #[cfg(feature = "gil-refs")] fn test_update_counts_does_not_deadlock() { // update_counts can run arbitrary Python code during Py_DECREF. // if the locking is implemented incorrectly, it will deadlock. + use crate::ffi; + use crate::gil::GILGuard; + Python::with_gil(|py| { let obj = get_object(py); unsafe extern "C" fn capsule_drop(capsule: *mut ffi::PyObject) { // This line will implicitly call update_counts // -> and so cause deadlock if update_counts is not handling recursion correctly. - #[allow(deprecated)] - let pool = GILPool::new(); + let pool = GILGuard::assume(); // Rebuild obj so that it can be dropped PyObject::from_owned_ptr( @@ -826,4 +830,28 @@ mod tests { POOL.update_counts(py); }) } + + #[test] + #[cfg(not(pyo3_disable_reference_pool))] + fn test_gil_guard_update_counts() { + use crate::gil::GILGuard; + + Python::with_gil(|py| { + let obj = get_object(py); + + // For GILGuard::acquire + + POOL.register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()); + assert!(pool_dec_refs_contains(&obj)); + let _guard = GILGuard::acquire(); + assert!(pool_dec_refs_does_not_contain(&obj)); + + // For GILGuard::assume + + POOL.register_decref(NonNull::new(obj.clone_ref(py).into_ptr()).unwrap()); + assert!(pool_dec_refs_contains(&obj)); + let _guard2 = unsafe { GILGuard::assume() }; + assert!(pool_dec_refs_does_not_contain(&obj)); + }) + } } diff --git a/src/marker.rs b/src/marker.rs index 1f4655d9656..ba2d5d48db0 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -1280,6 +1280,10 @@ mod tests { const GIL_NOT_HELD: c_int = 0; const GIL_HELD: c_int = 1; + // Before starting the interpreter the state of calling `PyGILState_Check` + // seems to be undefined, so let's ensure that Python is up. + crate::prepare_freethreaded_python(); + let state = unsafe { crate::ffi::PyGILState_Check() }; assert_eq!(state, GIL_NOT_HELD); From 3ce45c59f8403230c2409d90d59411e09c3c7ddf Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 21 May 2024 17:23:32 -0400 Subject: [PATCH 2/8] fixup conditional compilation --- src/gil.rs | 25 ++++++++++++++----------- src/marker.rs | 1 + 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index ff03ed41a85..c82d5f9b044 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -166,10 +166,11 @@ impl GILGuard { /// `GILGuard::Ensured` will be returned. pub(crate) fn acquire() -> Self { if gil_is_acquired() { + let guard = GILGuard::Assumed; // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(unsafe { Python::assume_gil_acquired() }); - return GILGuard::Assumed; + POOL.update_counts(guard.python()); + return guard; } // Maybe auto-initialize the GIL: @@ -207,7 +208,8 @@ impl GILGuard { } } - Self::acquire_unchecked() + // SAFETY: We have ensured the Python interpreter is initialized. + unsafe { Self::acquire_unchecked() } } /// Acquires the `GILGuard` without performing any state checking. @@ -215,12 +217,13 @@ impl GILGuard { /// This can be called in "unsafe" contexts where the normal interpreter state /// checking performed by `GILGuard::acquire` may fail. This includes calling /// as part of multi-phase interpreter initialization. - pub(crate) fn acquire_unchecked() -> Self { + pub(crate) unsafe fn acquire_unchecked() -> Self { if gil_is_acquired() { + let guard = GILGuard::Assumed; // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(unsafe { Python::assume_gil_acquired() }); - return GILGuard::Assumed; + POOL.update_counts(guard.python()); + return guard; } let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL @@ -243,7 +246,7 @@ impl GILGuard { pub(crate) unsafe fn assume() -> Self { // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(unsafe { Python::assume_gil_acquired() }); + POOL.update_counts(Python::assume_gil_acquired()); GILGuard::Assumed } @@ -553,11 +556,11 @@ fn decrement_gil_count() { #[cfg(test)] mod tests { use super::GIL_COUNT; - #[cfg(not(pyo3_disable_reference_pool))] - use super::{gil_is_acquired, POOL}; #[cfg(feature = "gil-refs")] #[allow(deprecated)] - use super::{GILPool, OWNED_OBJECTS}; + use super::OWNED_OBJECTS; + #[cfg(not(pyo3_disable_reference_pool))] + use super::{gil_is_acquired, POOL}; #[cfg(feature = "gil-refs")] use crate::{ffi, gil}; use crate::{gil::GILGuard, types::any::PyAnyMethods}; @@ -708,7 +711,7 @@ mod tests { #[test] #[allow(deprecated)] fn test_gil_counts() { - // Check with_gil and GILPool both increase counts correctly + // Check with_gil and GILGuard both increase counts correctly let get_gil_count = || GIL_COUNT.with(|c| c.get()); assert_eq!(get_gil_count(), 0); diff --git a/src/marker.rs b/src/marker.rs index ba2d5d48db0..a6b1e305252 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -1282,6 +1282,7 @@ mod tests { // Before starting the interpreter the state of calling `PyGILState_Check` // seems to be undefined, so let's ensure that Python is up. + #[cfg(not(any(PyPy, GraalPy)))] crate::prepare_freethreaded_python(); let state = unsafe { crate::ffi::PyGILState_Check() }; From 2a38c8b7de8aca03e82c2b4debb425feca21b5ee Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 21 May 2024 18:10:31 -0400 Subject: [PATCH 3/8] always increment gil count --- src/gil.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index c82d5f9b044..52469fe7167 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -166,8 +166,8 @@ impl GILGuard { /// `GILGuard::Ensured` will be returned. pub(crate) fn acquire() -> Self { if gil_is_acquired() { + increment_gil_count(); let guard = GILGuard::Assumed; - // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition #[cfg(not(pyo3_disable_reference_pool))] POOL.update_counts(guard.python()); return guard; @@ -219,8 +219,8 @@ impl GILGuard { /// as part of multi-phase interpreter initialization. pub(crate) unsafe fn acquire_unchecked() -> Self { if gil_is_acquired() { + increment_gil_count(); let guard = GILGuard::Assumed; - // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition #[cfg(not(pyo3_disable_reference_pool))] POOL.update_counts(guard.python()); return guard; @@ -233,7 +233,6 @@ impl GILGuard { #[allow(deprecated)] let pool = unsafe { mem::ManuallyDrop::new(GILPool::new()) }; - // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition #[cfg(not(pyo3_disable_reference_pool))] POOL.update_counts(unsafe { Python::assume_gil_acquired() }); GILGuard::Ensured { @@ -244,7 +243,7 @@ impl GILGuard { } /// Acquires the `GILGuard` while assuming that the GIL is already held. pub(crate) unsafe fn assume() -> Self { - // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition + increment_gil_count(); #[cfg(not(pyo3_disable_reference_pool))] POOL.update_counts(Python::assume_gil_acquired()); GILGuard::Assumed @@ -270,11 +269,10 @@ impl Drop for GILGuard { // Drop the objects in the pool before attempting to release the thread state #[cfg(feature = "gil-refs")] mem::ManuallyDrop::drop(pool); - ffi::PyGILState_Release(*gstate); - decrement_gil_count(); }, } + decrement_gil_count(); } } From 4cbedea2f9c7f0c1a694a64f6b1c93bb2b589edc Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 21 May 2024 20:41:43 -0400 Subject: [PATCH 4/8] correct test --- src/gil.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index 52469fe7167..4ba7a4b24c5 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -717,19 +717,19 @@ mod tests { assert_eq!(get_gil_count(), 1); let pool = unsafe { GILGuard::assume() }; - assert_eq!(get_gil_count(), 1); + assert_eq!(get_gil_count(), 2); let pool2 = unsafe { GILGuard::assume() }; - assert_eq!(get_gil_count(), 1); + assert_eq!(get_gil_count(), 3); drop(pool); - assert_eq!(get_gil_count(), 1); + assert_eq!(get_gil_count(), 2); Python::with_gil(|_| { - // nested with_gil doesn't update gil count - assert_eq!(get_gil_count(), 1); + // nested with_gil updates gil count + assert_eq!(get_gil_count(), 3); }); - assert_eq!(get_gil_count(), 1); + assert_eq!(get_gil_count(), 2); drop(pool2); assert_eq!(get_gil_count(), 1); From 96936fd0d04581481e7b95b996492b53a36ac747 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 21 May 2024 21:04:30 -0400 Subject: [PATCH 5/8] clippy fix --- src/gil.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gil.rs b/src/gil.rs index 4ba7a4b24c5..f9132fb082d 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -587,7 +587,7 @@ mod tests { .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } - #[cfg(all(not(pyo3_disable_reference_pool), not(target_arch = "wasm32")))] + #[cfg(all(not(pyo3_disable_reference_pool)))] fn pool_dec_refs_contains(obj: &PyObject) -> bool { POOL.pending_decrefs .lock() From a6df3d024352a4212ec45d854feaf04e1abe980d Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 22 May 2024 11:50:56 +0100 Subject: [PATCH 6/8] fix clippy --- src/gil.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gil.rs b/src/gil.rs index f9132fb082d..38527b6f2e8 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -587,7 +587,7 @@ mod tests { .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } - #[cfg(all(not(pyo3_disable_reference_pool)))] + #[cfg(not(pyo3_disable_reference_pool))] fn pool_dec_refs_contains(obj: &PyObject) -> bool { POOL.pending_decrefs .lock() From ba64f8e1c575f3ac6333d5d4536a59a686b7eb36 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 2 Jun 2024 11:57:55 +0200 Subject: [PATCH 7/8] Deduplicate construction of GILGuard::Assumed. --- src/gil.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index 38527b6f2e8..e2169801bb1 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -166,11 +166,8 @@ impl GILGuard { /// `GILGuard::Ensured` will be returned. pub(crate) fn acquire() -> Self { if gil_is_acquired() { - increment_gil_count(); - let guard = GILGuard::Assumed; - #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(guard.python()); - return guard; + // SAFETY: We just checked that the GIL is already acquired. + return unsafe { Self::assume() }; } // Maybe auto-initialize the GIL: @@ -219,11 +216,7 @@ impl GILGuard { /// as part of multi-phase interpreter initialization. pub(crate) unsafe fn acquire_unchecked() -> Self { if gil_is_acquired() { - increment_gil_count(); - let guard = GILGuard::Assumed; - #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(guard.python()); - return guard; + return Self::assume(); } let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL @@ -241,12 +234,14 @@ impl GILGuard { pool, } } + /// Acquires the `GILGuard` while assuming that the GIL is already held. pub(crate) unsafe fn assume() -> Self { increment_gil_count(); + let guard = GILGuard::Assumed; #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(Python::assume_gil_acquired()); - GILGuard::Assumed + POOL.update_counts(guard.python()); + guard } /// Gets the Python token associated with this [`GILGuard`]. From 69315988d6abdc58ec0e231cbf63b96895a68f1c Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 2 Jun 2024 12:51:02 +0200 Subject: [PATCH 8/8] Remove unsafe-block-with-unsafe-function triggering errors in our MSRV builds. --- src/gil.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index e2169801bb1..3e25c7cfb0f 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -219,15 +219,15 @@ impl GILGuard { return Self::assume(); } - let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL + let gstate = ffi::PyGILState_Ensure(); // acquire GIL increment_gil_count(); #[cfg(feature = "gil-refs")] #[allow(deprecated)] - let pool = unsafe { mem::ManuallyDrop::new(GILPool::new()) }; + let pool = mem::ManuallyDrop::new(GILPool::new()); #[cfg(not(pyo3_disable_reference_pool))] - POOL.update_counts(unsafe { Python::assume_gil_acquired() }); + POOL.update_counts(Python::assume_gil_acquired()); GILGuard::Ensured { gstate, #[cfg(feature = "gil-refs")]