diff --git a/Cargo.toml b/Cargo.toml index 4c5a083060d..aba8fd1bc98 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 e54b3b5cde2..81557e7f534 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 3fcfaca4bdc..91a6fb2c495 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 19f9b5d50ab..b79641a3803 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 07085a9e89c..6a25d40cedc 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 b37d83563e5..38a31f4d0ef 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 7e0420de22d..10b62002a02 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 c47a91deee5..b3d160fe6b1 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 00000000000..c9940f70f12 --- /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 00000000000..7f155ae04ef --- /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 59b9ff9686f..cede8836f35 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 24d3ae28124..54aff4d10de 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 2fa082ba16a..13527315e70 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 9f85296f661..14345b275c9 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 52e4b6c616a..29d4ac36294 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 29c4ffbe389..d4b92a4cff4 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 1c510b90be5..7835b9c79b6 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 b6821deb036..072b882c875 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 e0bacb86144..ea5face516b 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 0bdb280d066..34b30a8c6f4 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 c2b73a0d0f7..8628f9344aa 100644 --- a/src/types/capsule.rs +++ b/src/types/capsule.rs @@ -487,7 +487,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); }); @@ -551,7 +551,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]); }) @@ -570,7 +570,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]); @@ -587,7 +587,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 4562efde3f8..1835f484adf 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 f75d851973d..a5765ebc8b2 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 dca900808a8..700bcdcd206 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 a3f1e2fcafe..5adca3f154a 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 b7bee2638ca..d0e745377c8 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 ede8928f865..a46132b9586 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 11d9221c7ad..b95abd4adea 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 37f3b2d8bd6..615e2dba0af 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 3509a11f4be..89d54f4e057 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 9627f06ca75..8adba35c86a 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 f1d5bee4bee..9e97946b5f2 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| {