From c5f9001985c21ec9c5616c3ee09f2f1e5ca746af Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sat, 11 May 2024 16:48:45 +0200 Subject: [PATCH] Remove deferred reference count increments and make the global reference pool optional (#4095) * Add feature controlling the global reference pool to enable avoiding its overhead. * Document reference-pool feature in the performance guide. * Invert semantics of feature to disable reference pool so the new behaviour becomes opt-in * Remove delayed reference count increments as we cannot prevent reference count errors as long as these are available * Adjust tests to be compatible with disable-reference-pool feature * Adjust tests to be compatible with py-clone feature * Adjust the GIL benchmark to the updated reference pool semantics. * Further extend and clarify the documentation of the py-clone and disable-reference-pool features * Replace disable-reference-pool feature by pyo3_disable_reference_pool conditional compilation flag Such a flag is harder to use and thereby also harder to abuse. This seems appropriate as this is purely a performance-oriented change which show only be enabled by leaf crates and brings with it additional highly implicit sources of process aborts. * Add pyo3_leak_on_drop_without_reference_pool to turn aborts into leaks when the global reference pool is disabled and the GIL is not held --- Cargo.toml | 4 + examples/Cargo.toml | 2 +- guide/src/class.md | 4 +- guide/src/faq.md | 7 +- guide/src/features.md | 10 ++ guide/src/memory.md | 9 +- guide/src/migration.md | 13 +- guide/src/performance.md | 44 ++++++ newsfragments/4095.added.md | 1 + newsfragments/4095.changed.md | 1 + pyo3-benches/benches/bench_gil.rs | 12 +- pyo3-build-config/src/lib.rs | 2 + src/conversions/std/option.rs | 2 +- src/err/err_state.rs | 14 +- src/err/mod.rs | 2 +- src/gil.rs | 242 ++++++------------------------ src/instance.rs | 32 +++- src/marker.rs | 4 +- src/pybacked.rs | 9 +- src/tests/hygiene/pyclass.rs | 2 +- src/types/capsule.rs | 8 +- src/types/iterator.rs | 2 +- src/types/sequence.rs | 2 +- tests/test_buffer_protocol.rs | 2 +- tests/test_bytes.rs | 2 + tests/test_class_basics.rs | 14 +- tests/test_class_conversion.rs | 4 + tests/test_gc.rs | 2 + tests/test_methods.rs | 3 + tests/test_no_imports.rs | 3 + tests/test_sequence.rs | 3 + tests/test_serde.rs | 5 +- 32 files changed, 226 insertions(+), 240 deletions(-) create mode 100644 newsfragments/4095.added.md create mode 100644 newsfragments/4095.changed.md diff --git a/Cargo.toml b/Cargo.toml index 4c5a0830..aba8fd1b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -108,6 +108,9 @@ auto-initialize = [] # Allows use of the deprecated "GIL Refs" APIs. gil-refs = [] +# Enables `Clone`ing references to Python objects `Py` which panics if the GIL is not held. +py-clone = [] + # Optimizes PyObject to Vec conversion and so on. nightly = [] @@ -129,6 +132,7 @@ full = [ "num-bigint", "num-complex", "num-rational", + "py-clone", "rust_decimal", "serde", "smallvec", diff --git a/examples/Cargo.toml b/examples/Cargo.toml index e54b3b5c..81557e7f 100644 --- a/examples/Cargo.toml +++ b/examples/Cargo.toml @@ -10,5 +10,5 @@ pyo3 = { path = "..", features = ["auto-initialize", "extension-module"] } [[example]] name = "decorator" path = "decorator/src/lib.rs" -crate_type = ["cdylib"] +crate-type = ["cdylib"] doc-scrape-examples = true diff --git a/guide/src/class.md b/guide/src/class.md index 3fcfaca4..91a6fb2c 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -249,7 +249,7 @@ fn return_myclass() -> Py { let obj = return_myclass(); -Python::with_gil(|py| { +Python::with_gil(move |py| { let bound = obj.bind(py); // Py::bind returns &Bound<'py, MyClass> let obj_ref = bound.borrow(); // Get PyRef assert_eq!(obj_ref.num, 1); @@ -280,6 +280,8 @@ let py_counter: Py = Python::with_gil(|py| { }); py_counter.get().value.fetch_add(1, Ordering::Relaxed); + +Python::with_gil(move |_py| drop(py_counter)); ``` Frozen classes are likely to become the default thereby guiding the PyO3 ecosystem towards a more deliberate application of interior mutability. Eventually, this should enable further optimizations of PyO3's internals and avoid downstream code paying the cost of interior mutability when it is not actually required. diff --git a/guide/src/faq.md b/guide/src/faq.md index 19f9b5d5..b79641a3 100644 --- a/guide/src/faq.md +++ b/guide/src/faq.md @@ -127,12 +127,10 @@ If you don't want that cloning to happen, a workaround is to allocate the field ```rust # use pyo3::prelude::*; #[pyclass] -#[derive(Clone)] struct Inner {/* fields omitted */} #[pyclass] struct Outer { - #[pyo3(get)] inner: Py, } @@ -144,6 +142,11 @@ impl Outer { inner: Py::new(py, Inner {})?, }) } + + #[getter] + fn inner(&self, py: Python<'_>) -> Py { + self.inner.clone_ref(py) + } } ``` This time `a` and `b` *are* the same object: diff --git a/guide/src/features.md b/guide/src/features.md index 07085a9e..6a25d40c 100644 --- a/guide/src/features.md +++ b/guide/src/features.md @@ -75,6 +75,14 @@ This feature is a backwards-compatibility feature to allow continued use of the This feature and the APIs it enables is expected to be removed in a future PyO3 version. +### `py-clone` + +This feature was introduced to ease migration. It was found that delayed reference counts cannot be made sound and hence `Clon`ing an instance of `Py` must panic without the GIL being held. To avoid migrations introducing new panics without warning, the `Clone` implementation itself is now gated behind this feature. + +### `pyo3_disable_reference_pool` + +This is a performance-oriented conditional compilation flag, e.g. [set via `$RUSTFLAGS`][set-configuration-options], which disabled the global reference pool and the assocaited overhead for the crossing the Python-Rust boundary. However, if enabled, `Drop`ping an instance of `Py` without the GIL being held will abort the process. + ### `macros` This feature enables a dependency on the `pyo3-macros` crate, which provides the procedural macros portion of PyO3's API: @@ -195,3 +203,5 @@ struct User { ### `smallvec` Adds a dependency on [smallvec](https://docs.rs/smallvec) and enables conversions into its [`SmallVec`](https://docs.rs/smallvec/latest/smallvec/struct.SmallVec.html) type. + +[set-configuration-options]: https://doc.rust-lang.org/reference/conditional-compilation.html#set-configuration-options diff --git a/guide/src/memory.md b/guide/src/memory.md index b37d8356..38a31f4d 100644 --- a/guide/src/memory.md +++ b/guide/src/memory.md @@ -212,7 +212,8 @@ This example wasn't very interesting. We could have just used a GIL-bound we are *not* holding the GIL? ```rust -# #![allow(unused_imports)] +# #![allow(unused_imports, dead_code)] +# #[cfg(not(pyo3_disable_reference_pool))] { # use pyo3::prelude::*; # use pyo3::types::PyString; # fn main() -> PyResult<()> { @@ -239,12 +240,14 @@ Python::with_gil(|py| # } # Ok(()) # } +# } ``` When `hello` is dropped *nothing* happens to the pointed-to memory on Python's heap because nothing _can_ happen if we're not holding the GIL. Fortunately, -the memory isn't leaked. PyO3 keeps track of the memory internally and will -release it the next time we acquire the GIL. +the memory isn't leaked. If the `pyo3_disable_reference_pool` conditional compilation flag +is not enabled, PyO3 keeps track of the memory internally and will release it +the next time we acquire the GIL. We can avoid the delay in releasing memory if we are careful to drop the `Py` while the GIL is held. diff --git a/guide/src/migration.md b/guide/src/migration.md index 7e0420de..10b62002 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -35,7 +35,16 @@ fn increment(x: u64, amount: Option) -> u64 { x + amount.unwrap_or(1) } ``` + +### `Py::clone` is now gated behind the `py-clone` feature +
+Click to expand +If you rely on `impl Clone for Py` to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind, the newly introduced feature `py-clone` must be enabled. + +However, take care to note that the behaviour is different from previous versions. If `Clone` was called without the GIL being held, we tried to delay the application of these reference count increments until PyO3-based code would re-acquire it. This turned out to be impossible to implement in a sound manner and hence was removed. Now, if `Clone` is called without the GIL being held, we panic instead for which calling code might not be prepared. + +Related to this, we also added a `pyo3_disable_reference_pool` conditional compilation flag which removes the infrastructure necessary to apply delayed reference count decrements implied by `impl Drop for Py`. They do not appear to be a soundness hazard as they should lead to memory leaks in the worst case. However, the global synchronization adds significant overhead to cross the Python-Rust boundary. Enabling this feature will remove these costs and make the `Drop` implementation abort the process if called without the GIL being held instead.
## from 0.20.* to 0.21 @@ -676,7 +685,7 @@ drop(second); The replacement is [`Python::with_gil`](https://docs.rs/pyo3/0.18.3/pyo3/marker/struct.Python.html#method.with_gil) which is more cumbersome but enforces the proper nesting by design, e.g. -```rust +```rust,ignore # #![allow(dead_code)] # use pyo3::prelude::*; @@ -701,7 +710,7 @@ let second = Python::with_gil(|py| Object::new(py)); drop(first); drop(second); -// Or it ensure releasing the inner lock before the outer one. +// Or it ensures releasing the inner lock before the outer one. Python::with_gil(|py| { let first = Object::new(py); let second = Python::with_gil(|py| Object::new(py)); diff --git a/guide/src/performance.md b/guide/src/performance.md index c47a91de..b3d160fe 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -96,3 +96,47 @@ impl PartialEq for FooBound<'_> { } } ``` + +## Disable the global reference pool + +PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Drop for Py` being called without the GIL being held. The necessary synchronization to obtain and apply these reference count updates when PyO3-based code next acquires the GIL is somewhat expensive and can become a significant part of the cost of crossing the Python-Rust boundary. + +This functionality can be avoided by setting the `pyo3_disable_reference_pool` conditional compilation flag. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Drop` implementation for `Py` which is necessary to interoperate with existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementation but abort when `Drop` is called without the GIL being held. If `pyo3_leak_on_drop_without_reference_pool` is additionally enabled, objects dropped without the GIL being held will be leaked instead which is always sound but might have determinal effects like resource exhaustion in the long term. + +This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` (or types containing it like `PyErr`, `PyBackedStr` or `PyBackedBytes`) returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code + +```rust,ignore +# use pyo3::prelude::*; +# use pyo3::types::PyList; +let numbers: Py = Python::with_gil(|py| PyList::empty_bound(py).unbind()); + +Python::with_gil(|py| { + numbers.bind(py).append(23).unwrap(); +}); + +Python::with_gil(|py| { + numbers.bind(py).append(42).unwrap(); +}); +``` + +will abort if the list not explicitly disposed via + +```rust +# use pyo3::prelude::*; +# use pyo3::types::PyList; +let numbers: Py = Python::with_gil(|py| PyList::empty_bound(py).unbind()); + +Python::with_gil(|py| { + numbers.bind(py).append(23).unwrap(); +}); + +Python::with_gil(|py| { + numbers.bind(py).append(42).unwrap(); +}); + +Python::with_gil(move |py| { + drop(numbers); +}); +``` + +[conditional-compilation]: https://doc.rust-lang.org/reference/conditional-compilation.html diff --git a/newsfragments/4095.added.md b/newsfragments/4095.added.md new file mode 100644 index 00000000..c9940f70 --- /dev/null +++ b/newsfragments/4095.added.md @@ -0,0 +1 @@ +Add `pyo3_disable_reference_pool` conditional compilation flag to avoid the overhead of the global reference pool at the cost of known limitations as explained in the performance section of the guide. diff --git a/newsfragments/4095.changed.md b/newsfragments/4095.changed.md new file mode 100644 index 00000000..7f155ae0 --- /dev/null +++ b/newsfragments/4095.changed.md @@ -0,0 +1 @@ +`Clone`ing pointers into the Python heap has been moved behind the `py-clone` feature, as it must panic without the GIL being held as a soundness fix. diff --git a/pyo3-benches/benches/bench_gil.rs b/pyo3-benches/benches/bench_gil.rs index 59b9ff96..cede8836 100644 --- a/pyo3-benches/benches/bench_gil.rs +++ b/pyo3-benches/benches/bench_gil.rs @@ -1,4 +1,4 @@ -use codspeed_criterion_compat::{criterion_group, criterion_main, BatchSize, Bencher, Criterion}; +use codspeed_criterion_compat::{criterion_group, criterion_main, Bencher, Criterion}; use pyo3::prelude::*; @@ -9,14 +9,8 @@ fn bench_clean_acquire_gil(b: &mut Bencher<'_>) { fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { let obj = Python::with_gil(|py| py.None()); - b.iter_batched( - || { - // Clone and drop an object so that the GILPool has work to do. - let _ = obj.clone(); - }, - |_| Python::with_gil(|_| {}), - BatchSize::NumBatches(1), - ); + // Drop the returned clone of the object so that the reference pool has work to do. + b.iter(|| Python::with_gil(|py| obj.clone_ref(py))); } fn criterion_benchmark(c: &mut Criterion) { diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 24d3ae28..54aff4d1 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -165,6 +165,8 @@ pub fn print_expected_cfgs() { println!("cargo:rustc-check-cfg=cfg(GraalPy)"); println!("cargo:rustc-check-cfg=cfg(py_sys_config, values(\"Py_DEBUG\", \"Py_REF_DEBUG\", \"Py_TRACE_REFS\", \"COUNT_ALLOCS\"))"); println!("cargo:rustc-check-cfg=cfg(invalid_from_utf8_lint)"); + println!("cargo:rustc-check-cfg=cfg(pyo3_disable_reference_pool)"); + println!("cargo:rustc-check-cfg=cfg(pyo3_leak_on_drop_without_reference_pool)"); // allow `Py_3_*` cfgs from the minimum supported version up to the // maximum minor version (+1 for development for the next) diff --git a/src/conversions/std/option.rs b/src/conversions/std/option.rs index 2fa082ba..13527315 100644 --- a/src/conversions/std/option.rs +++ b/src/conversions/std/option.rs @@ -61,7 +61,7 @@ mod tests { assert_eq!(option.as_ptr(), std::ptr::null_mut()); let none = py.None(); - option = Some(none.clone()); + option = Some(none.clone_ref(py)); let ref_cnt = none.get_refcnt(py); assert_eq!(option.as_ptr(), none.as_ptr()); diff --git a/src/err/err_state.rs b/src/err/err_state.rs index 9f85296f..14345b27 100644 --- a/src/err/err_state.rs +++ b/src/err/err_state.rs @@ -5,7 +5,6 @@ use crate::{ Bound, IntoPy, Py, PyAny, PyObject, PyTypeInfo, Python, }; -#[derive(Clone)] pub(crate) struct PyErrStateNormalized { #[cfg(not(Py_3_12))] ptype: Py, @@ -63,6 +62,19 @@ impl PyErrStateNormalized { ptraceback: Py::from_owned_ptr_or_opt(py, ptraceback), } } + + pub fn clone_ref(&self, py: Python<'_>) -> Self { + Self { + #[cfg(not(Py_3_12))] + ptype: self.ptype.clone_ref(py), + pvalue: self.pvalue.clone_ref(py), + #[cfg(not(Py_3_12))] + ptraceback: self + .ptraceback + .as_ref() + .map(|ptraceback| ptraceback.clone_ref(py)), + } + } } pub(crate) struct PyErrStateLazyFnOutput { diff --git a/src/err/mod.rs b/src/err/mod.rs index 52e4b6c6..29d4ac36 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -837,7 +837,7 @@ impl PyErr { /// ``` #[inline] pub fn clone_ref(&self, py: Python<'_>) -> PyErr { - PyErr::from_state(PyErrState::Normalized(self.normalized(py).clone())) + PyErr::from_state(PyErrState::Normalized(self.normalized(py).clone_ref(py))) } /// Return the cause (either an exception instance, or None, set by `raise ... from ...`) diff --git a/src/gil.rs b/src/gil.rs index 29c4ffbe..d4b92a4c 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -1,6 +1,8 @@ //! Interaction with Python's global interpreter lock use crate::impl_::not_send::{NotSend, NOT_SEND}; +#[cfg(pyo3_disable_reference_pool)] +use crate::impl_::panic::PanicTrap; use crate::{ffi, Python}; use std::cell::Cell; #[cfg(debug_assertions)] @@ -233,42 +235,32 @@ impl Drop for GILGuard { // Vector of PyObject type PyObjVec = Vec>; +#[cfg(not(pyo3_disable_reference_pool))] /// Thread-safe storage for objects which were inc_ref / dec_ref while the GIL was not held. struct ReferencePool { - // .0 is INCREFs, .1 is DECREFs - pointer_ops: sync::Mutex<(PyObjVec, PyObjVec)>, + pending_decrefs: sync::Mutex, } +#[cfg(not(pyo3_disable_reference_pool))] impl ReferencePool { const fn new() -> Self { Self { - pointer_ops: sync::Mutex::new((Vec::new(), Vec::new())), + pending_decrefs: sync::Mutex::new(Vec::new()), } } - fn register_incref(&self, obj: NonNull) { - self.pointer_ops.lock().unwrap().0.push(obj); - } - fn register_decref(&self, obj: NonNull) { - self.pointer_ops.lock().unwrap().1.push(obj); + self.pending_decrefs.lock().unwrap().push(obj); } fn update_counts(&self, _py: Python<'_>) { - let mut ops = self.pointer_ops.lock().unwrap(); - if ops.0.is_empty() && ops.1.is_empty() { + let mut pending_decrefs = self.pending_decrefs.lock().unwrap(); + if pending_decrefs.is_empty() { return; } - let (increfs, decrefs) = mem::take(&mut *ops); - drop(ops); - - // Always increase reference counts first - as otherwise objects which have a - // nonzero total reference count might be incorrectly dropped by Python during - // this update. - for ptr in increfs { - unsafe { ffi::Py_INCREF(ptr.as_ptr()) }; - } + let decrefs = mem::take(&mut *pending_decrefs); + drop(pending_decrefs); for ptr in decrefs { unsafe { ffi::Py_DECREF(ptr.as_ptr()) }; @@ -276,8 +268,10 @@ impl ReferencePool { } } +#[cfg(not(pyo3_disable_reference_pool))] unsafe impl Sync for ReferencePool {} +#[cfg(not(pyo3_disable_reference_pool))] static POOL: ReferencePool = ReferencePool::new(); /// A guard which can be used to temporarily release the GIL and restore on `Drop`. @@ -302,6 +296,7 @@ impl Drop for SuspendGIL { ffi::PyEval_RestoreThread(self.tstate); // Update counts of PyObjects / Py that were cloned or dropped while the GIL was released. + #[cfg(not(pyo3_disable_reference_pool))] POOL.update_counts(Python::assume_gil_acquired()); } } @@ -376,6 +371,7 @@ impl GILPool { pub unsafe fn new() -> GILPool { 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(Python::assume_gil_acquired()); GILPool { start: OWNED_OBJECTS @@ -434,11 +430,13 @@ impl Drop for GILPool { /// /// # Safety /// The object must be an owned Python reference. +#[cfg(feature = "py-clone")] +#[track_caller] pub unsafe fn register_incref(obj: NonNull) { if gil_is_acquired() { ffi::Py_INCREF(obj.as_ptr()) } else { - POOL.register_incref(obj); + panic!("Cannot clone pointer into Python heap without the GIL being held."); } } @@ -450,11 +448,21 @@ pub unsafe fn register_incref(obj: NonNull) { /// /// # Safety /// The object must be an owned Python reference. +#[track_caller] pub unsafe fn register_decref(obj: NonNull) { if gil_is_acquired() { ffi::Py_DECREF(obj.as_ptr()) } else { + #[cfg(not(pyo3_disable_reference_pool))] POOL.register_decref(obj); + #[cfg(all( + pyo3_disable_reference_pool, + not(pyo3_leak_on_drop_without_reference_pool) + ))] + { + let _trap = PanicTrap::new("Aborting the process to avoid panic-from-drop."); + panic!("Cannot drop pointer into Python heap without the GIL being held."); + } } } @@ -508,15 +516,18 @@ fn decrement_gil_count() { mod tests { #[allow(deprecated)] use super::GILPool; - use super::{gil_is_acquired, GIL_COUNT, POOL}; + #[cfg(not(pyo3_disable_reference_pool))] + use super::POOL; + use super::{gil_is_acquired, GIL_COUNT}; + #[cfg(not(pyo3_disable_reference_pool))] + use crate::ffi; use crate::types::any::PyAnyMethods; - use crate::{ffi, PyObject, Python}; + use crate::{PyObject, Python}; #[cfg(feature = "gil-refs")] use {super::OWNED_OBJECTS, crate::gil}; + #[cfg(not(pyo3_disable_reference_pool))] use std::ptr::NonNull; - #[cfg(not(target_arch = "wasm32"))] - use std::sync; fn get_object(py: Python<'_>) -> PyObject { py.eval_bound("object()", None, None).unwrap().unbind() @@ -531,30 +542,20 @@ mod tests { len } - fn pool_inc_refs_does_not_contain(obj: &PyObject) -> bool { - !POOL - .pointer_ops - .lock() - .unwrap() - .0 - .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) - } - + #[cfg(not(pyo3_disable_reference_pool))] fn pool_dec_refs_does_not_contain(obj: &PyObject) -> bool { !POOL - .pointer_ops + .pending_decrefs .lock() .unwrap() - .1 .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(not(pyo3_disable_reference_pool), not(target_arch = "wasm32")))] fn pool_dec_refs_contains(obj: &PyObject) -> bool { - POOL.pointer_ops + POOL.pending_decrefs .lock() .unwrap() - .1 .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } @@ -629,20 +630,20 @@ mod tests { let reference = obj.clone_ref(py); assert_eq!(obj.get_refcnt(py), 2); - assert!(pool_inc_refs_does_not_contain(&obj)); + #[cfg(not(pyo3_disable_reference_pool))] assert!(pool_dec_refs_does_not_contain(&obj)); // With the GIL held, reference count will be decreased immediately. drop(reference); assert_eq!(obj.get_refcnt(py), 1); - assert!(pool_inc_refs_does_not_contain(&obj)); + #[cfg(not(pyo3_disable_reference_pool))] assert!(pool_dec_refs_does_not_contain(&obj)); }); } #[test] - #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled + #[cfg(all(not(pyo3_disable_reference_pool), not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled fn test_pyobject_drop_without_gil_doesnt_decrease_refcnt() { let obj = Python::with_gil(|py| { let obj = get_object(py); @@ -650,7 +651,6 @@ mod tests { let reference = obj.clone_ref(py); assert_eq!(obj.get_refcnt(py), 2); - assert!(pool_inc_refs_does_not_contain(&obj)); assert!(pool_dec_refs_does_not_contain(&obj)); // Drop reference in a separate thread which doesn't have the GIL. @@ -659,7 +659,6 @@ mod tests { // The reference count should not have changed (the GIL has always // been held by this thread), it is remembered to release later. assert_eq!(obj.get_refcnt(py), 2); - assert!(pool_inc_refs_does_not_contain(&obj)); assert!(pool_dec_refs_contains(&obj)); obj }); @@ -667,9 +666,7 @@ mod tests { // Next time the GIL is acquired, the reference is released Python::with_gil(|py| { assert_eq!(obj.get_refcnt(py), 1); - let non_null = unsafe { NonNull::new_unchecked(obj.as_ptr()) }; - assert!(!POOL.pointer_ops.lock().unwrap().0.contains(&non_null)); - assert!(!POOL.pointer_ops.lock().unwrap().1.contains(&non_null)); + assert!(pool_dec_refs_does_not_contain(&obj)); }); } @@ -725,19 +722,16 @@ mod tests { assert!(!gil_is_acquired()); } + #[cfg(feature = "py-clone")] #[test] + #[should_panic] fn test_allow_threads_updates_refcounts() { Python::with_gil(|py| { // Make a simple object with 1 reference let obj = get_object(py); assert!(obj.get_refcnt(py) == 1); - // Clone the object without the GIL to use internal tracking - let escaped_ref = py.allow_threads(|| obj.clone()); - // But after the block the refcounts are updated - assert!(obj.get_refcnt(py) == 2); - drop(escaped_ref); - assert!(obj.get_refcnt(py) == 1); - drop(obj); + // Clone the object without the GIL which should panic + py.allow_threads(|| obj.clone()); }); } @@ -752,6 +746,7 @@ mod tests { }) } + #[cfg(feature = "py-clone")] #[test] fn test_clone_with_gil() { Python::with_gil(|py| { @@ -765,147 +760,8 @@ mod tests { }) } - #[cfg(not(target_arch = "wasm32"))] - struct Event { - set: sync::Mutex, - wait: sync::Condvar, - } - - #[cfg(not(target_arch = "wasm32"))] - impl Event { - const fn new() -> Self { - Self { - set: sync::Mutex::new(false), - wait: sync::Condvar::new(), - } - } - - fn set(&self) { - *self.set.lock().unwrap() = true; - self.wait.notify_all(); - } - - fn wait(&self) { - drop( - self.wait - .wait_while(self.set.lock().unwrap(), |s| !*s) - .unwrap(), - ); - } - } - - #[test] - #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled - fn test_clone_without_gil() { - use crate::{Py, PyAny}; - use std::{sync::Arc, thread}; - - // Some events for synchronizing - static GIL_ACQUIRED: Event = Event::new(); - static OBJECT_CLONED: Event = Event::new(); - static REFCNT_CHECKED: Event = Event::new(); - - Python::with_gil(|py| { - let obj: Arc> = Arc::new(get_object(py)); - let thread_obj = Arc::clone(&obj); - - let count = obj.get_refcnt(py); - println!( - "1: The object has been created and its reference count is {}", - count - ); - - let handle = thread::spawn(move || { - Python::with_gil(move |py| { - println!("3. The GIL has been acquired on another thread."); - GIL_ACQUIRED.set(); - - // Wait while the main thread registers obj in POOL - OBJECT_CLONED.wait(); - println!("5. Checking refcnt"); - assert_eq!(thread_obj.get_refcnt(py), count); - - REFCNT_CHECKED.set(); - }) - }); - - let cloned = py.allow_threads(|| { - println!("2. The GIL has been released."); - - // Wait until the GIL has been acquired on the thread. - GIL_ACQUIRED.wait(); - - println!("4. The other thread is now hogging the GIL, we clone without it held"); - // Cloning without GIL should not update reference count - let cloned = Py::clone(&*obj); - OBJECT_CLONED.set(); - cloned - }); - - REFCNT_CHECKED.wait(); - - println!("6. The main thread has acquired the GIL again and processed the pool."); - - // Total reference count should be one higher - assert_eq!(obj.get_refcnt(py), count + 1); - - // Clone dropped - drop(cloned); - // Ensure refcount of the arc is 1 - handle.join().unwrap(); - - // Overall count is now back to the original, and should be no pending change - assert_eq!(Arc::try_unwrap(obj).unwrap().get_refcnt(py), count); - }); - } - - #[test] - #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled - fn test_clone_in_other_thread() { - use crate::Py; - use std::{sync::Arc, thread}; - - // Some events for synchronizing - static OBJECT_CLONED: Event = Event::new(); - - let (obj, count, ptr) = Python::with_gil(|py| { - let obj = Arc::new(get_object(py)); - let count = obj.get_refcnt(py); - let thread_obj = Arc::clone(&obj); - - // Start a thread which does not have the GIL, and clone it - let t = thread::spawn(move || { - // Cloning without GIL should not update reference count - #[allow(clippy::redundant_clone)] - let _ = Py::clone(&*thread_obj); - OBJECT_CLONED.set(); - }); - - OBJECT_CLONED.wait(); - assert_eq!(count, obj.get_refcnt(py)); - - t.join().unwrap(); - let ptr = NonNull::new(obj.as_ptr()).unwrap(); - - // The pointer should appear once in the incref pool, and once in the - // decref pool (for the clone being created and also dropped) - assert!(POOL.pointer_ops.lock().unwrap().0.contains(&ptr)); - assert!(POOL.pointer_ops.lock().unwrap().1.contains(&ptr)); - - (obj, count, ptr) - }); - - Python::with_gil(|py| { - // Acquiring the gil clears the pool - assert!(!POOL.pointer_ops.lock().unwrap().0.contains(&ptr)); - assert!(!POOL.pointer_ops.lock().unwrap().1.contains(&ptr)); - - // Overall count is still unchanged - assert_eq!(count, obj.get_refcnt(py)); - }); - } - #[test] + #[cfg(not(pyo3_disable_reference_pool))] 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. diff --git a/src/instance.rs b/src/instance.rs index 1c510b90..7835b9c7 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -81,10 +81,11 @@ where /// struct Foo {/* fields omitted */} /// /// # fn main() -> PyResult<()> { - /// Python::with_gil(|py| -> PyResult> { + /// let foo: Py = Python::with_gil(|py| -> PyResult<_> { /// let foo: Bound<'_, Foo> = Bound::new(py, Foo {})?; /// Ok(foo.into()) /// })?; + /// # Python::with_gil(move |_py| drop(foo)); /// # Ok(()) /// # } /// ``` @@ -865,7 +866,9 @@ impl IntoPy for Borrowed<'_, '_, T> { /// // All of these are valid syntax /// let second = Py::clone_ref(&first, py); /// let third = first.clone_ref(py); +/// #[cfg(feature = "py-clone")] /// let fourth = Py::clone(&first); +/// #[cfg(feature = "py-clone")] /// let fifth = first.clone(); /// /// // Disposing of our original `Py` just decrements the reference count. @@ -873,7 +876,9 @@ impl IntoPy for Borrowed<'_, '_, T> { /// /// // They all point to the same object /// assert!(second.is(&third)); +/// #[cfg(feature = "py-clone")] /// assert!(fourth.is(&fifth)); +/// #[cfg(feature = "py-clone")] /// assert!(second.is(&fourth)); /// }); /// # } @@ -935,10 +940,11 @@ where /// struct Foo {/* fields omitted */} /// /// # fn main() -> PyResult<()> { - /// Python::with_gil(|py| -> PyResult> { + /// let foo = Python::with_gil(|py| -> PyResult<_> { /// let foo: Py = Py::new(py, Foo {})?; /// Ok(foo) /// })?; + /// # Python::with_gil(move |_py| drop(foo)); /// # Ok(()) /// # } /// ``` @@ -1244,6 +1250,7 @@ where /// }); /// /// cell.get().value.fetch_add(1, Ordering::Relaxed); + /// # Python::with_gil(move |_py| drop(cell)); /// ``` #[inline] pub fn get(&self) -> &T @@ -1804,9 +1811,12 @@ where } /// If the GIL is held this increments `self`'s reference count. -/// Otherwise this registers the [`Py`]`` instance to have its reference count -/// incremented the next time PyO3 acquires the GIL. +/// Otherwise, it will panic. +/// +/// Only available if the `py-clone` feature is enabled. +#[cfg(feature = "py-clone")] impl Clone for Py { + #[track_caller] fn clone(&self) -> Self { unsafe { gil::register_incref(self.0); @@ -1815,8 +1825,16 @@ impl Clone for Py { } } -/// Dropping a `Py` instance decrements the reference count on the object by 1. +/// Dropping a `Py` instance decrements the reference count +/// on the object by one if the GIL is held. +/// +/// Otherwise and by default, this registers the underlying pointer to have its reference count +/// decremented the next time PyO3 acquires the GIL. +/// +/// However, if the `pyo3_disable_reference_pool` conditional compilation flag +/// is enabled, it will abort the process. impl Drop for Py { + #[track_caller] fn drop(&mut self) { unsafe { gil::register_decref(self.0); @@ -2039,7 +2057,9 @@ mod tests { Py::from(native) }); - assert_eq!(Python::with_gil(|py| dict.get_refcnt(py)), 1); + Python::with_gil(move |py| { + assert_eq!(dict.get_refcnt(py), 1); + }); } #[test] diff --git a/src/marker.rs b/src/marker.rs index b6821deb..072b882c 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -1178,7 +1178,6 @@ impl<'unbound> Python<'unbound> { mod tests { use super::*; use crate::types::{IntoPyDict, PyList}; - use std::sync::Arc; #[test] fn test_eval() { @@ -1264,11 +1263,12 @@ mod tests { }); } + #[cfg(not(pyo3_disable_reference_pool))] #[test] fn test_allow_threads_pass_stuff_in() { let list = Python::with_gil(|py| PyList::new_bound(py, vec!["foo", "bar"]).unbind()); let mut v = vec![1, 2, 3]; - let a = Arc::new(String::from("foo")); + let a = std::sync::Arc::new(String::from("foo")); Python::with_gil(|py| { py.allow_threads(|| { diff --git a/src/pybacked.rs b/src/pybacked.rs index e0bacb86..ea5face5 100644 --- a/src/pybacked.rs +++ b/src/pybacked.rs @@ -13,7 +13,7 @@ use crate::{ /// A wrapper around `str` where the storage is owned by a Python `bytes` or `str` object. /// /// This type gives access to the underlying data via a `Deref` implementation. -#[derive(Clone)] +#[cfg_attr(feature = "py-clone", derive(Clone))] pub struct PyBackedStr { #[allow(dead_code)] // only held so that the storage is not dropped storage: Py, @@ -88,7 +88,7 @@ impl FromPyObject<'_> for PyBackedStr { /// A wrapper around `[u8]` where the storage is either owned by a Python `bytes` object, or a Rust `Box<[u8]>`. /// /// This type gives access to the underlying data via a `Deref` implementation. -#[derive(Clone)] +#[cfg_attr(feature = "py-clone", derive(Clone))] pub struct PyBackedBytes { #[allow(dead_code)] // only held so that the storage is not dropped storage: PyBackedBytesStorage, @@ -96,7 +96,7 @@ pub struct PyBackedBytes { } #[allow(dead_code)] -#[derive(Clone)] +#[cfg_attr(feature = "py-clone", derive(Clone))] enum PyBackedBytesStorage { Python(Py), Rust(Arc<[u8]>), @@ -336,6 +336,7 @@ mod test { is_sync::(); } + #[cfg(feature = "py-clone")] #[test] fn test_backed_str_clone() { Python::with_gil(|py| { @@ -398,6 +399,7 @@ mod test { }) } + #[cfg(feature = "py-clone")] #[test] fn test_backed_bytes_from_bytes_clone() { Python::with_gil(|py| { @@ -410,6 +412,7 @@ mod test { }); } + #[cfg(feature = "py-clone")] #[test] fn test_backed_bytes_from_bytearray_clone() { Python::with_gil(|py| { diff --git a/src/tests/hygiene/pyclass.rs b/src/tests/hygiene/pyclass.rs index 0bdb280d..34b30a8c 100644 --- a/src/tests/hygiene/pyclass.rs +++ b/src/tests/hygiene/pyclass.rs @@ -25,7 +25,7 @@ pub struct Bar { a: u8, #[pyo3(get, set)] b: Foo, - #[pyo3(get, set)] + #[pyo3(set)] c: ::std::option::Option>, } diff --git a/src/types/capsule.rs b/src/types/capsule.rs index 2851970b..9b9445cd 100644 --- a/src/types/capsule.rs +++ b/src/types/capsule.rs @@ -489,7 +489,7 @@ mod tests { cap.into() }); - Python::with_gil(|py| { + Python::with_gil(move |py| { let f = unsafe { cap.bind(py).reference:: u32>() }; assert_eq!(f(123), 123); }); @@ -553,7 +553,7 @@ mod tests { cap.into() }); - Python::with_gil(|py| { + Python::with_gil(move |py| { let ctx: &Vec = unsafe { cap.bind(py).reference() }; assert_eq!(ctx, &[1, 2, 3, 4]); }) @@ -572,7 +572,7 @@ mod tests { cap.into() }); - Python::with_gil(|py| { + Python::with_gil(move |py| { let ctx_ptr: *mut c_void = cap.bind(py).context().unwrap(); let ctx = unsafe { *Box::from_raw(ctx_ptr.cast::<&Vec>()) }; assert_eq!(ctx, &vec![1_u8, 2, 3, 4]); @@ -589,7 +589,7 @@ mod tests { context.send(true).unwrap(); } - Python::with_gil(|py| { + Python::with_gil(move |py| { let name = CString::new("foo").unwrap(); let cap = PyCapsule::new_bound_with_destructor(py, 0, Some(name), destructor).unwrap(); cap.set_context(Box::into_raw(Box::new(tx)).cast()).unwrap(); diff --git a/src/types/iterator.rs b/src/types/iterator.rs index 4562efde..1835f484 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -195,7 +195,7 @@ mod tests { ); }); - Python::with_gil(|py| { + Python::with_gil(move |py| { assert_eq!(count, obj.get_refcnt(py)); }); } diff --git a/src/types/sequence.rs b/src/types/sequence.rs index f75d8519..a5765ebc 100644 --- a/src/types/sequence.rs +++ b/src/types/sequence.rs @@ -823,7 +823,7 @@ mod tests { assert!(seq.get_item(1).unwrap().as_ptr() == obj.as_ptr()); }); - Python::with_gil(|py| { + Python::with_gil(move |py| { assert_eq!(1, obj.get_refcnt(py)); }); } diff --git a/tests/test_buffer_protocol.rs b/tests/test_buffer_protocol.rs index dca90080..700bcdcd 100644 --- a/tests/test_buffer_protocol.rs +++ b/tests/test_buffer_protocol.rs @@ -122,7 +122,7 @@ fn test_releasebuffer_unraisable_error() { let capture = UnraisableCapture::install(py); let instance = Py::new(py, ReleaseBufferError {}).unwrap(); - let env = [("ob", instance.clone())].into_py_dict_bound(py); + let env = [("ob", instance.clone_ref(py))].into_py_dict_bound(py); assert!(capture.borrow(py).capture.is_none()); diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index a3f1e2fc..5adca3f1 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -48,4 +48,6 @@ fn test_py_as_bytes() { let data = Python::with_gil(|py| pyobj.as_bytes(py)); assert_eq!(data, b"abc"); + + Python::with_gil(move |_py| drop(pyobj)); } diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index b7bee263..d0e74537 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -172,6 +172,7 @@ fn empty_class_in_module() { }); } +#[cfg(feature = "py-clone")] #[pyclass] struct ClassWithObjectField { // It used to be that PyObject was not supported with (get, set) @@ -180,6 +181,7 @@ struct ClassWithObjectField { value: PyObject, } +#[cfg(feature = "py-clone")] #[pymethods] impl ClassWithObjectField { #[new] @@ -188,6 +190,7 @@ impl ClassWithObjectField { } } +#[cfg(feature = "py-clone")] #[test] fn class_with_object_field() { Python::with_gil(|py| { @@ -229,7 +232,7 @@ impl UnsendableChild { } fn test_unsendable() -> PyResult<()> { - let obj = Python::with_gil(|py| -> PyResult<_> { + let (keep_obj_here, obj) = Python::with_gil(|py| -> PyResult<_> { let obj: Py = PyType::new_bound::(py).call1((5,))?.extract()?; // Accessing the value inside this thread should not panic @@ -241,14 +244,13 @@ fn test_unsendable() -> PyResult<()> { .is_err(); assert!(!caught_panic); - Ok(obj) - })?; - let keep_obj_here = obj.clone(); + Ok((obj.clone_ref(py), obj)) + })?; let caught_panic = std::thread::spawn(move || { // This access must panic - Python::with_gil(|py| { + Python::with_gil(move |py| { obj.borrow(py); }); }) @@ -549,6 +551,8 @@ fn access_frozen_class_without_gil() { }); assert_eq!(py_counter.get().value.load(Ordering::Relaxed), 1); + + Python::with_gil(move |_py| drop(py_counter)); } #[test] diff --git a/tests/test_class_conversion.rs b/tests/test_class_conversion.rs index ede8928f..a46132b9 100644 --- a/tests/test_class_conversion.rs +++ b/tests/test_class_conversion.rs @@ -54,12 +54,14 @@ impl SubClass { } } +#[cfg(feature = "py-clone")] #[pyclass] struct PolymorphicContainer { #[pyo3(get, set)] inner: Py, } +#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_stores_base_class() { Python::with_gil(|py| { @@ -76,6 +78,7 @@ fn test_polymorphic_container_stores_base_class() { }); } +#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_stores_sub_class() { Python::with_gil(|py| { @@ -103,6 +106,7 @@ fn test_polymorphic_container_stores_sub_class() { }); } +#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_does_not_accept_other_types() { Python::with_gil(|py| { diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 11d9221c..b95abd4a 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -445,6 +445,7 @@ impl DropDuringTraversal { } } +#[cfg(not(pyo3_disable_reference_pool))] #[test] fn drop_during_traversal_with_gil() { let drop_called = Arc::new(AtomicBool::new(false)); @@ -476,6 +477,7 @@ fn drop_during_traversal_with_gil() { assert!(drop_called.load(Ordering::Relaxed)); } +#[cfg(not(pyo3_disable_reference_pool))] #[test] fn drop_during_traversal_without_gil() { let drop_called = Arc::new(AtomicBool::new(false)); diff --git a/tests/test_methods.rs b/tests/test_methods.rs index 37f3b2d8..615e2dba 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -874,6 +874,7 @@ fn test_from_sequence() { }); } +#[cfg(feature = "py-clone")] #[pyclass] struct r#RawIdents { #[pyo3(get, set)] @@ -882,6 +883,7 @@ struct r#RawIdents { r#subsubtype: PyObject, } +#[cfg(feature = "py-clone")] #[pymethods] impl r#RawIdents { #[new] @@ -946,6 +948,7 @@ impl r#RawIdents { } } +#[cfg(feature = "py-clone")] #[test] fn test_raw_idents() { Python::with_gil(|py| { diff --git a/tests/test_no_imports.rs b/tests/test_no_imports.rs index 3509a11f..89d54f4e 100644 --- a/tests/test_no_imports.rs +++ b/tests/test_no_imports.rs @@ -143,12 +143,14 @@ fn test_basic() { }); } +#[cfg(feature = "py-clone")] #[pyo3::pyclass] struct NewClassMethod { #[pyo3(get)] cls: pyo3::PyObject, } +#[cfg(feature = "py-clone")] #[pyo3::pymethods] impl NewClassMethod { #[new] @@ -160,6 +162,7 @@ impl NewClassMethod { } } +#[cfg(feature = "py-clone")] #[test] fn test_new_class_method() { pyo3::Python::with_gil(|py| { diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index 9627f06c..8adba35c 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -248,12 +248,14 @@ fn test_inplace_repeat() { // Check that #[pyo3(get, set)] works correctly for Vec +#[cfg(feature = "py-clone")] #[pyclass] struct GenericList { #[pyo3(get, set)] items: Vec, } +#[cfg(feature = "py-clone")] #[test] fn test_generic_list_get() { Python::with_gil(|py| { @@ -266,6 +268,7 @@ fn test_generic_list_get() { }); } +#[cfg(feature = "py-clone")] #[test] fn test_generic_list_set() { Python::with_gil(|py| { diff --git a/tests/test_serde.rs b/tests/test_serde.rs index f1d5bee4..9e97946b 100644 --- a/tests/test_serde.rs +++ b/tests/test_serde.rs @@ -11,7 +11,7 @@ mod test_serde { } #[pyclass] - #[derive(Debug, Clone, Serialize, Deserialize)] + #[derive(Debug, Serialize, Deserialize)] struct User { username: String, group: Option>, @@ -27,7 +27,8 @@ mod test_serde { }; let friend2 = User { username: "friend 2".into(), - ..friend1.clone() + group: None, + friends: vec![], }; let user = Python::with_gil(|py| {