Skip to content

Commit 9cc7862

Browse files
davidhewittdavidhewitt
authored
require PyCFunction::new_closure closures to be Sync (#6096)
* require `PyCFunction::new_closure` closures to be `Sync` * newsfragment * fix pyo3's own test suite
1 parent 8c9ad3a commit 9cc7862

9 files changed

Lines changed: 133 additions & 22 deletions

‎newsfragments/6096.fixed.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix missing `Sync` bound on closure type in `PyCFunction::new_closure`.

‎src/types/function.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ impl PyCFunction {
103103
closure: F,
104104
) -> PyResult<Bound<'py, Self>>
105105
where
106-
F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + 'static,
106+
F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static,
107107
for<'p> R: crate::impl_::callback::IntoPyCallbackOutput<'p, *mut ffi::PyObject>,
108108
{
109109
let name = name.unwrap_or(c"pyo3-closure");

‎tests/test_pyfunction.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#![warn(unsafe_op_in_unsafe_fn)]
33

44
use std::collections::HashMap;
5+
use std::sync::atomic::{AtomicI32, Ordering};
56

67
#[cfg(not(Py_LIMITED_API))]
78
use pyo3::buffer::PyBuffer;
@@ -565,13 +566,12 @@ fn test_closure() {
565566
#[test]
566567
fn test_closure_counter() {
567568
Python::attach(|py| {
568-
let counter = std::cell::RefCell::new(0);
569+
let counter = AtomicI32::new(0);
569570
let counter_fn = move |_args: &Bound<'_, types::PyTuple>,
570571
_kwargs: Option<&Bound<'_, types::PyDict>>|
571572
-> PyResult<i32> {
572-
let mut counter = counter.borrow_mut();
573-
*counter += 1;
574-
Ok(*counter)
573+
let prev_count = counter.fetch_add(1, Ordering::SeqCst);
574+
Ok(prev_count + 1)
575575
};
576576
let counter_py = PyCFunction::new_closure(py, None, None, counter_fn).unwrap();
577577

‎tests/ui/invalid_closure.rs‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,22 +2,17 @@ use pyo3::prelude::*;
22
use pyo3::types::{PyCFunction, PyDict, PyTuple};
33

44
fn main() {
5-
let fun: Py<PyCFunction> = Python::attach(|py| {
5+
// Closure must be `'static`
6+
Python::attach(|py| {
67
let local_data = vec![0, 1, 2, 3, 4];
78
let ref_: &[u8] = &local_data;
8-
//~^ ERROR: `local_data` does not live long enough
9+
//~^ ERROR: `local_data` does not live long enough
910

1011
let closure_fn =
1112
|_args: &Bound<'_, PyTuple>, _kwargs: Option<&Bound<'_, PyDict>>| -> PyResult<()> {
1213
println!("This is five: {:?}", ref_.len());
1314
Ok(())
1415
};
15-
PyCFunction::new_closure(py, None, None, closure_fn)
16-
.unwrap()
17-
.into()
18-
});
19-
20-
Python::attach(|py| {
21-
fun.call0(py).unwrap();
16+
PyCFunction::new_closure(py, None, None, closure_fn).unwrap();
2217
});
2318
}

‎tests/ui/invalid_closure.stderr‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,21 @@
11
error[E0597]: `local_data` does not live long enough
2-
--> tests/ui/invalid_closure.rs:7:27
2+
--> tests/ui/invalid_closure.rs:8:27
33
|
4-
6 | let local_data = vec![0, 1, 2, 3, 4];
4+
7 | let local_data = vec![0, 1, 2, 3, 4];
55
| ---------- binding `local_data` declared here
6-
7 | let ref_: &[u8] = &local_data;
6+
8 | let ref_: &[u8] = &local_data;
77
| ^^^^^^^^^^^ borrowed value does not live long enough
88
...
9-
15 | PyCFunction::new_closure(py, None, None, closure_fn)
9+
16 | PyCFunction::new_closure(py, None, None, closure_fn).unwrap();
1010
| ---------------------------------------------------- argument requires that `local_data` is borrowed for `'static`
11-
...
12-
18 | });
11+
17 | });
1312
| - `local_data` dropped here while still borrowed
1413
|
1514
note: requirement that the value outlives `'static` introduced here
1615
--> src/types/function.rs
1716
|
18-
| F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + 'static,
19-
| ^^^^^^^
17+
| F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static,
18+
| ^^^^^^^
2019

2120
error: aborting due to 1 previous error
2221

‎tests/ui/invalid_closure_send.rs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
use pyo3::prelude::*;
2+
use pyo3::types::{PyCFunction, PyDict, PyTuple};
3+
4+
#[derive(Clone)]
5+
struct NotSend(*mut std::ffi::c_void);
6+
unsafe impl Sync for NotSend {}
7+
8+
fn main() {
9+
// Closure must be `Send`
10+
Python::attach(|py| {
11+
let value = NotSend(std::ptr::null_mut());
12+
let closure_fn = move |_args: &Bound<'_, PyTuple>,
13+
_kwargs: Option<&Bound<'_, PyDict>>|
14+
-> PyResult<()> {
15+
let _ = value.clone();
16+
Ok(())
17+
};
18+
19+
PyCFunction::new_closure(py, None, None, closure_fn).unwrap();
20+
//~^ ERROR: `*mut c_void` cannot be sent between threads safely
21+
});
22+
}

‎tests/ui/invalid_closure_send.stderr‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
error[E0277]: `*mut c_void` cannot be sent between threads safely
2+
--> tests/ui/invalid_closure_send.rs:19:50
3+
|
4+
12 | let closure_fn = move |_args: &Bound<'_, PyTuple>,
5+
| __________________________-
6+
13 | | _kwargs: Option<&Bound<'_, PyDict>>|
7+
14 | | -> PyResult<()> {
8+
| |_____________________________- within this `{closure@tests/ui/invalid_closure_send.rs:12:26: 14:30}`
9+
...
10+
19 | PyCFunction::new_closure(py, None, None, closure_fn).unwrap();
11+
| ------------------------ ^^^^^^^^^^ `*mut c_void` cannot be sent between threads safely
12+
| |
13+
| required by a bound introduced by this call
14+
|
15+
= help: within `{closure@tests/ui/invalid_closure_send.rs:12:26: 14:30}`, the trait `Send` is not implemented for `*mut c_void`
16+
note: required because it appears within the type `NotSend`
17+
--> tests/ui/invalid_closure_send.rs:5:8
18+
|
19+
5 | struct NotSend(*mut std::ffi::c_void);
20+
| ^^^^^^^
21+
note: required because it's used within this closure
22+
--> tests/ui/invalid_closure_send.rs:12:26
23+
|
24+
12 | let closure_fn = move |_args: &Bound<'_, PyTuple>,
25+
| __________________________^
26+
13 | | _kwargs: Option<&Bound<'_, PyDict>>|
27+
14 | | -> PyResult<()> {
28+
| |_____________________________^
29+
note: required by a bound in `PyCFunction::new_closure`
30+
--> src/types/function.rs
31+
|
32+
| pub fn new_closure<'py, F, R>(
33+
| ----------- required by a bound in this associated function
34+
...
35+
| F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static,
36+
| ^^^^ required by this bound in `PyCFunction::new_closure`
37+
38+
error: aborting due to 1 previous error
39+
40+
For more information about this error, try `rustc --explain E0277`.

‎tests/ui/invalid_closure_sync.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
use pyo3::prelude::*;
2+
use pyo3::types::{PyCFunction, PyDict, PyTuple};
3+
4+
fn main() {
5+
// Closure must be `Sync`
6+
Python::attach(|py| {
7+
let data = std::cell::Cell::new(0);
8+
let closure_fn = move |_args: &Bound<'_, PyTuple>,
9+
_kwargs: Option<&Bound<'_, PyDict>>|
10+
-> PyResult<()> {
11+
let _ = data.clone();
12+
Ok(())
13+
};
14+
15+
PyCFunction::new_closure(py, None, None, closure_fn).unwrap();
16+
//~^ ERROR: `Cell<i32>` cannot be shared between threads safely
17+
});
18+
}

‎tests/ui/invalid_closure_sync.stderr‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
error[E0277]: `Cell<i32>` cannot be shared between threads safely
2+
--> tests/ui/invalid_closure_sync.rs:15:50
3+
|
4+
8 | let closure_fn = move |_args: &Bound<'_, PyTuple>,
5+
| __________________________-
6+
9 | | _kwargs: Option<&Bound<'_, PyDict>>|
7+
10 | | -> PyResult<()> {
8+
| |_____________________________- within this `{closure@tests/ui/invalid_closure_sync.rs:8:26: 10:30}`
9+
...
10+
15 | PyCFunction::new_closure(py, None, None, closure_fn).unwrap();
11+
| ------------------------ ^^^^^^^^^^ `Cell<i32>` cannot be shared between threads safely
12+
| |
13+
| required by a bound introduced by this call
14+
|
15+
= help: within `{closure@tests/ui/invalid_closure_sync.rs:8:26: 10:30}`, the trait `Sync` is not implemented for `Cell<i32>`
16+
= note: if you want to do aliasing and mutation between multiple threads, use `std::sync::RwLock` or `std::sync::atomic::AtomicI32` instead
17+
note: required because it's used within this closure
18+
--> tests/ui/invalid_closure_sync.rs:8:26
19+
|
20+
8 | let closure_fn = move |_args: &Bound<'_, PyTuple>,
21+
| __________________________^
22+
9 | | _kwargs: Option<&Bound<'_, PyDict>>|
23+
10 | | -> PyResult<()> {
24+
| |_____________________________^
25+
note: required by a bound in `PyCFunction::new_closure`
26+
--> src/types/function.rs
27+
|
28+
| pub fn new_closure<'py, F, R>(
29+
| ----------- required by a bound in this associated function
30+
...
31+
| F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static,
32+
| ^^^^ required by this bound in `PyCFunction::new_closure`
33+
34+
error: aborting due to 1 previous error
35+
36+
For more information about this error, try `rustc --explain E0277`.

0 commit comments

Comments
 (0)