From dcba984b51be253cb5d385e72008ee9578807504 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 15 Mar 2024 10:25:27 +0000 Subject: [PATCH] deprecate `GILPool` (#3947) * deprecate `GILPool` * review: adamreichold * fix deprecation warnings in tests --- guide/src/memory.md | 14 +++++++++++++- guide/src/migration.md | 2 ++ newsfragments/3947.changed.md | 1 + src/gil.rs | 30 +++++++++++++++++++++--------- src/impl_/trampoline.rs | 8 +++++++- src/lib.rs | 1 + src/marker.rs | 32 ++++++++++++++++++++++++++------ tests/ui/deprecations.rs | 1 + tests/ui/deprecations.stderr | 32 ++++++++++++++++---------------- 9 files changed, 88 insertions(+), 33 deletions(-) create mode 100644 newsfragments/3947.changed.md diff --git a/guide/src/memory.md b/guide/src/memory.md index fe98184e..78f9f348 100644 --- a/guide/src/memory.md +++ b/guide/src/memory.md @@ -83,6 +83,13 @@ bound to the `GILPool`, not the for loop. The `GILPool` isn't dropped until the end of the `with_gil()` closure, at which point the 10 copies of `hello` are finally released to the Python garbage collector. +
+⚠️ Warning: `GILPool` is no longer the preferred way to manage memory with PyO3 🛠️ + +PyO3 0.21 has introduced a new API known as the Bound API, which doesn't have the same surprising results. Instead, each `Bound` smart pointer releases the Python reference immediately on drop. See [the smart pointer types](./types.md#pyo3s-smart-pointers) for more details. +
+ + In general we don't want unbounded memory growth during loops! One workaround is to acquire and release the GIL with each iteration of the loop. @@ -114,6 +121,7 @@ this is unsafe. # fn main() -> PyResult<()> { Python::with_gil(|py| -> PyResult<()> { for _ in 0..10 { + #[allow(deprecated)] // `new_pool` is not needed in code not using the GIL Refs API let pool = unsafe { py.new_pool() }; let py = pool.python(); #[allow(deprecated)] // py.eval() is part of the GIL Refs API @@ -146,7 +154,11 @@ function call, releasing objects when the function returns. Most functions only a few objects, meaning this doesn't have a significant impact. Occasionally functions with long complex loops may need to use `Python::new_pool` as shown above. -This behavior may change in future, see [issue #1056](https://github.com/PyO3/pyo3/issues/1056). +
+⚠️ Warning: `GILPool` is no longer the preferred way to manage memory with PyO3 🛠️ + +PyO3 0.21 has introduced a new API known as the Bound API, which doesn't have the same surprising results. Instead, each `Bound` smart pointer releases the Python reference immediately on drop. See [the smart pointer types](./types.md#pyo3s-smart-pointers) for more details. +
## GIL-independent memory diff --git a/guide/src/migration.md b/guide/src/migration.md index 1c5ca597..8ea3f073 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -280,6 +280,8 @@ The expectation is that in 0.22 `extract_bound` will have the default implementa As a final step of migration, deactivating the `gil-refs` feature will set up code for best performance and is intended to set up a forward-compatible API for PyO3 0.22. +At this point code which needed to manage GIL Ref memory can safely remove uses of `GILPool` (which are constructed by calls to `Python::new_pool` and `Python::with_pool`). Deprecation warnings will highlight these cases. + There is one notable API removed when this feature is disabled. `FromPyObject` trait implementations for types which borrow directly from the input data cannot be implemented by PyO3 without GIL Refs (while the migration is ongoing). These types are `&str`, `Cow<'_, str>`, `&[u8]`, `Cow<'_, u8>`. To ease pain during migration, these types instead implement a new temporary trait `FromPyObjectBound` which is the expected future form of `FromPyObject`. The new temporary trait ensures is that `obj.extract::<&str>()` continues to work (with the new constraint that the extracted value now depends on the input `obj` lifetime), as well for these types in `#[pyfunction]` arguments. diff --git a/newsfragments/3947.changed.md b/newsfragments/3947.changed.md new file mode 100644 index 00000000..4618e937 --- /dev/null +++ b/newsfragments/3947.changed.md @@ -0,0 +1 @@ +Deprecate `GILPool`, `Python::with_pool`, and `Python::new_pool`. diff --git a/src/gil.rs b/src/gil.rs index 91c3d1cd..5ca3167e 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -139,6 +139,7 @@ where ffi::Py_InitializeEx(0); // Safety: the GIL is already held because of the Py_IntializeEx call. + #[allow(deprecated)] // TODO: remove this with the GIL Refs feature in 0.22 let pool = GILPool::new(); // Import the threading module - this ensures that it will associate this thread as the "main" @@ -160,6 +161,7 @@ where /// RAII type that represents the Global Interpreter Lock acquisition. pub(crate) struct GILGuard { gstate: ffi::PyGILState_STATE, + #[allow(deprecated)] // TODO: remove this with the gil-refs feature in 0.22 pool: mem::ManuallyDrop, } @@ -222,6 +224,7 @@ impl GILGuard { } let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL + #[allow(deprecated)] let pool = unsafe { mem::ManuallyDrop::new(GILPool::new()) }; Some(GILGuard { gstate, pool }) @@ -358,6 +361,13 @@ impl Drop for LockGIL { /// /// [Memory Management]: https://pyo3.rs/main/memory.html#gil-bound-memory +#[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "`GILPool` has no function if PyO3's deprecated GIL Refs API is not used" + ) +)] pub struct GILPool { /// Initial length of owned objects and anys. /// `Option` is used since TSL can be broken when `new` is called from `atexit`. @@ -365,6 +375,7 @@ pub struct GILPool { _not_send: NotSend, } +#[allow(deprecated)] impl GILPool { /// Creates a new [`GILPool`]. This function should only ever be called with the GIL held. /// @@ -401,6 +412,7 @@ impl GILPool { } } +#[allow(deprecated)] impl Drop for GILPool { fn drop(&mut self) { if let Some(start) = self.start { @@ -506,21 +518,17 @@ fn decrement_gil_count() { #[cfg(test)] mod tests { - use super::{gil_is_acquired, GILPool, GIL_COUNT, OWNED_OBJECTS, POOL}; + #[allow(deprecated)] + use super::GILPool; + use super::{gil_is_acquired, GIL_COUNT, OWNED_OBJECTS, POOL}; use crate::types::any::PyAnyMethods; - use crate::{ffi, gil, PyObject, Python, ToPyObject}; + use crate::{ffi, gil, PyObject, Python}; #[cfg(not(target_arch = "wasm32"))] use parking_lot::{const_mutex, Condvar, Mutex}; use std::ptr::NonNull; fn get_object(py: Python<'_>) -> PyObject { - // Convenience function for getting a single unique object, using `new_pool` so as to leave - // the original pool state unchanged. - let pool = unsafe { py.new_pool() }; - let py = pool.python(); - - let obj = py.eval_bound("object()", None, None).unwrap(); - obj.to_object(py) + py.eval_bound("object()", None, None).unwrap().unbind() } fn owned_object_count() -> usize { @@ -556,6 +564,7 @@ mod tests { } #[test] + #[allow(deprecated)] fn test_owned() { Python::with_gil(|py| { let obj = get_object(py); @@ -581,6 +590,7 @@ mod tests { } #[test] + #[allow(deprecated)] fn test_owned_nested() { Python::with_gil(|py| { let obj = get_object(py); @@ -666,6 +676,7 @@ mod tests { } #[test] + #[allow(deprecated)] fn test_gil_counts() { // Check with_gil and GILPool both increase counts correctly let get_gil_count = || GIL_COUNT.with(|c| c.get()); @@ -906,6 +917,7 @@ mod tests { 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(); // Rebuild obj so that it can be dropped diff --git a/src/impl_/trampoline.rs b/src/impl_/trampoline.rs index 4b4eac17..4d77f329 100644 --- a/src/impl_/trampoline.rs +++ b/src/impl_/trampoline.rs @@ -9,9 +9,11 @@ use std::{ panic::{self, UnwindSafe}, }; +#[allow(deprecated)] +use crate::gil::GILPool; use crate::{ callback::PyCallbackOutput, ffi, ffi_ptr_ext::FfiPtrExt, impl_::panic::PanicTrap, - methods::IPowModulo, panic::PanicException, types::PyModule, GILPool, Py, PyResult, Python, + methods::IPowModulo, panic::PanicException, types::PyModule, Py, PyResult, Python, }; #[inline] @@ -174,6 +176,8 @@ where R: PyCallbackOutput, { let trap = PanicTrap::new("uncaught panic at ffi boundary"); + // Necessary to construct a pool until PyO3 0.22 when the GIL Refs API is fully disabled + #[allow(deprecated)] let pool = unsafe { GILPool::new() }; let py = pool.python(); let out = panic_result_into_callback_output( @@ -219,6 +223,8 @@ where F: for<'py> FnOnce(Python<'py>) -> PyResult<()> + UnwindSafe, { let trap = PanicTrap::new("uncaught panic at ffi boundary"); + // Necessary to construct a pool until PyO3 0.22 when the GIL Refs API is fully disabled + #[allow(deprecated)] let pool = GILPool::new(); let py = pool.python(); if let Err(py_err) = panic::catch_unwind(move || body(py)) diff --git a/src/lib.rs b/src/lib.rs index a7c03d1b..dab48fbe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -319,6 +319,7 @@ pub use crate::conversion::{FromPyPointer, PyTryFrom, PyTryInto}; pub use crate::err::{ DowncastError, DowncastIntoError, PyDowncastError, PyErr, PyErrArguments, PyResult, ToPyErr, }; +#[allow(deprecated)] pub use crate::gil::GILPool; #[cfg(not(PyPy))] pub use crate::gil::{prepare_freethreaded_python, with_embedded_python_interpreter}; diff --git a/src/marker.rs b/src/marker.rs index 2a38b83c..08b90424 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -118,7 +118,7 @@ //! [`Py`]: crate::Py use crate::err::{self, PyDowncastError, PyErr, PyResult}; use crate::ffi_ptr_ext::FfiPtrExt; -use crate::gil::{GILGuard, GILPool, SuspendGIL}; +use crate::gil::{GILGuard, SuspendGIL}; use crate::impl_::not_send::NotSend; use crate::py_result_ext::PyResultExt; use crate::type_object::HasPyGilRef; @@ -127,9 +127,9 @@ use crate::types::{ PyAny, PyDict, PyEllipsis, PyModule, PyNone, PyNotImplemented, PyString, PyType, }; use crate::version::PythonVersionInfo; -#[allow(deprecated)] -use crate::FromPyPointer; use crate::{ffi, Bound, IntoPy, Py, PyNativeType, PyObject, PyTypeCheck, PyTypeInfo}; +#[allow(deprecated)] +use crate::{gil::GILPool, FromPyPointer}; use std::ffi::{CStr, CString}; use std::marker::PhantomData; use std::os::raw::c_int; @@ -1053,9 +1053,10 @@ impl<'py> Python<'py> { err::error_on_minusone(self, unsafe { ffi::PyErr_CheckSignals() }) } - /// Create a new pool for managing PyO3's owned references. + /// Create a new pool for managing PyO3's GIL Refs. This has no functional + /// use for code which does not use the deprecated GIL Refs API. /// - /// When this `GILPool` is dropped, all PyO3 owned references created after this `GILPool` will + /// When this `GILPool` is dropped, all GIL Refs created after this `GILPool` will /// all have their Python reference counts decremented, potentially allowing Python to drop /// the corresponding Python objects. /// @@ -1074,6 +1075,7 @@ impl<'py> Python<'py> { /// // Some long-running process like a webserver, which never releases the GIL. /// loop { /// // Create a new pool, so that PyO3 can clear memory at the end of the loop. + /// #[allow(deprecated)] // `new_pool` is not needed in code not using the GIL Refs API /// let pool = unsafe { py.new_pool() }; /// /// // It is recommended to *always* immediately set py to the pool's Python, to help @@ -1108,13 +1110,22 @@ impl<'py> Python<'py> { /// /// [`.python()`]: crate::GILPool::python #[inline] + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "code not using the GIL Refs API can safely remove use of `Python::new_pool`" + ) + )] + #[allow(deprecated)] pub unsafe fn new_pool(self) -> GILPool { GILPool::new() } } impl Python<'_> { - /// Creates a scope using a new pool for managing PyO3's owned references. + /// Creates a scope using a new pool for managing PyO3's GIL Refs. This has no functional + /// use for code which does not use the deprecated GIL Refs API. /// /// This is a safe alterantive to [`new_pool`][Self::new_pool] as /// it limits the closure to using the new GIL token at the cost of @@ -1131,6 +1142,7 @@ impl Python<'_> { /// // Some long-running process like a webserver, which never releases the GIL. /// loop { /// // Create a new scope, so that PyO3 can clear memory at the end of the loop. + /// #[allow(deprecated)] // `with_pool` is not needed in code not using the GIL Refs API /// py.with_pool(|py| { /// // do stuff... /// }); @@ -1167,6 +1179,14 @@ impl Python<'_> { /// }); /// ``` #[inline] + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "code not using the GIL Refs API can safely remove use of `Python::with_pool`" + ) + )] + #[allow(deprecated)] pub fn with_pool(&self, f: F) -> R where F: for<'py> FnOnce(Python<'py>) -> R + Ungil, diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index 1f3cc302..d82c406f 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -1,4 +1,5 @@ #![deny(deprecated)] +#![allow(dead_code)] use pyo3::prelude::*; use pyo3::types::{PyString, PyType}; diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index d4a3e01f..e54e1a4e 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -1,7 +1,7 @@ error: use of deprecated constant `pyo3::impl_::deprecations::PYMETHODS_NEW_DEPRECATED_FORM`: use `#[new]` instead of `#[__new__]` - --> tests/ui/deprecations.rs:11:7 + --> tests/ui/deprecations.rs:12:7 | -11 | #[__new__] +12 | #[__new__] | ^^^^^^^ | note: the lint level is defined here @@ -11,45 +11,45 @@ note: the lint level is defined here | ^^^^^^^^^^ error: use of deprecated struct `pyo3::PyCell`: `PyCell` was merged into `Bound`, use that instead; see the migration guide for more info - --> tests/ui/deprecations.rs:22:30 + --> tests/ui/deprecations.rs:23:30 | -22 | fn method_gil_ref(_slf: &PyCell) {} +23 | fn method_gil_ref(_slf: &PyCell) {} | ^^^^^^ error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:17:33 + --> tests/ui/deprecations.rs:18:33 | -17 | fn cls_method_gil_ref(_cls: &PyType) {} +18 | fn cls_method_gil_ref(_cls: &PyType) {} | ^ error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:22:29 + --> tests/ui/deprecations.rs:23:29 | -22 | fn method_gil_ref(_slf: &PyCell) {} +23 | fn method_gil_ref(_slf: &PyCell) {} | ^ error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:37:43 + --> tests/ui/deprecations.rs:38:43 | -37 | fn pyfunction_with_module_gil_ref(module: &PyModule) -> PyResult<&str> { +38 | fn pyfunction_with_module_gil_ref(module: &PyModule) -> PyResult<&str> { | ^ error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:47:19 + --> tests/ui/deprecations.rs:48:19 | -47 | fn module_gil_ref(m: &PyModule) -> PyResult<()> { +48 | fn module_gil_ref(m: &PyModule) -> PyResult<()> { | ^ error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:53:57 + --> tests/ui/deprecations.rs:54:57 | -53 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { +54 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { | ^ error: use of deprecated method `pyo3::methods::Extractor::>::is_python`: use `wrap_pyfunction_bound!` instead - --> tests/ui/deprecations.rs:78:13 + --> tests/ui/deprecations.rs:79:13 | -78 | let _ = wrap_pyfunction!(double, py); +79 | let _ = wrap_pyfunction!(double, py); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `wrap_pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)