Skip to content

Commit

Permalink
Leak instead of abort on dropping with GIL and global reference pool …
Browse files Browse the repository at this point in the history
…as the friendlier alternative that is consistent with how we treat unsendable objects.
  • Loading branch information
adamreichold committed Apr 19, 2024
1 parent 1555462 commit 698b312
Show file tree
Hide file tree
Showing 3 changed files with 2 additions and 11 deletions.
4 changes: 2 additions & 2 deletions guide/src/performance.md
Expand Up @@ -101,7 +101,7 @@ impl PartialEq<Foo> for FooBound<'_> {

PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Clone for Py<T>` and `impl Drop for Py<T>` 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<T>` 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<T>` 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<T>` returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code

Expand All @@ -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::*;
Expand Down
8 changes: 0 additions & 8 deletions 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")]
Expand Down Expand Up @@ -477,18 +475,12 @@ pub unsafe fn register_incref(obj: NonNull<ffi::PyObject>) {
///
/// # Safety
/// The object must be an owned Python reference.
#[track_caller]
pub unsafe fn register_decref(obj: NonNull<ffi::PyObject>) {
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.");
}
}
}

Expand Down
1 change: 0 additions & 1 deletion src/instance.rs
Expand Up @@ -1813,7 +1813,6 @@ impl<T> Clone for Py<T> {

/// Dropping a `Py` instance decrements the reference count on the object by 1.
impl<T> Drop for Py<T> {
#[track_caller]
fn drop(&mut self) {
unsafe {
gil::register_decref(self.0);
Expand Down

0 comments on commit 698b312

Please sign in to comment.