Skip to content

Commit 2973477

Browse files
adamreicholdadamreichold
committed
Add Python::with_pool as a safe alternative to GILPool usage.
1 parent 3ec966d commit 2973477

File tree

8 files changed

+126
-41
lines changed

8 files changed

+126
-41
lines changed

‎guide/src/memory.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,11 @@ this is unsafe.
9595
# fn main() -> PyResult<()> {
9696
Python::with_gil(|py| -> PyResult<()> {
9797
for _ in 0..10 {
98-
let pool = unsafe { py.new_pool() };
99-
let py = pool.python();
100-
let hello: &PyString = py.eval("\"Hello World!\"", None, None)?.extract()?;
101-
println!("Python says: {}", hello);
98+
py.with_pool(|py| {
99+
let hello: &PyString = py.eval("\"Hello World!\"", None, None)?.extract()?;
100+
println!("Python says: {}", hello);
101+
Ok::<_, PyErr>(())
102+
})?;
102103
}
103104
Ok(())
104105
})?;

‎newsfragments/3167.added.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Add `Python::with_pool` as a safe alternative to `Python::new_pool`

‎newsfragments/3167.changed.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Deprecate `Python::new_pool` as the safe alternative `Python::with_pool` was added.

‎src/gil.rs

Lines changed: 37 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -493,11 +493,11 @@ mod tests {
493493
fn get_object(py: Python<'_>) -> PyObject {
494494
// Convenience function for getting a single unique object, using `new_pool` so as to leave
495495
// the original pool state unchanged.
496-
let pool = unsafe { py.new_pool() };
497-
let py = pool.python();
498-
499-
let obj = py.eval("object()", None, None).unwrap();
500-
obj.to_object(py)
496+
py.with_pool(|py| {
497+
// TODO: abuse old py????
498+
let obj = py.eval("object()", None, None).unwrap();
499+
obj.to_object(py)
500+
})
501501
}
502502

503503
fn owned_object_count() -> usize {
@@ -520,24 +520,22 @@ mod tests {
520520
fn test_owned() {
521521
Python::with_gil(|py| {
522522
let obj = get_object(py);
523-
let obj_ptr = obj.as_ptr();
524523
// Ensure that obj does not get freed
525524
let _ref = obj.clone_ref(py);
526525

527-
unsafe {
528-
{
529-
let pool = py.new_pool();
530-
gil::register_owned(pool.python(), NonNull::new_unchecked(obj.into_ptr()));
531-
532-
assert_eq!(owned_object_count(), 1);
533-
assert_eq!(ffi::Py_REFCNT(obj_ptr), 2);
534-
}
535-
{
536-
let _pool = py.new_pool();
537-
assert_eq!(owned_object_count(), 0);
538-
assert_eq!(ffi::Py_REFCNT(obj_ptr), 1);
526+
py.with_pool(|py| {
527+
unsafe {
528+
gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
539529
}
540-
}
530+
531+
assert_eq!(owned_object_count(), 1);
532+
assert_eq!(_ref.get_refcnt(py), 2);
533+
});
534+
535+
py.with_pool(|py| {
536+
assert_eq!(owned_object_count(), 0);
537+
assert_eq!(_ref.get_refcnt(py), 1);
538+
});
541539
})
542540
}
543541

@@ -547,30 +545,32 @@ mod tests {
547545
let obj = get_object(py);
548546
// Ensure that obj does not get freed
549547
let _ref = obj.clone_ref(py);
550-
let obj_ptr = obj.as_ptr();
551548

552-
unsafe {
553-
{
554-
let _pool = py.new_pool();
555-
assert_eq!(owned_object_count(), 0);
549+
py.with_pool(|py| {
550+
assert_eq!(owned_object_count(), 0);
556551

552+
unsafe {
557553
gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
554+
}
555+
556+
assert_eq!(owned_object_count(), 1);
557+
assert_eq!(_ref.get_refcnt(py), 2);
558+
559+
py.with_pool(|py| {
560+
let obj = get_object(py);
558561

559-
assert_eq!(owned_object_count(), 1);
560-
assert_eq!(ffi::Py_REFCNT(obj_ptr), 2);
561-
{
562-
let _pool = py.new_pool();
563-
let obj = get_object(py);
562+
unsafe {
564563
gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
565-
assert_eq!(owned_object_count(), 2);
566564
}
567-
assert_eq!(owned_object_count(), 1);
568-
}
569-
{
570-
assert_eq!(owned_object_count(), 0);
571-
assert_eq!(ffi::Py_REFCNT(obj_ptr), 1);
572-
}
573-
}
565+
566+
assert_eq!(owned_object_count(), 2);
567+
});
568+
569+
assert_eq!(owned_object_count(), 1);
570+
});
571+
572+
assert_eq!(owned_object_count(), 0);
573+
assert_eq!(_ref.get_refcnt(py), 1);
574574
});
575575
}
576576

‎src/marker.rs

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,45 @@ impl Python<'_> {
354354
{
355355
f(gil::ensure_gil_unchecked().python())
356356
}
357+
358+
/// Create a new pool for managing PyO3's owned references.
359+
///
360+
/// When the given closure returns, all PyO3 owned references created by it will
361+
/// all have their Python reference counts decremented, potentially allowing Python to drop
362+
/// the corresponding Python objects.
363+
///
364+
/// Typical usage of PyO3 will not need this API, as [`Python::with_gil`] automatically creates
365+
/// a `GILPool` where appropriate.
366+
///
367+
/// Advanced uses of PyO3 which perform long-running tasks which never free the GIL may need
368+
/// to use this API to clear memory, as PyO3 usually does not clear memory until the GIL is
369+
/// released.
370+
///
371+
/// # Examples
372+
///
373+
/// ```rust
374+
/// # use pyo3::prelude::*;
375+
/// Python::with_gil(|py| {
376+
/// // Some long-running process like a webserver, which never releases the GIL.
377+
/// loop {
378+
/// // Create a new pool, so that PyO3 can clear memory at the end of the loop.
379+
/// py.with_pool(|py| {
380+
/// // do stuff...
381+
/// });
382+
/// # break; // Exit the loop so that doctest terminates!
383+
/// }
384+
/// });
385+
/// ```
386+
#[inline]
387+
pub fn with_pool<F, R>(self, f: F) -> R
388+
where
389+
F: for<'py> FnOnce(Python<'py>) -> R + Ungil,
390+
R: Ungil,
391+
{
392+
let pool = unsafe { GILPool::new() };
393+
394+
f(pool.python())
395+
}
357396
}
358397

359398
impl<'py> Python<'py> {
@@ -785,9 +824,13 @@ impl<'py> Python<'py> {
785824
/// to use this API to clear memory, as PyO3 usually does not clear memory until the GIL is
786825
/// released.
787826
///
827+
/// [`Python::with_pool`] provides a safe alternative albeit adding the requirement that all
828+
/// types interacting with the new pool are marked with the `Ungil` trait.
829+
///
788830
/// # Examples
789831
///
790832
/// ```rust
833+
/// # #![allow(deprecated)]
791834
/// # use pyo3::prelude::*;
792835
/// Python::with_gil(|py| {
793836
/// // Some long-running process like a webserver, which never releases the GIL.
@@ -827,6 +870,7 @@ impl<'py> Python<'py> {
827870
///
828871
/// [`.python()`]: crate::GILPool::python
829872
#[inline]
873+
#[deprecated(since = "0.19.0", note = "Please use `with_pool` instead.")]
830874
pub unsafe fn new_pool(self) -> GILPool {
831875
GILPool::new()
832876
}

‎tests/test_compile_error.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ fn _test_compile_errors() {
4545
t.compile_fail("tests/ui/invalid_pymethod_names.rs");
4646
t.compile_fail("tests/ui/invalid_pymodule_args.rs");
4747
t.compile_fail("tests/ui/reject_generics.rs");
48+
t.compile_fail("tests/ui/with_pool.rs");
4849

4950
tests_rust_1_49(&t);
5051
tests_rust_1_56(&t);

‎tests/ui/with_pool.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
use pyo3::prelude::*;
2+
3+
fn reuse_old_token_in_with_pool(old_py: Python<'_>) {
4+
old_py.with_pool(|new_py| { drop(old_py); });
5+
}
6+
7+
fn main() {
8+
Python::with_gil(|py| {
9+
reuse_old_token_in_with_pool(py);
10+
})
11+
}

‎tests/ui/with_pool.stderr

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
error[E0277]: `*mut pyo3::Python<'static>` cannot be shared between threads safely
2+
--> tests/ui/with_pool.rs:4:22
3+
|
4+
4 | old_py.with_pool(|new_py| { drop(old_py); });
5+
| --------- ^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut pyo3::Python<'static>` cannot be shared between threads safely
6+
| |
7+
| required by a bound introduced by this call
8+
|
9+
= help: within `pyo3::Python<'_>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>`
10+
= note: required because it appears within the type `PhantomData<*mut Python<'static>>`
11+
= note: required because it appears within the type `NotSend`
12+
= note: required because it appears within the type `(&EnsureGIL, NotSend)`
13+
= note: required because it appears within the type `PhantomData<(&EnsureGIL, NotSend)>`
14+
= note: required because it appears within the type `Python<'_>`
15+
= note: required for `&pyo3::Python<'_>` to implement `Send`
16+
note: required because it's used within this closure
17+
--> tests/ui/with_pool.rs:4:22
18+
|
19+
4 | old_py.with_pool(|new_py| { drop(old_py); });
20+
| ^^^^^^^^
21+
= note: required for `[closure@$DIR/tests/ui/with_pool.rs:4:22: 4:30]` to implement `Ungil`
22+
note: required by a bound in `pyo3::Python::<'_>::with_pool`
23+
--> src/marker.rs
24+
|
25+
| F: for<'py> FnOnce(Python<'py>) -> R + Ungil,
26+
| ^^^^^ required by this bound in `Python::<'_>::with_pool`

0 commit comments

Comments
 (0)