RFC: Add Python::with_pool as a safe alternative to GILPool usage.

guide/src/memory.md

@@ -95,10 +95,11 @@ this is unsafe.
 # fn main() -> PyResult<()> {
 Python::with_gil(|py| -> PyResult<()> {
     for _ in 0..10 {
-        let pool = unsafe { py.new_pool() };
-        let py = pool.python();
-        let hello: &PyString = py.eval("\"Hello World!\"", None, None)?.extract()?;
-        println!("Python says: {}", hello);
+        py.with_pool(|py| {
+            let hello: &PyString = py.eval("\"Hello World!\"", None, None)?.extract()?;
+            println!("Python says: {}", hello);
+            Ok::<_, PyErr>(())
+        })?;
     }
     Ok(())
 })?;

newsfragments/3167.added.md

@@ -0,0 +1 @@
+Add `Python::with_pool` as a safe alternative to `Python::new_pool`

newsfragments/3167.changed.md

@@ -0,0 +1 @@
+Deprecate `Python::new_pool` as the safe alternative `Python::with_pool` was added.

src/gil.rs

@@ -494,11 +494,10 @@ mod tests {
     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("object()", None, None).unwrap();
-        obj.to_object(py)
+        py.with_pool(|py| {
+            let obj = py.eval("object()", None, None).unwrap();
+            obj.to_object(py)
+        })
     }
 
     fn owned_object_count() -> usize {
@@ -521,24 +520,22 @@ mod tests {
     fn test_owned() {
         Python::with_gil(|py| {
             let obj = get_object(py);
-            let obj_ptr = obj.as_ptr();
             // Ensure that obj does not get freed
             let _ref = obj.clone_ref(py);
 
-            unsafe {
-                {
-                    let pool = py.new_pool();
-                    gil::register_owned(pool.python(), NonNull::new_unchecked(obj.into_ptr()));
-
-                    assert_eq!(owned_object_count(), 1);
-                    assert_eq!(ffi::Py_REFCNT(obj_ptr), 2);
-                }
-                {
-                    let _pool = py.new_pool();
-                    assert_eq!(owned_object_count(), 0);
-                    assert_eq!(ffi::Py_REFCNT(obj_ptr), 1);
+            py.with_pool(|py| {
+                unsafe {
+                    gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
                 }
-            }
+
+                assert_eq!(owned_object_count(), 1);
+                assert_eq!(_ref.get_refcnt(py), 2);
+            });
+
+            py.with_pool(|py| {
+                assert_eq!(owned_object_count(), 0);
+                assert_eq!(_ref.get_refcnt(py), 1);
+            });
         })
     }
 
@@ -548,30 +545,32 @@ mod tests {
             let obj = get_object(py);
             // Ensure that obj does not get freed
             let _ref = obj.clone_ref(py);
-            let obj_ptr = obj.as_ptr();
 
-            unsafe {
-                {
-                    let _pool = py.new_pool();
-                    assert_eq!(owned_object_count(), 0);
+            py.with_pool(|py| {
+                assert_eq!(owned_object_count(), 0);
 
+                unsafe {
                     gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
+                }
+
+                assert_eq!(owned_object_count(), 1);
+                assert_eq!(_ref.get_refcnt(py), 2);
+
+                py.with_pool(|py| {
+                    let obj = get_object(py);
 
-                    assert_eq!(owned_object_count(), 1);
-                    assert_eq!(ffi::Py_REFCNT(obj_ptr), 2);
-                    {
-                        let _pool = py.new_pool();
-                        let obj = get_object(py);
+                    unsafe {
                         gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
-                        assert_eq!(owned_object_count(), 2);
                     }
-                    assert_eq!(owned_object_count(), 1);
-                }
-                {
-                    assert_eq!(owned_object_count(), 0);
-                    assert_eq!(ffi::Py_REFCNT(obj_ptr), 1);
-                }
-            }
+
+                    assert_eq!(owned_object_count(), 2);
+                });
+
+                assert_eq!(owned_object_count(), 1);
+            });
+
+            assert_eq!(owned_object_count(), 0);
+            assert_eq!(_ref.get_refcnt(py), 1);
         });
     }
 

src/marker.rs

@@ -466,6 +466,45 @@ impl Python<'_> {
         // SAFETY: Either the GIL was already acquired or we just created a new `GILGuard`.
         f(Python::assume_gil_acquired())
     }
+
+    /// Create a new pool for managing PyO3's owned references.
+    ///
+    /// When the given closure returns, all PyO3 owned references created by it will
+    /// all have their Python reference counts decremented, potentially allowing Python to drop
+    /// the corresponding Python objects.
+    ///
+    /// Typical usage of PyO3 will not need this API, as [`Python::with_gil`] automatically creates
+    /// a `GILPool` where appropriate.
+    ///
+    /// Advanced uses of PyO3 which perform long-running tasks which never free the GIL may need
+    /// to use this API to clear memory, as PyO3 usually does not clear memory until the GIL is
+    /// released.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// # use pyo3::prelude::*;
+    /// Python::with_gil(|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.
+    ///         py.with_pool(|py| {
+    ///             // do stuff...
+    ///         });
+    /// #       break;  // Exit the loop so that doctest terminates!
+    ///     }
+    /// });
+    /// ```
+    #[inline]
+    pub fn with_pool<F, R>(self, f: F) -> R
+    where
+        F: for<'py> FnOnce(Python<'py>) -> R + Ungil,
+        R: Ungil,
+    {
+        let pool = unsafe { GILPool::new() };
+
+        f(pool.python())
+    }
 }
 
 impl<'py> Python<'py> {
@@ -897,9 +936,13 @@ impl<'py> Python<'py> {
     /// to use this API to clear memory, as PyO3 usually does not clear memory until the GIL is
     /// released.
     ///
+    /// [`Python::with_pool`] provides a safe alternative albeit adding the requirement that all
+    /// types interacting with the new pool are marked with the `Ungil` trait.
+    ///
     /// # Examples
     ///
     /// ```rust
+    /// # #![allow(deprecated)]
     /// # use pyo3::prelude::*;
     /// Python::with_gil(|py| {
     ///     // Some long-running process like a webserver, which never releases the GIL.
@@ -939,6 +982,7 @@ impl<'py> Python<'py> {
     ///
     /// [`.python()`]: crate::GILPool::python
     #[inline]
+    #[deprecated(since = "0.19.0", note = "Please use `with_pool` instead.")]
     pub unsafe fn new_pool(self) -> GILPool {
         GILPool::new()
     }

tests/test_compile_error.rs

@@ -38,4 +38,5 @@ fn test_compile_errors() {
     t.compile_fail("tests/ui/not_send2.rs");
     t.compile_fail("tests/ui/get_set_all.rs");
     t.compile_fail("tests/ui/traverse_bare_self.rs");
+    t.compile_fail("tests/ui/with_pool.rs");
 }

tests/ui/with_pool.rs

@@ -0,0 +1,11 @@
+use pyo3::prelude::*;
+
+fn reuse_old_token_in_with_pool(old_py: Python<'_>) {
+    old_py.with_pool(|new_py| { drop(old_py); });
+}
+
+fn main() {
+    Python::with_gil(|py| {
+        reuse_old_token_in_with_pool(py);
+    })
+}

tests/ui/with_pool.stderr

@@ -0,0 +1,26 @@
+error[E0277]: `*mut pyo3::Python<'static>` cannot be shared between threads safely
+ --> tests/ui/with_pool.rs:4:22
+  |
+4 |     old_py.with_pool(|new_py| { drop(old_py); });
+  |            --------- ^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut pyo3::Python<'static>` cannot be shared between threads safely
+  |            |
+  |            required by a bound introduced by this call
+  |
+  = help: within `pyo3::Python<'_>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>`
+  = note: required because it appears within the type `PhantomData<*mut Python<'static>>`
+  = note: required because it appears within the type `NotSend`
+  = note: required because it appears within the type `(&EnsureGIL, NotSend)`
+  = note: required because it appears within the type `PhantomData<(&EnsureGIL, NotSend)>`
+  = note: required because it appears within the type `Python<'_>`
+  = note: required for `&pyo3::Python<'_>` to implement `Send`
+note: required because it's used within this closure
+ --> tests/ui/with_pool.rs:4:22
+  |
+4 |     old_py.with_pool(|new_py| { drop(old_py); });
+  |                      ^^^^^^^^
+  = note: required for `[closure@$DIR/tests/ui/with_pool.rs:4:22: 4:30]` to implement `Ungil`
+note: required by a bound in `pyo3::Python::<'_>::with_pool`
+ --> src/marker.rs
+  |
+  |         F: for<'py> FnOnce(Python<'py>) -> R + Ungil,
+  |                                                ^^^^^ required by this bound in `Python::<'_>::with_pool`