From 698b31222dfd9e7f13cce15bcdd3fec897535504 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Fri, 19 Apr 2024 14:17:03 +0200 Subject: [PATCH] Leak instead of abort on dropping with GIL and global reference pool as the friendlier alternative that is consistent with how we treat unsendable objects. --- guide/src/performance.md | 4 ++-- src/gil.rs | 8 -------- src/instance.rs | 1 - 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/guide/src/performance.md b/guide/src/performance.md index f302b779ef5..5b789316d29 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -101,7 +101,7 @@ impl PartialEq for FooBound<'_> { PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Clone for Py` and `impl Drop for Py` being called without the currently 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 disabling default features and not enabling the `reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Clone` and `Drop` implementations for `Py` which are often necessary to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementations but panic when `Clone` is called without the GIL being held and abort when `Drop` is called without the GIL being held. +This functionality can be avoided by disabling default features and not enabling the `reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Clone` and `Drop` implementations for `Py` which are often necessary to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementations but panic when `Clone` is called without the GIL being held and leak when `Drop` is called without the GIL being held. 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` returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code @@ -119,7 +119,7 @@ Python::with_gil(|py| { }); ``` -will abort if the list not explicitly disposed via +will leak `numbers` if the list not explicitly disposed via ```rust # use pyo3::prelude::*; diff --git a/src/gil.rs b/src/gil.rs index 10787d1a8f4..f9092c242d2 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -1,8 +1,6 @@ //! Interaction with Python's global interpreter lock use crate::impl_::not_send::{NotSend, NOT_SEND}; -#[cfg(not(feature = "reference-pool"))] -use crate::impl_::panic::PanicTrap; use crate::{ffi, Python}; use parking_lot::Once; #[cfg(feature = "reference-pool")] @@ -477,18 +475,12 @@ 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(feature = "reference-pool")] POOL.register_decref(obj); - #[cfg(not(feature = "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."); - } } } diff --git a/src/instance.rs b/src/instance.rs index 0050c1e38f5..9a77b103c70 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1813,7 +1813,6 @@ impl Clone for Py { /// Dropping a `Py` instance decrements the reference count on the object by 1. impl Drop for Py { - #[track_caller] fn drop(&mut self) { unsafe { gil::register_decref(self.0);