From 0b6cd6574616794e5a2a2923dc782986c2711edd Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 12 Dec 2023 16:11:52 +0100 Subject: [PATCH 1/8] Close TLS-based soundness holes by running closure on a newly created thread. --- Cargo.toml | 3 - guide/src/features.md | 4 - src/err/mod.rs | 2 - src/instance.rs | 2 - src/lib.rs | 6 - src/marker.rs | 397 ++++++++------------------------------ tests/ui/not_send.stderr | 5 +- tests/ui/not_send2.stderr | 5 +- 8 files changed, 85 insertions(+), 339 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 69d2608536e..38f258ae2c1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,9 +96,6 @@ generate-import-lib = ["pyo3-ffi/generate-import-lib"] # Changes `Python::with_gil` to automatically initialize the Python interpreter if needed. auto-initialize = [] -# Optimizes PyObject to Vec conversion and so on. -nightly = [] - # Activates all additional features # This is mostly intended for testing purposes - activating *all* of these isn't particularly useful. full = [ diff --git a/guide/src/features.md b/guide/src/features.md index 8ed2a2ed0bc..3306a0b859d 100644 --- a/guide/src/features.md +++ b/guide/src/features.md @@ -81,10 +81,6 @@ Most users should only need a single `#[pymethods]` per `#[pyclass]`. In additio See [the `#[pyclass]` implementation details](class.md#implementation-details) for more information. -### `nightly` - -The `nightly` feature needs the nightly Rust compiler. This allows PyO3 to use the `auto_traits` and `negative_impls` features to fix the `Python::allow_threads` function. - ### `resolve-config` The `resolve-config` feature of the `pyo3-build-config` crate controls whether that crate's diff --git a/src/err/mod.rs b/src/err/mod.rs index 00ba111f8c5..6c439e6553d 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -37,8 +37,6 @@ pub struct PyErr { } // The inner value is only accessed through ways that require proving the gil is held -#[cfg(feature = "nightly")] -unsafe impl crate::marker::Ungil for PyErr {} unsafe impl Send for PyErr {} unsafe impl Sync for PyErr {} diff --git a/src/instance.rs b/src/instance.rs index 8406e86abbe..ee5a0f7ec10 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -518,8 +518,6 @@ impl Copy for Borrowed<'_, '_, T> {} pub struct Py(NonNull, PhantomData); // The inner value is only accessed through ways that require proving the gil is held -#[cfg(feature = "nightly")] -unsafe impl crate::marker::Ungil for Py {} unsafe impl Send for Py {} unsafe impl Sync for Py {} diff --git a/src/lib.rs b/src/lib.rs index f70ea01b91f..4089db45fbc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,4 @@ #![warn(missing_docs)] -#![cfg_attr(feature = "nightly", feature(auto_traits, negative_impls))] #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] // Deny some lints in doctests. // Use `#[allow(...)]` locally to override. @@ -97,10 +96,6 @@ //! [`Py`]`` for all `T` that implement [`Serialize`] and [`Deserialize`]. //! - [`smallvec`][smallvec]: Enables conversions between Python list and [smallvec]'s [`SmallVec`]. //! -//! ## Unstable features -//! -//! - `nightly`: Uses `#![feature(auto_traits, negative_impls)]` to define [`Ungil`] as an auto trait. -// //! ## `rustc` environment flags //! //! PyO3 uses `rustc`'s `--cfg` flags to enable or disable code used for different Python versions. @@ -292,7 +287,6 @@ //! [Python from Rust]: https://github.com/PyO3/pyo3#using-python-from-rust //! [Rust from Python]: https://github.com/PyO3/pyo3#using-rust-from-python //! [Features chapter of the guide]: https://pyo3.rs/latest/features.html#features-reference "Features Reference - PyO3 user guide" -//! [`Ungil`]: crate::marker::Ungil pub use crate::class::*; pub use crate::conversion::{AsPyPointer, FromPyObject, FromPyPointer, IntoPy, ToPyObject}; #[allow(deprecated)] diff --git a/src/marker.rs b/src/marker.rs index 97f826e571f..e01c5c6563b 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -14,19 +14,8 @@ //! awaiting a future //! - Once that is done, reacquire the GIL //! -//! That API is provided by [`Python::allow_threads`] and enforced via the [`Ungil`] bound on the -//! closure and the return type. This is done by relying on the [`Send`] auto trait. `Ungil` is -//! defined as the following: -//! -//! ```rust -//! pub unsafe trait Ungil {} -//! -//! unsafe impl Ungil for T {} -//! ``` -//! -//! We piggy-back off the `Send` auto trait because it is not possible to implement custom auto -//! traits on stable Rust. This is the solution which enables it for as many types as possible while -//! making the API usable. +//! That API is provided by [`Python::allow_threads`] and enforced via the [`Send`] bound on the +//! closure and the return type. //! //! In practice this API works quite well, but it comes with some drawbacks: //! @@ -37,82 +26,19 @@ //! thread. //! //! ```rust, compile_fail -//! # #[cfg(feature = "nightly")] -//! # compile_error!("this actually works on nightly") //! use pyo3::prelude::*; //! use std::rc::Rc; //! -//! fn main() { -//! Python::with_gil(|py| { -//! let rc = Rc::new(5); -//! -//! py.allow_threads(|| { -//! // This would actually be fine... -//! println!("{:?}", *rc); -//! }); -//! }); -//! } -//! ``` -//! -//! Because we are using `Send` for something it's not quite meant for, other code that -//! (correctly) upholds the invariants of [`Send`] can cause problems. -//! -//! [`SendWrapper`] is one of those. Per its documentation: -//! -//! > A wrapper which allows you to move around non-Send-types between threads, as long as you -//! > access the contained value only from within the original thread and make sure that it is -//! > dropped from within the original thread. -//! -//! This will "work" to smuggle Python references across the closure, because we're not actually -//! doing anything with threads: -//! -//! ```rust, no_run -//! use pyo3::prelude::*; -//! use pyo3::types::PyString; -//! use send_wrapper::SendWrapper; -//! //! Python::with_gil(|py| { -//! let string = PyString::new(py, "foo"); -//! -//! let wrapped = SendWrapper::new(string); +//! let rc = Rc::new(5); //! //! py.allow_threads(|| { -//! # #[cfg(not(feature = "nightly"))] -//! # { -//! // 💥 Unsound! 💥 -//! let smuggled: &PyString = *wrapped; -//! println!("{:?}", smuggled); -//! # } +//! // This would actually be fine... +//! println!("{:?}", *rc); //! }); //! }); //! ``` //! -//! For now the answer to that is "don't do that". -//! -//! # A proper implementation using an auto trait -//! -//! However on nightly Rust and when PyO3's `nightly` feature is -//! enabled, `Ungil` is defined as the following: -//! -//! ```rust -//! # #[cfg(FALSE)] -//! # { -//! #![feature(auto_traits, negative_impls)] -//! -//! pub unsafe auto trait Ungil {} -//! -//! // It is unimplemented for the `Python` struct and Python objects. -//! impl !Ungil for Python<'_> {} -//! impl !Ungil for ffi::PyObject {} -//! -//! // `Py` wraps it in a safe api, so this is OK -//! unsafe impl Ungil for Py {} -//! # } -//! ``` -//! -//! With this feature enabled, the above two examples will start working and not working, respectively. -//! -//! [`SendWrapper`]: https://docs.rs/send_wrapper/latest/send_wrapper/struct.SendWrapper.html //! [`Rc`]: std::rc::Rc //! [`Py`]: crate::Py use crate::err::{self, PyDowncastError, PyErr, PyResult}; @@ -127,174 +53,7 @@ use crate::{ffi, FromPyPointer, IntoPy, Py, PyObject, PyTypeCheck, PyTypeInfo}; use std::ffi::{CStr, CString}; use std::marker::PhantomData; use std::os::raw::c_int; - -/// Types that are safe to access while the GIL is not held. -/// -/// # Safety -/// -/// The type must not carry borrowed Python references or, if it does, not allow access to them if -/// the GIL is not held. -/// -/// See the [module-level documentation](self) for more information. -/// -/// # Examples -/// -/// This tracking is currently imprecise as it relies on the [`Send`] auto trait on stable Rust. -/// For example, an `Rc` smart pointer should be usable without the GIL, but we currently prevent that: -/// -/// ```compile_fail -/// # use pyo3::prelude::*; -/// use std::rc::Rc; -/// -/// Python::with_gil(|py| { -/// let rc = Rc::new(42); -/// -/// py.allow_threads(|| { -/// println!("{:?}", rc); -/// }); -/// }); -/// ``` -/// -/// This also implies that the interplay between `with_gil` and `allow_threads` is unsound, for example -/// one can circumvent this protection using the [`send_wrapper`](https://docs.rs/send_wrapper/) crate: -/// -/// ```no_run -/// # use pyo3::prelude::*; -/// # use pyo3::types::PyString; -/// use send_wrapper::SendWrapper; -/// -/// Python::with_gil(|py| { -/// let string = PyString::new(py, "foo"); -/// -/// let wrapped = SendWrapper::new(string); -/// -/// py.allow_threads(|| { -/// let sneaky: &PyString = *wrapped; -/// -/// println!("{:?}", sneaky); -/// }); -/// }); -/// ``` -/// -/// Fixing this loophole on stable Rust has significant ergonomic issues, but it is fixed when using -/// nightly Rust and the `nightly` feature, c.f. [#2141](https://github.com/PyO3/pyo3/issues/2141). -#[cfg_attr(docsrs, doc(cfg(all())))] // Hide the cfg flag -#[cfg(not(feature = "nightly"))] -pub unsafe trait Ungil {} - -#[cfg_attr(docsrs, doc(cfg(all())))] // Hide the cfg flag -#[cfg(not(feature = "nightly"))] -unsafe impl Ungil for T {} - -#[cfg(feature = "nightly")] -mod nightly { - macro_rules! define { - ($($tt:tt)*) => { $($tt)* } - } - - define! { - /// Types that are safe to access while the GIL is not held. - /// - /// # Safety - /// - /// The type must not carry borrowed Python references or, if it does, not allow access to them if - /// the GIL is not held. - /// - /// See the [module-level documentation](self) for more information. - /// - /// # Examples - /// - /// Types which are `Ungil` cannot be used in contexts where the GIL was released, e.g. - /// - /// ```compile_fail - /// # use pyo3::prelude::*; - /// # use pyo3::types::PyString; - /// Python::with_gil(|py| { - /// let string = PyString::new(py, "foo"); - /// - /// py.allow_threads(|| { - /// println!("{:?}", string); - /// }); - /// }); - /// ``` - /// - /// This applies to the GIL token `Python` itself as well, e.g. - /// - /// ```compile_fail - /// # use pyo3::prelude::*; - /// Python::with_gil(|py| { - /// py.allow_threads(|| { - /// drop(py); - /// }); - /// }); - /// ``` - /// - /// On nightly Rust, this is not based on the [`Send`] auto trait and hence we are able - /// to prevent incorrectly circumventing it using e.g. the [`send_wrapper`](https://docs.rs/send_wrapper/) crate: - /// - /// ```compile_fail - /// # use pyo3::prelude::*; - /// # use pyo3::types::PyString; - /// use send_wrapper::SendWrapper; - /// - /// Python::with_gil(|py| { - /// let string = PyString::new(py, "foo"); - /// - /// let wrapped = SendWrapper::new(string); - /// - /// py.allow_threads(|| { - /// let sneaky: &PyString = *wrapped; - /// - /// println!("{:?}", sneaky); - /// }); - /// }); - /// ``` - /// - /// This also enables using non-[`Send`] types in `allow_threads`, - /// at least if they are not also bound to the GIL: - /// - /// ```rust - /// # use pyo3::prelude::*; - /// use std::rc::Rc; - /// - /// Python::with_gil(|py| { - /// let rc = Rc::new(42); - /// - /// py.allow_threads(|| { - /// println!("{:?}", rc); - /// }); - /// }); - /// ``` - pub unsafe auto trait Ungil {} - } - - impl !Ungil for crate::Python<'_> {} - - // This means that PyString, PyList, etc all inherit !Ungil from this. - impl !Ungil for crate::PyAny {} - - // All the borrowing wrappers - impl !Ungil for crate::PyCell {} - impl !Ungil for crate::PyRef<'_, T> {} - impl !Ungil for crate::PyRefMut<'_, T> {} - - // FFI pointees - impl !Ungil for crate::ffi::PyObject {} - impl !Ungil for crate::ffi::PyLongObject {} - - impl !Ungil for crate::ffi::PyThreadState {} - impl !Ungil for crate::ffi::PyInterpreterState {} - impl !Ungil for crate::ffi::PyWeakReference {} - impl !Ungil for crate::ffi::PyFrameObject {} - impl !Ungil for crate::ffi::PyCodeObject {} - #[cfg(not(Py_LIMITED_API))] - impl !Ungil for crate::ffi::PyDictKeysObject {} - #[cfg(not(any(Py_LIMITED_API, Py_3_10)))] - impl !Ungil for crate::ffi::PyArena {} -} - -#[cfg(feature = "nightly")] -pub use nightly::Ungil; +use std::thread; /// A marker token that represents holding the GIL. /// @@ -478,7 +237,7 @@ impl<'py> Python<'py> { /// interpreter for some time and have other Python threads around, this will let you run /// Rust-only code while letting those other Python threads make progress. /// - /// Only types that implement [`Ungil`] can cross the closure. See the + /// Only types that implement [`Send`] can cross the closure. See the /// [module level documentation](self) for more information. /// /// If you need to pass Python objects into the closure you can use [`Py`]``to create a @@ -528,20 +287,92 @@ impl<'py> Python<'py> { /// } /// ``` /// + /// # Example: The `send_wrapper` loophole is closed by running the closure on dedicated thread + /// + /// ```should_panic + /// # use pyo3::prelude::*; + /// # use pyo3::types::PyString; + /// use send_wrapper::SendWrapper; + /// + /// Python::with_gil(|py| { + /// let string = PyString::new(py, "foo"); + /// + /// let wrapped = SendWrapper::new(string); + /// + /// py.allow_threads(|| { + /// // panicks because this is not the thread which created `wrapped` + /// let sneaky: &PyString = *wrapped; + /// println!("{:?}", sneaky); + /// }); + /// }); + /// ``` + /// /// [`Py`]: crate::Py /// [`PyString`]: crate::types::PyString /// [auto-traits]: https://doc.rust-lang.org/nightly/unstable-book/language-features/auto-traits.html /// [Parallelism]: https://pyo3.rs/main/parallelism.html pub fn allow_threads(self, f: F) -> T where - F: Ungil + FnOnce() -> T, - T: Ungil, + F: Send + FnOnce() -> T, + T: Send, { // Use a guard pattern to handle reacquiring the GIL, // so that the GIL will be reacquired even if `f` panics. // The `Send` bound on the closure prevents the user from // transferring the `Python` token into the closure. let _guard = unsafe { SuspendGIL::new() }; + + // To close soundness loopholes w.r.t. `send_wrapper` or `scoped-tls`, + // we run the closure on a newly created thread so that it cannot + // access thread-local storage from the current thread. + thread::scope(|s| s.spawn(f).join().unwrap()) + } + + /// An unsafe version of [`allow_threads`][Self::allow_threads] + /// + /// This version does not run the given closure on a dedicated runtime thread, + /// therefore it is more efficient and has access to thread-local storage + /// established at the call site. + /// + /// However, it is also subject to soundness loopholes based on thread identity + /// for example when `send_wrapper` is used: + /// + /// ```no_run + /// # use pyo3::prelude::*; + /// # use pyo3::types::PyString; + /// use send_wrapper::SendWrapper; + /// + /// Python::with_gil(|py| { + /// let string = PyString::new(py, "foo"); + /// + /// let wrapped = SendWrapper::new(string); + /// + /// unsafe { + /// py.unsafe_allow_threads(|| { + /// // 💥 Unsound! 💥 + /// let sneaky: &PyString = *wrapped; + /// println!("{:?}", sneaky); + /// }); + /// } + /// }); + /// ``` + /// + /// # Safety + /// + /// The caller must ensure that no code within the closure accesses GIL-protected data + /// bound to the current thread. Note that this property is highly non-local as for example + /// `scoped-tls` allows "smuggling" GIL-bound references using what is essentially global state. + pub unsafe fn unsafe_allow_threads(self, f: F) -> T + where + F: Send + FnOnce() -> T, + T: Send, + { + // Use a guard pattern to handle reacquiring the GIL, + // so that the GIL will be reacquired even if `f` panics. + // The `Send` bound on the closure prevents the user from + // transferring the `Python` token into the closure. + let _guard = SuspendGIL::new(); + f() } @@ -975,72 +806,6 @@ impl<'py> Python<'py> { } } -impl Python<'_> { - /// Creates a scope using a new pool for managing PyO3's owned references. - /// - /// 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 - /// being unable to capture existing GIL-bound references. - /// - /// Note that on stable Rust, this API suffers from the same the `SendWrapper` loophole - /// as [`allow_threads`][Self::allow_threads], c.f. the documentation of the [`Ungil`] trait, - /// - /// # 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 scope, 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! - /// } - /// }); - /// ``` - /// - /// The `Ungil` bound on the closure does prevent hanging on to existing GIL-bound references - /// - /// ```compile_fail - /// # use pyo3::prelude::*; - /// # use pyo3::types::PyString; - /// - /// Python::with_gil(|py| { - /// let old_str = PyString::new(py, "a message from the past"); - /// - /// py.with_pool(|_py| { - /// print!("{:?}", old_str); - /// }); - /// }); - /// ``` - /// - /// or continuing to use the old GIL token - /// - /// ```compile_fail - /// # use pyo3::prelude::*; - /// - /// Python::with_gil(|old_py| { - /// old_py.with_pool(|_new_py| { - /// let _none = old_py.None(); - /// }); - /// }); - /// ``` - #[inline] - pub fn with_pool(&self, f: F) -> R - where - F: for<'py> FnOnce(Python<'py>) -> R + Ungil, - { - // SAFETY: The closure is `Ungil`, - // i.e. it does not capture any GIL-bound references - // and accesses only the newly created GIL token. - let pool = unsafe { GILPool::new() }; - - f(pool.python()) - } -} - impl<'unbound> Python<'unbound> { /// Unsafely creates a Python token with an unbounded lifetime. /// diff --git a/tests/ui/not_send.stderr b/tests/ui/not_send.stderr index 9ac51f36ec0..da4f776b7f6 100644 --- a/tests/ui/not_send.stderr +++ b/tests/ui/not_send.stderr @@ -34,12 +34,11 @@ note: required because it's used within this closure | 4 | py.allow_threads(|| { drop(py); }); | ^^ - = note: required for `{closure@$DIR/tests/ui/not_send.rs:4:22: 4:24}` to implement `Ungil` note: required by a bound in `pyo3::Python::<'py>::allow_threads` --> src/marker.rs | | pub fn allow_threads(self, f: F) -> T | ------------- required by a bound in this associated function | where - | F: Ungil + FnOnce() -> T, - | ^^^^^ required by this bound in `Python::<'py>::allow_threads` + | F: Send + FnOnce() -> T, + | ^^^^ required by this bound in `Python::<'py>::allow_threads` diff --git a/tests/ui/not_send2.stderr b/tests/ui/not_send2.stderr index d3a60a1f708..075b28d8cc5 100644 --- a/tests/ui/not_send2.stderr +++ b/tests/ui/not_send2.stderr @@ -27,12 +27,11 @@ note: required because it's used within this closure | 8 | py.allow_threads(|| { | ^^ - = note: required for `{closure@$DIR/tests/ui/not_send2.rs:8:26: 8:28}` to implement `Ungil` note: required by a bound in `pyo3::Python::<'py>::allow_threads` --> src/marker.rs | | pub fn allow_threads(self, f: F) -> T | ------------- required by a bound in this associated function | where - | F: Ungil + FnOnce() -> T, - | ^^^^^ required by this bound in `Python::<'py>::allow_threads` + | F: Send + FnOnce() -> T, + | ^^^^ required by this bound in `Python::<'py>::allow_threads` From dfd83f71cf609daae24b6dd883e3da9a55a6bd57 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 12 Dec 2023 20:33:08 +0100 Subject: [PATCH 2/8] Re-use threads when calling closures after releasing GIL. --- src/gil.rs | 24 +++++++------- src/marker.rs | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 97 insertions(+), 14 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index d346ad95ea9..5725ae2df03 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -696,23 +696,25 @@ mod tests { #[test] fn test_allow_threads() { - assert!(!gil_is_acquired()); + for _ in 0..10 { + assert!(!gil_is_acquired()); - Python::with_gil(|py| { - assert!(gil_is_acquired()); + Python::with_gil(|py| { + assert!(gil_is_acquired()); - py.allow_threads(move || { - assert!(!gil_is_acquired()); + py.allow_threads(move || { + assert!(!gil_is_acquired()); - Python::with_gil(|_| assert!(gil_is_acquired())); + Python::with_gil(|_| assert!(gil_is_acquired())); - assert!(!gil_is_acquired()); - }); + assert!(!gil_is_acquired()); + }); - assert!(gil_is_acquired()); - }); + assert!(gil_is_acquired()); + }); - assert!(!gil_is_acquired()); + assert!(!gil_is_acquired()); + } } #[test] diff --git a/src/marker.rs b/src/marker.rs index e01c5c6563b..06930bd9608 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -53,7 +53,6 @@ use crate::{ffi, FromPyPointer, IntoPy, Py, PyObject, PyTypeCheck, PyTypeInfo}; use std::ffi::{CStr, CString}; use std::marker::PhantomData; use std::os::raw::c_int; -use std::thread; /// A marker token that represents holding the GIL. /// @@ -316,6 +315,16 @@ impl<'py> Python<'py> { F: Send + FnOnce() -> T, T: Send, { + use std::mem::transmute; + use std::panic::{catch_unwind, resume_unwind, AssertUnwindSafe}; + use std::sync::mpsc::{sync_channel, SendError, SyncSender}; + use std::thread::{Builder, Result}; + use std::time::Duration; + + use parking_lot::{const_mutex, Mutex}; + + use crate::impl_::panic::PanicTrap; + // Use a guard pattern to handle reacquiring the GIL, // so that the GIL will be reacquired even if `f` panics. // The `Send` bound on the closure prevents the user from @@ -323,9 +332,81 @@ impl<'py> Python<'py> { let _guard = unsafe { SuspendGIL::new() }; // To close soundness loopholes w.r.t. `send_wrapper` or `scoped-tls`, - // we run the closure on a newly created thread so that it cannot + // we run the closure on a separate thread so that it cannot // access thread-local storage from the current thread. - thread::scope(|s| s.spawn(f).join().unwrap()) + + // 1. Construct a task + struct Task(*mut (dyn FnMut() + 'static)); + unsafe impl Send for Task {} + + let (result_sender, result_receiver) = sync_channel::>(0); + + let mut f = Some(f); + + let mut task = || { + let f = f + .take() + .expect("allow_threads closure called more than once"); + + let result = catch_unwind(AssertUnwindSafe(f)); + + result_sender + .send(result) + .expect("allow_threads runtime task was abandoned"); + }; + + // SAFETY: the current thread will block until the closure has returned + let mut task = Task(unsafe { transmute(&mut task as &mut (dyn FnMut() + '_)) }); + + // 2. Dispatch task to waiting thread, spawn new thread if necessary + let trap = PanicTrap::new( + "allow_threads panicked while stack data was accessed by another thread, please report this as a bug at https://github.com/PyO3/pyo3/issues", + ); + + static THREADS: Mutex>> = const_mutex(Vec::new()); + + let dispatch = move || { + while let Some(task_sender) = THREADS.lock().pop() { + match task_sender.send(task) { + Ok(()) => return task_sender, + Err(SendError(same_task)) => task = same_task, + } + } + + let (task_sender, task_receiver) = sync_channel::(0); + + Builder::new() + .name("pyo3 allow_threads runtime".to_owned()) + .spawn(move || { + let mut next_task = Ok(task); + + while let Ok(task) = next_task { + // SAFETY: all data accessed by `task` will stay alive until it completes + unsafe { (*task.0)() }; + + next_task = task_receiver.recv_timeout(Duration::from_secs(60)); + } + }) + .expect("failed to create allow_threads runtime thread"); + + task_sender + }; + + let task_sender = dispatch(); + + // 3. Wait for completion and check result + let result = result_receiver + .recv() + .expect("allow_threads runtime thread died unexpectedly"); + + trap.disarm(); + + THREADS.lock().push(task_sender); + + match result { + Ok(result) => result, + Err(payload) => resume_unwind(payload), + } } /// An unsafe version of [`allow_threads`][Self::allow_threads] From 513f5b2a0582fd97e63a571340037bfe099a2361 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Thu, 14 Dec 2023 13:26:35 +0100 Subject: [PATCH 3/8] Extend the documentation on the thread-level shenanigans now employed by allow_threads. --- Cargo.toml | 1 + noxfile.py | 1 + src/marker.rs | 80 ++++++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 38f258ae2c1..b8baf41a92d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,6 +52,7 @@ chrono = { version = "0.4.25" } trybuild = ">=1.0.70" proptest = { version = "1.0", default-features = false, features = ["std"] } send_wrapper = "0.6" +scoped-tls = "1.0" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.61" rayon = "1.6.1" diff --git a/noxfile.py b/noxfile.py index 8288cbb7b1c..a4e3297857b 100644 --- a/noxfile.py +++ b/noxfile.py @@ -506,6 +506,7 @@ def set_minimal_package_versions(session: nox.Session): "crossbeam-deque": "0.8.3", "crossbeam-epoch": "0.9.15", "crossbeam-utils": "0.8.16", + "scoped-tls": "1.0.0", } # run cargo update first to ensure that everything is at highest diff --git a/src/marker.rs b/src/marker.rs index 06930bd9608..f9eee7ae766 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -17,13 +17,13 @@ //! That API is provided by [`Python::allow_threads`] and enforced via the [`Send`] bound on the //! closure and the return type. //! -//! In practice this API works quite well, but it comes with some drawbacks: +//! In practice this API works quite well, but it comes with a big drawback: +//! There is no instrinsic reason to prevent `!Send` types like [`Rc`] from crossing the closure. +//! After all, we release the GIL to let other Python threads run, not necessarily to launch new threads. //! -//! ## Drawbacks -//! -//! There is no reason to prevent `!Send` types like [`Rc`] from crossing the closure. After all, -//! [`Python::allow_threads`] just lets other Python threads run - it does not itself launch a new -//! thread. +//! But to isolate the closure from references bound to the current thread holding the GIL +//! and to close soundness holes implied by thread-local storage hiding such references, +//! we do need to run the closure on a dedicated runtime thread. //! //! ```rust, compile_fail //! use pyo3::prelude::*; @@ -33,12 +33,62 @@ //! let rc = Rc::new(5); //! //! py.allow_threads(|| { -//! // This would actually be fine... +//! // This could be fine... //! println!("{:?}", *rc); //! }); //! }); //! ``` //! +//! However, running the closure on a distinct thread is required as otherwise +//! thread-local storage could be used to "smuggle" GIL-bound data into it +//! independently of any trait bounds (whether using `Send` or an auto trait +//! dedicated to handling GIL-bound data): +//! +//! ```rust, no_run +//! use pyo3::prelude::*; +//! use pyo3::types::PyString; +//! use scoped_tls::scoped_thread_local; +//! +//! scoped_thread_local!(static WRAPPED: PyString); +//! +//! fn callback() { +//! WRAPPED.with(|smuggled: &PyString| { +//! println!("{:?}", smuggled); +//! }); +//! } +//! +//! Python::with_gil(|py| { +//! let string = PyString::new(py, "foo"); +//! +//! WRAPPED.set(string, || { +//! py.allow_threads(callback); +//! }); +//! }); +//! ``` +//! +//! PyO3 tries to minimize the overhead of using dedicated threads by re-using them, +//! i.e. after a thread is spawned to execute a closure with the GIL temporarily released, +//! it is kept around for up to one minute to potentially service subsequent invocations of `allow_threads`. +//! +//! Note that PyO3 will however not wait to re-use an existing that is currently blocked by other work, +//! i.e. to keep latency to a minimum a new thread will be started to immediately run the given closure. +//! +//! These long-lived background threads are named `pyo3 allow_threads runtime` +//! to facilitate diagnosing any performance issues they might cause on the process level. +//! +//! One important consequence of this approach is that the state of thread-local storage (TLS) +//! is essentially undefined: The thread might be newly spawn so that TLS needs to be newly initialized, +//! but it might also be re-used so that TLS contains values created by previous calls to `allow_threads`. +//! +//! If the performance overhead of shunting the closure to another is too high +//! or code requires access to thread-local storage established by the calling thread, +//! there is the unsafe escape hatch [`Python::unsafe_allow_threads`] +//! which executes the closure directly after suspending the GIL. +//! +//! However, note establishing the required invariants to soundly call this function +//! requires highly non-local reasoning as thread-local storage allows "smuggling" GIL-bound references +//! using what is essentially global state. +//! //! [`Rc`]: std::rc::Rc //! [`Py`]: crate::Py use crate::err::{self, PyDowncastError, PyErr, PyResult}; @@ -232,17 +282,19 @@ impl<'py> Python<'py> { /// Temporarily releases the GIL, thus allowing other Python threads to run. The GIL will be /// reacquired when `F`'s scope ends. /// - /// If you don't need to touch the Python - /// interpreter for some time and have other Python threads around, this will let you run - /// Rust-only code while letting those other Python threads make progress. + /// If you don't need to touch the Python interpreter for some time and have other Python threads around, + /// this will let you run Rust-only code while letting those other Python threads make progress. /// - /// Only types that implement [`Send`] can cross the closure. See the - /// [module level documentation](self) for more information. + /// Only types that implement [`Send`] can cross the closure + /// because *it is executed on a dedicated runtime thread* + /// to prevent access to GIL-bound references based on thread identity. /// /// If you need to pass Python objects into the closure you can use [`Py`]``to create a /// reference independent of the GIL lifetime. However, you cannot do much with those without a /// [`Python`] token, for which you'd need to reacquire the GIL. /// + /// See the [module level documentation](self) for more information. + /// /// # Example: Releasing the GIL while running a computation in Rust-only code /// /// ``` @@ -411,7 +463,7 @@ impl<'py> Python<'py> { /// An unsafe version of [`allow_threads`][Self::allow_threads] /// - /// This version does not run the given closure on a dedicated runtime thread, + /// This version does _not_ run the given closure on a dedicated runtime thread, /// therefore it is more efficient and has access to thread-local storage /// established at the call site. /// @@ -438,6 +490,8 @@ impl<'py> Python<'py> { /// }); /// ``` /// + /// See the [module level documentation](self) for more information. + /// /// # Safety /// /// The caller must ensure that no code within the closure accesses GIL-protected data From 1f446a5946407f7115ad46da7500895ab8ee0b3e Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sat, 16 Dec 2023 15:26:42 +0100 Subject: [PATCH 4/8] Add micro benchmark comparing the two variants of allow_threads. --- pyo3-benches/benches/bench_gil.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/pyo3-benches/benches/bench_gil.rs b/pyo3-benches/benches/bench_gil.rs index e25345e1fe9..a3dd739fd2e 100644 --- a/pyo3-benches/benches/bench_gil.rs +++ b/pyo3-benches/benches/bench_gil.rs @@ -1,4 +1,6 @@ -use codspeed_criterion_compat::{criterion_group, criterion_main, BatchSize, Bencher, Criterion}; +use codspeed_criterion_compat::{ + black_box, criterion_group, criterion_main, BatchSize, Bencher, Criterion, +}; use pyo3::{prelude::*, GILPool}; @@ -27,10 +29,25 @@ fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { ); } +fn bench_allow_threads(b: &mut Bencher<'_>) { + Python::with_gil(|py| { + py.allow_threads(|| ()); + b.iter(|| py.allow_threads(|| black_box(42))); + }); +} + +fn bench_unsafe_allow_threads(b: &mut Bencher<'_>) { + Python::with_gil(|py| { + b.iter(|| unsafe { py.unsafe_allow_threads(|| black_box(42)) }); + }); +} + fn criterion_benchmark(c: &mut Criterion) { c.bench_function("clean_gilpool_new", bench_clean_gilpool_new); c.bench_function("clean_acquire_gil", bench_clean_acquire_gil); c.bench_function("dirty_acquire_gil", bench_dirty_acquire_gil); + c.bench_function("allow_threads", bench_allow_threads); + c.bench_function("unsafe_allow_threads", bench_unsafe_allow_threads); } criterion_group!(benches, criterion_benchmark); From 8141a057a7bcba3c7c902847516fbac276987f27 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 17 Dec 2023 12:51:30 +0100 Subject: [PATCH 5/8] Avoid serializing effect of rendezvous channels This also synchronizes using a single mutex and condition variable and uses a single dynamic allocation per worker thread which leads to measurable if small improvements in the benchmark. Furthermore, this makes the state machine shared between main and worker thread more explicit in the code. --- src/marker.rs | 160 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 125 insertions(+), 35 deletions(-) diff --git a/src/marker.rs b/src/marker.rs index f9eee7ae766..a82ec85225a 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -367,13 +367,13 @@ impl<'py> Python<'py> { F: Send + FnOnce() -> T, T: Send, { - use std::mem::transmute; + use std::mem::{replace, transmute}; use std::panic::{catch_unwind, resume_unwind, AssertUnwindSafe}; - use std::sync::mpsc::{sync_channel, SendError, SyncSender}; - use std::thread::{Builder, Result}; + use std::sync::Arc; + use std::thread::Builder; use std::time::Duration; - use parking_lot::{const_mutex, Mutex}; + use parking_lot::{const_mutex, Condvar, Mutex}; use crate::impl_::panic::PanicTrap; @@ -391,20 +391,15 @@ impl<'py> Python<'py> { struct Task(*mut (dyn FnMut() + 'static)); unsafe impl Send for Task {} - let (result_sender, result_receiver) = sync_channel::>(0); - let mut f = Some(f); + let mut result = None; let mut task = || { let f = f .take() .expect("allow_threads closure called more than once"); - let result = catch_unwind(AssertUnwindSafe(f)); - - result_sender - .send(result) - .expect("allow_threads runtime task was abandoned"); + result = Some(catch_unwind(AssertUnwindSafe(f))); }; // SAFETY: the current thread will block until the closure has returned @@ -415,47 +410,142 @@ impl<'py> Python<'py> { "allow_threads panicked while stack data was accessed by another thread, please report this as a bug at https://github.com/PyO3/pyo3/issues", ); - static THREADS: Mutex>> = const_mutex(Vec::new()); + enum MailboxInner { + Empty, + Task(Task), + Working, + Done, + Abandoned, + } - let dispatch = move || { - while let Some(task_sender) = THREADS.lock().pop() { - match task_sender.send(task) { - Ok(()) => return task_sender, - Err(SendError(same_task)) => task = same_task, + struct Mailbox { + inner: Mutex, + flag: Condvar, + } + + impl Mailbox { + fn new(task: Task) -> Self { + Self { + inner: Mutex::new(MailboxInner::Task(task)), + flag: Condvar::new(), } } - let (task_sender, task_receiver) = sync_channel::(0); + fn send_task(&self, task: Task) -> Option { + use MailboxInner::*; + let mut inner = self.inner.lock(); + match &*inner { + Empty => { + *inner = Task(task); + drop(inner); + self.flag.notify_one(); + None + } + Abandoned => Some(task), + Task(_) | Working | Done => unreachable!("sent task to active worker"), + } + } - Builder::new() - .name("pyo3 allow_threads runtime".to_owned()) - .spawn(move || { - let mut next_task = Ok(task); + fn recv_task(&self) -> Option { + use MailboxInner::*; + let mut inner = self.inner.lock(); + loop { + match &*inner { + Empty | Done => { + if self + .flag + .wait_for(&mut inner, Duration::from_secs(60)) + .timed_out() + { + *inner = Abandoned; + return None; + } + } + Task(_) => match replace(&mut *inner, Working) { + Task(task) => return Some(task), + _ => unreachable!(), + }, + Working | Abandoned => { + unreachable!("received task on active or exited worker") + } + } + } + } - while let Ok(task) = next_task { - // SAFETY: all data accessed by `task` will stay alive until it completes - unsafe { (*task.0)() }; + fn signal_done(&self) { + use MailboxInner::*; + let mut inner = self.inner.lock(); + match &*inner { + Working => { + *inner = Done; + drop(inner); + self.flag.notify_one(); + } + Empty | Task(_) | Done | Abandoned => { + unreachable!("signalled completion on inactive worker") + } + } + } - next_task = task_receiver.recv_timeout(Duration::from_secs(60)); + fn await_done(&self) { + use MailboxInner::*; + let mut inner = self.inner.lock(); + loop { + match &*inner { + Done => { + *inner = Empty; + return; + } + Task(_) | Working => self.flag.wait(&mut inner), + Empty | Abandoned => { + unreachable!("awaited completion from inactive worker") + } } - }) - .expect("failed to create allow_threads runtime thread"); + } + } + } + + static THREADS: Mutex>> = const_mutex(Vec::new()); + + let dispatch = move || { + while let Some(mailbox) = THREADS.lock().pop() { + match mailbox.send_task(task) { + None => return mailbox, + Some(same_task) => task = same_task, + } + } + + let mailbox = Arc::new(Mailbox::new(task)); + + { + let mailbox = Arc::clone(&mailbox); + + Builder::new() + .name("pyo3 allow_threads runtime".to_owned()) + .spawn(move || { + while let Some(task) = mailbox.recv_task() { + // SAFETY: all data accessed by `task` will stay alive until it completes + unsafe { (*task.0)() }; + + mailbox.signal_done(); + } + }) + .expect("failed to create allow_threads runtime thread"); + } - task_sender + mailbox }; - let task_sender = dispatch(); + let mailbox = dispatch(); // 3. Wait for completion and check result - let result = result_receiver - .recv() - .expect("allow_threads runtime thread died unexpectedly"); + mailbox.await_done(); trap.disarm(); - THREADS.lock().push(task_sender); + THREADS.lock().push(mailbox); - match result { + match result.expect("allow_threads runtime thread did not set result") { Ok(result) => result, Err(payload) => resume_unwind(payload), } From 3d132f62185d17525caaa4dcb0be6ed20bbdd5d5 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 24 Dec 2023 11:16:22 +0100 Subject: [PATCH 6/8] Use one allow_threads runtime thread per user thread to improve locality and simplify dispatch. --- src/marker.rs | 50 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/marker.rs b/src/marker.rs index a82ec85225a..8ca7e665fcf 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -373,7 +373,7 @@ impl<'py> Python<'py> { use std::thread::Builder; use std::time::Duration; - use parking_lot::{const_mutex, Condvar, Mutex}; + use parking_lot::{Condvar, Mutex}; use crate::impl_::panic::PanicTrap; @@ -403,7 +403,7 @@ impl<'py> Python<'py> { }; // SAFETY: the current thread will block until the closure has returned - let mut task = Task(unsafe { transmute(&mut task as &mut (dyn FnMut() + '_)) }); + let task = Task(unsafe { transmute(&mut task as &mut (dyn FnMut() + '_)) }); // 2. Dispatch task to waiting thread, spawn new thread if necessary let trap = PanicTrap::new( @@ -424,13 +424,24 @@ impl<'py> Python<'py> { } impl Mailbox { - fn new(task: Task) -> Self { + fn new() -> Self { Self { - inner: Mutex::new(MailboxInner::Task(task)), + inner: Mutex::new(MailboxInner::Abandoned), flag: Condvar::new(), } } + fn init(&self, task: Task) { + use MailboxInner::*; + let mut inner = self.inner.lock(); + match &*inner { + Abandoned => *inner = MailboxInner::Task(task), + Empty | Task(_) | Working | Done => { + unreachable!("initializing existing worker") + } + } + } + fn send_task(&self, task: Task) -> Option { use MailboxInner::*; let mut inner = self.inner.lock(); @@ -505,20 +516,15 @@ impl<'py> Python<'py> { } } - static THREADS: Mutex>> = const_mutex(Vec::new()); - - let dispatch = move || { - while let Some(mailbox) = THREADS.lock().pop() { - match mailbox.send_task(task) { - None => return mailbox, - Some(same_task) => task = same_task, - } - } + thread_local! { + static MAILBOX: Arc = Arc::new(Mailbox::new()); + } - let mailbox = Arc::new(Mailbox::new(task)); + MAILBOX.with(|mailbox| { + if let Some(task) = mailbox.send_task(task) { + let mailbox = Arc::clone(mailbox); - { - let mailbox = Arc::clone(&mailbox); + mailbox.init(task); Builder::new() .name("pyo3 allow_threads runtime".to_owned()) @@ -533,18 +539,12 @@ impl<'py> Python<'py> { .expect("failed to create allow_threads runtime thread"); } - mailbox - }; - - let mailbox = dispatch(); - - // 3. Wait for completion and check result - mailbox.await_done(); + // 3. Wait for completion and check result + mailbox.await_done(); + }); trap.disarm(); - THREADS.lock().push(mailbox); - match result.expect("allow_threads runtime thread did not set result") { Ok(result) => result, Err(payload) => resume_unwind(payload), From 44ba7e7d77cf89a96be1d77f50c0072f1ae7a767 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 24 Dec 2023 11:45:40 +0100 Subject: [PATCH 7/8] Use a builder-like API for allow_threads to simplify choosing a strategy before eventually running the closure. --- examples/word-count/src/lib.rs | 2 +- guide/src/async-await.md | 2 +- guide/src/parallelism.md | 2 +- pyo3-benches/benches/bench_gil.rs | 10 +- src/gil.rs | 6 +- src/marker.rs | 239 ++------------------ src/sync.rs | 260 +++++++++++++++++++++- src/types/bytearray.rs | 4 +- tests/test_class_basics.rs | 2 +- tests/test_compile_error.rs | 1 - tests/test_pyfunction.rs | 2 +- tests/ui/invalid_result_conversion.stderr | 2 +- tests/ui/not_send.rs | 22 +- tests/ui/not_send.stderr | 64 ++++-- tests/ui/not_send2.rs | 12 - tests/ui/not_send2.stderr | 37 --- 16 files changed, 362 insertions(+), 305 deletions(-) delete mode 100644 tests/ui/not_send2.rs delete mode 100644 tests/ui/not_send2.stderr diff --git a/examples/word-count/src/lib.rs b/examples/word-count/src/lib.rs index b7d3a8033a6..95ffcff1d0b 100644 --- a/examples/word-count/src/lib.rs +++ b/examples/word-count/src/lib.rs @@ -18,7 +18,7 @@ fn search_sequential(contents: &str, needle: &str) -> usize { #[pyfunction] fn search_sequential_allow_threads(py: Python<'_>, contents: &str, needle: &str) -> usize { - py.allow_threads(|| search_sequential(contents, needle)) + py.allow_threads().with(|| search_sequential(contents, needle)) } /// Count the occurrences of needle in line, case insensitive diff --git a/guide/src/async-await.md b/guide/src/async-await.md index c14b5d93d84..1b8d038c6b6 100644 --- a/guide/src/async-await.md +++ b/guide/src/async-await.md @@ -60,7 +60,7 @@ where fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let waker = cx.waker(); Python::with_gil(|gil| { - gil.allow_threads(|| pin!(&mut self.0).poll(&mut Context::from_waker(waker))) + gil.allow_threads().with(|| pin!(&mut self.0).poll(&mut Context::from_waker(waker))) }) } } diff --git a/guide/src/parallelism.md b/guide/src/parallelism.md index 195106b2420..ac255c7ae6c 100644 --- a/guide/src/parallelism.md +++ b/guide/src/parallelism.md @@ -69,7 +69,7 @@ To enable parallel execution of this function, the [`Python::allow_threads`] met # } #[pyfunction] fn search_sequential_allow_threads(py: Python<'_>, contents: &str, needle: &str) -> usize { - py.allow_threads(|| search_sequential(contents, needle)) + py.allow_threads().with(|| search_sequential(contents, needle)) } ``` diff --git a/pyo3-benches/benches/bench_gil.rs b/pyo3-benches/benches/bench_gil.rs index a3dd739fd2e..b1b2ca0f0c6 100644 --- a/pyo3-benches/benches/bench_gil.rs +++ b/pyo3-benches/benches/bench_gil.rs @@ -31,14 +31,14 @@ fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { fn bench_allow_threads(b: &mut Bencher<'_>) { Python::with_gil(|py| { - py.allow_threads(|| ()); - b.iter(|| py.allow_threads(|| black_box(42))); + py.allow_threads().with(|| ()); + b.iter(|| py.allow_threads().with(|| black_box(42))); }); } -fn bench_unsafe_allow_threads(b: &mut Bencher<'_>) { +fn bench_local_allow_threads(b: &mut Bencher<'_>) { Python::with_gil(|py| { - b.iter(|| unsafe { py.unsafe_allow_threads(|| black_box(42)) }); + b.iter(|| unsafe { py.allow_threads().local() }.with(|| black_box(42))); }); } @@ -47,7 +47,7 @@ fn criterion_benchmark(c: &mut Criterion) { c.bench_function("clean_acquire_gil", bench_clean_acquire_gil); c.bench_function("dirty_acquire_gil", bench_dirty_acquire_gil); c.bench_function("allow_threads", bench_allow_threads); - c.bench_function("unsafe_allow_threads", bench_unsafe_allow_threads); + c.bench_function("local_allow_threads", bench_local_allow_threads); } criterion_group!(benches, criterion_benchmark); diff --git a/src/gil.rs b/src/gil.rs index 5725ae2df03..1f07e7a6835 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -702,7 +702,7 @@ mod tests { Python::with_gil(|py| { assert!(gil_is_acquired()); - py.allow_threads(move || { + py.allow_threads().with(move || { assert!(!gil_is_acquired()); Python::with_gil(|_| assert!(gil_is_acquired())); @@ -724,7 +724,7 @@ mod tests { let obj = get_object(py); assert!(obj.get_refcnt(py) == 1); // Clone the object without the GIL to use internal tracking - let escaped_ref = py.allow_threads(|| obj.clone()); + let escaped_ref = py.allow_threads().with(|| obj.clone()); // But after the block the refcounts are updated assert!(obj.get_refcnt(py) == 2); drop(escaped_ref); @@ -820,7 +820,7 @@ mod tests { }) }); - let cloned = py.allow_threads(|| { + let cloned = py.allow_threads().with(|| { println!("2. The GIL has been released."); // Wait until the GIL has been acquired on the thread. diff --git a/src/marker.rs b/src/marker.rs index 8ca7e665fcf..eb3be5c1870 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -25,14 +25,14 @@ //! and to close soundness holes implied by thread-local storage hiding such references, //! we do need to run the closure on a dedicated runtime thread. //! -//! ```rust, compile_fail +//! ```rust //! use pyo3::prelude::*; //! use std::rc::Rc; //! //! Python::with_gil(|py| { //! let rc = Rc::new(5); //! -//! py.allow_threads(|| { +//! unsafe { py.allow_threads().local_unconstrained() }.with(|| { //! // This could be fine... //! println!("{:?}", *rc); //! }); @@ -61,7 +61,7 @@ //! let string = PyString::new(py, "foo"); //! //! WRAPPED.set(string, || { -//! py.allow_threads(callback); +//! py.allow_threads().with(callback); //! }); //! }); //! ``` @@ -82,7 +82,7 @@ //! //! If the performance overhead of shunting the closure to another is too high //! or code requires access to thread-local storage established by the calling thread, -//! there is the unsafe escape hatch [`Python::unsafe_allow_threads`] +//! there is the unsafe escape hatch `Python::unsafe_allow_threads` //! which executes the closure directly after suspending the GIL. //! //! However, note establishing the required invariants to soundly call this function @@ -92,8 +92,9 @@ //! [`Rc`]: std::rc::Rc //! [`Py`]: crate::Py use crate::err::{self, PyDowncastError, PyErr, PyResult}; -use crate::gil::{GILGuard, GILPool, SuspendGIL}; +use crate::gil::{GILGuard, GILPool}; use crate::impl_::not_send::NotSend; +use crate::sync::RemoteAllowThreads; use crate::type_object::HasPyGilRef; use crate::types::{ PyAny, PyDict, PyEllipsis, PyModule, PyNone, PyNotImplemented, PyString, PyType, @@ -303,7 +304,7 @@ impl<'py> Python<'py> { /// #[pyfunction] /// fn sum_numbers(py: Python<'_>, numbers: Vec) -> PyResult { /// // We release the GIL here so any other Python threads get a chance to run. - /// py.allow_threads(move || { + /// py.allow_threads().with(move || { /// // An example of an "expensive" Rust calculation /// let sum = numbers.iter().sum(); /// @@ -332,7 +333,7 @@ impl<'py> Python<'py> { /// /// fn parallel_print(py: Python<'_>) { /// let s = PyString::new(py, "This object cannot be accessed without holding the GIL >_<"); - /// py.allow_threads(move || { + /// py.allow_threads().with(move || { /// println!("{:?}", s); // This causes a compile error. /// }); /// } @@ -350,7 +351,7 @@ impl<'py> Python<'py> { /// /// let wrapped = SendWrapper::new(string); /// - /// py.allow_threads(|| { + /// py.allow_threads().with(|| { /// // panicks because this is not the thread which created `wrapped` /// let sneaky: &PyString = *wrapped; /// println!("{:?}", sneaky); @@ -362,195 +363,7 @@ impl<'py> Python<'py> { /// [`PyString`]: crate::types::PyString /// [auto-traits]: https://doc.rust-lang.org/nightly/unstable-book/language-features/auto-traits.html /// [Parallelism]: https://pyo3.rs/main/parallelism.html - pub fn allow_threads(self, f: F) -> T - where - F: Send + FnOnce() -> T, - T: Send, - { - use std::mem::{replace, transmute}; - use std::panic::{catch_unwind, resume_unwind, AssertUnwindSafe}; - use std::sync::Arc; - use std::thread::Builder; - use std::time::Duration; - - use parking_lot::{Condvar, Mutex}; - - use crate::impl_::panic::PanicTrap; - - // Use a guard pattern to handle reacquiring the GIL, - // so that the GIL will be reacquired even if `f` panics. - // The `Send` bound on the closure prevents the user from - // transferring the `Python` token into the closure. - let _guard = unsafe { SuspendGIL::new() }; - - // To close soundness loopholes w.r.t. `send_wrapper` or `scoped-tls`, - // we run the closure on a separate thread so that it cannot - // access thread-local storage from the current thread. - - // 1. Construct a task - struct Task(*mut (dyn FnMut() + 'static)); - unsafe impl Send for Task {} - - let mut f = Some(f); - let mut result = None; - - let mut task = || { - let f = f - .take() - .expect("allow_threads closure called more than once"); - - result = Some(catch_unwind(AssertUnwindSafe(f))); - }; - - // SAFETY: the current thread will block until the closure has returned - let task = Task(unsafe { transmute(&mut task as &mut (dyn FnMut() + '_)) }); - - // 2. Dispatch task to waiting thread, spawn new thread if necessary - let trap = PanicTrap::new( - "allow_threads panicked while stack data was accessed by another thread, please report this as a bug at https://github.com/PyO3/pyo3/issues", - ); - - enum MailboxInner { - Empty, - Task(Task), - Working, - Done, - Abandoned, - } - - struct Mailbox { - inner: Mutex, - flag: Condvar, - } - - impl Mailbox { - fn new() -> Self { - Self { - inner: Mutex::new(MailboxInner::Abandoned), - flag: Condvar::new(), - } - } - - fn init(&self, task: Task) { - use MailboxInner::*; - let mut inner = self.inner.lock(); - match &*inner { - Abandoned => *inner = MailboxInner::Task(task), - Empty | Task(_) | Working | Done => { - unreachable!("initializing existing worker") - } - } - } - - fn send_task(&self, task: Task) -> Option { - use MailboxInner::*; - let mut inner = self.inner.lock(); - match &*inner { - Empty => { - *inner = Task(task); - drop(inner); - self.flag.notify_one(); - None - } - Abandoned => Some(task), - Task(_) | Working | Done => unreachable!("sent task to active worker"), - } - } - - fn recv_task(&self) -> Option { - use MailboxInner::*; - let mut inner = self.inner.lock(); - loop { - match &*inner { - Empty | Done => { - if self - .flag - .wait_for(&mut inner, Duration::from_secs(60)) - .timed_out() - { - *inner = Abandoned; - return None; - } - } - Task(_) => match replace(&mut *inner, Working) { - Task(task) => return Some(task), - _ => unreachable!(), - }, - Working | Abandoned => { - unreachable!("received task on active or exited worker") - } - } - } - } - - fn signal_done(&self) { - use MailboxInner::*; - let mut inner = self.inner.lock(); - match &*inner { - Working => { - *inner = Done; - drop(inner); - self.flag.notify_one(); - } - Empty | Task(_) | Done | Abandoned => { - unreachable!("signalled completion on inactive worker") - } - } - } - - fn await_done(&self) { - use MailboxInner::*; - let mut inner = self.inner.lock(); - loop { - match &*inner { - Done => { - *inner = Empty; - return; - } - Task(_) | Working => self.flag.wait(&mut inner), - Empty | Abandoned => { - unreachable!("awaited completion from inactive worker") - } - } - } - } - } - - thread_local! { - static MAILBOX: Arc = Arc::new(Mailbox::new()); - } - - MAILBOX.with(|mailbox| { - if let Some(task) = mailbox.send_task(task) { - let mailbox = Arc::clone(mailbox); - - mailbox.init(task); - - Builder::new() - .name("pyo3 allow_threads runtime".to_owned()) - .spawn(move || { - while let Some(task) = mailbox.recv_task() { - // SAFETY: all data accessed by `task` will stay alive until it completes - unsafe { (*task.0)() }; - - mailbox.signal_done(); - } - }) - .expect("failed to create allow_threads runtime thread"); - } - - // 3. Wait for completion and check result - mailbox.await_done(); - }); - - trap.disarm(); - - match result.expect("allow_threads runtime thread did not set result") { - Ok(result) => result, - Err(payload) => resume_unwind(payload), - } - } - + /// /// An unsafe version of [`allow_threads`][Self::allow_threads] /// /// This version does _not_ run the given closure on a dedicated runtime thread, @@ -570,13 +383,11 @@ impl<'py> Python<'py> { /// /// let wrapped = SendWrapper::new(string); /// - /// unsafe { - /// py.unsafe_allow_threads(|| { - /// // 💥 Unsound! 💥 - /// let sneaky: &PyString = *wrapped; - /// println!("{:?}", sneaky); - /// }); - /// } + /// unsafe { py.allow_threads().local() }.with(|| { + /// // 💥 Unsound! 💥 + /// let sneaky: &PyString = *wrapped; + /// println!("{:?}", sneaky); + /// }); /// }); /// ``` /// @@ -587,18 +398,8 @@ impl<'py> Python<'py> { /// The caller must ensure that no code within the closure accesses GIL-protected data /// bound to the current thread. Note that this property is highly non-local as for example /// `scoped-tls` allows "smuggling" GIL-bound references using what is essentially global state. - pub unsafe fn unsafe_allow_threads(self, f: F) -> T - where - F: Send + FnOnce() -> T, - T: Send, - { - // Use a guard pattern to handle reacquiring the GIL, - // so that the GIL will be reacquired even if `f` panics. - // The `Send` bound on the closure prevents the user from - // transferring the `Python` token into the closure. - let _guard = SuspendGIL::new(); - - f() + pub fn allow_threads(self) -> RemoteAllowThreads<'py> { + RemoteAllowThreads(self) } /// Evaluates a Python expression in the given context and returns the result. @@ -1110,7 +911,7 @@ mod tests { let b2 = b.clone(); std::thread::spawn(move || Python::with_gil(|_| b2.wait())); - py.allow_threads(|| { + py.allow_threads().with(|| { // If allow_threads does not release the GIL, this will deadlock because // the thread spawned above will never be able to acquire the GIL. b.wait(); @@ -1130,7 +931,7 @@ mod tests { Python::with_gil(|py| { let result = std::panic::catch_unwind(|| unsafe { let py = Python::assume_gil_acquired(); - py.allow_threads(|| { + py.allow_threads().with(|| { panic!("There was a panic!"); }); }); @@ -1155,7 +956,7 @@ mod tests { let a = Arc::new(String::from("foo")); Python::with_gil(|py| { - py.allow_threads(|| { + py.allow_threads().with(|| { drop((list, &mut v, a)); }); }); diff --git a/src/sync.rs b/src/sync.rs index 62843113a8e..c7d8f1e1831 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -1,6 +1,21 @@ //! Synchronization mechanisms based on the Python GIL. -use crate::{types::PyString, types::PyType, Py, PyResult, PyVisit, Python}; -use std::cell::UnsafeCell; +use std::{ + cell::UnsafeCell, + mem::{replace, transmute}, + panic::{catch_unwind, resume_unwind, AssertUnwindSafe}, + sync::Arc, + thread::Builder, + time::Duration, +}; + +use parking_lot::{Condvar, Mutex}; + +use crate::{ + gil::SuspendGIL, + impl_::panic::PanicTrap, + types::{PyString, PyType}, + Py, PyResult, PyVisit, Python, +}; /// Value with concurrent access protected by the GIL. /// @@ -266,6 +281,247 @@ impl Interned { } } +/// TODO +pub struct RemoteAllowThreads<'py>(pub(crate) Python<'py>); + +impl<'py> RemoteAllowThreads<'py> { + /// TODO + /// + /// # Safety + /// + /// TODO + pub unsafe fn local(self) -> LocalAllowThreads<'py> { + LocalAllowThreads(self.0) + } + + /// TODO + /// + /// # Safety + /// + /// TODO + pub unsafe fn local_unconstrained(self) -> LocalUnconstrainedAllowThreads<'py> { + LocalUnconstrainedAllowThreads(self.0) + } + + /// TODO + pub fn with(self, f: F) -> T + where + F: Send + FnOnce() -> T, + T: Send, + { + // Use a guard pattern to handle reacquiring the GIL, + // so that the GIL will be reacquired even if `f` panics. + // The `Send` bound on the closure prevents the user from + // transferring the `Python` token into the closure. + let _guard = unsafe { SuspendGIL::new() }; + + // To close soundness loopholes w.r.t. `send_wrapper` or `scoped-tls`, + // we run the closure on a separate thread so that it cannot + // access thread-local storage from the current thread. + + // 1. Construct a task + let mut f = Some(f); + let mut result = None; + + let mut task = || { + let f = f + .take() + .expect("allow_threads closure called more than once"); + + result = Some(catch_unwind(AssertUnwindSafe(f))); + }; + + // SAFETY: the current thread will block until the closure has returned + let task = Task(unsafe { transmute(&mut task as &mut (dyn FnMut() + '_)) }); + + // 2. Dispatch task to waiting thread, spawn new thread if necessary + let trap = PanicTrap::new( + "allow_threads panicked while stack data was accessed by another thread, please report this as a bug at https://github.com/PyO3/pyo3/issues", + ); + + thread_local! { + static MAILBOX: Arc = Arc::new(Mailbox::new()); + } + + MAILBOX.with(|mailbox| { + if let Some(task) = mailbox.send_task(task) { + let mailbox = Arc::clone(mailbox); + + mailbox.init(task); + + Builder::new() + .name("pyo3 allow_threads runtime".to_owned()) + .spawn(move || { + while let Some(task) = mailbox.recv_task() { + // SAFETY: all data accessed by `task` will stay alive until it completes + unsafe { (*task.0)() }; + + mailbox.signal_done(); + } + }) + .expect("failed to create allow_threads runtime thread"); + } + + // 3. Wait for completion and check result + mailbox.await_done(); + }); + + trap.disarm(); + + match result.expect("allow_threads runtime thread did not set result") { + Ok(result) => result, + Err(payload) => resume_unwind(payload), + } + } +} + +/// TODO +pub struct LocalAllowThreads<'py>(Python<'py>); + +impl LocalAllowThreads<'_> { + /// TODO + pub fn with(self, f: F) -> T + where + F: Send + FnOnce() -> T, + T: Send, + { + // Use a guard pattern to handle reacquiring the GIL, + // so that the GIL will be reacquired even if `f` panics. + // The `Send` bound on the closure prevents the user from + // transferring the `Python` token into the closure. + let _guard = unsafe { SuspendGIL::new() }; + + f() + } +} + +/// TODO +pub struct LocalUnconstrainedAllowThreads<'py>(Python<'py>); + +impl LocalUnconstrainedAllowThreads<'_> { + /// TODO + pub fn with(self, f: F) -> T + where + F: FnOnce() -> T, + { + // Use a guard pattern to handle reacquiring the GIL, + // so that the GIL will be reacquired even if `f` panics. + let _guard = unsafe { SuspendGIL::new() }; + + f() + } +} + +struct Task(*mut (dyn FnMut() + 'static)); + +unsafe impl Send for Task {} + +enum MailboxInner { + Empty, + Task(Task), + Working, + Done, + Abandoned, +} + +struct Mailbox { + inner: Mutex, + flag: Condvar, +} + +impl Mailbox { + fn new() -> Self { + Self { + inner: Mutex::new(MailboxInner::Abandoned), + flag: Condvar::new(), + } + } + + fn init(&self, task: Task) { + use MailboxInner::*; + let mut inner = self.inner.lock(); + match &*inner { + Abandoned => *inner = MailboxInner::Task(task), + Empty | Task(_) | Working | Done => { + unreachable!("initializing existing worker") + } + } + } + + fn send_task(&self, task: Task) -> Option { + use MailboxInner::*; + let mut inner = self.inner.lock(); + match &*inner { + Empty => { + *inner = Task(task); + drop(inner); + self.flag.notify_one(); + None + } + Abandoned => Some(task), + Task(_) | Working | Done => unreachable!("sent task to active worker"), + } + } + + fn recv_task(&self) -> Option { + use MailboxInner::*; + let mut inner = self.inner.lock(); + loop { + match &*inner { + Empty | Done => { + if self + .flag + .wait_for(&mut inner, Duration::from_secs(60)) + .timed_out() + { + *inner = Abandoned; + return None; + } + } + Task(_) => match replace(&mut *inner, Working) { + Task(task) => return Some(task), + _ => unreachable!(), + }, + Working | Abandoned => { + unreachable!("received task on active or exited worker") + } + } + } + } + + fn signal_done(&self) { + use MailboxInner::*; + let mut inner = self.inner.lock(); + match &*inner { + Working => { + *inner = Done; + drop(inner); + self.flag.notify_one(); + } + Empty | Task(_) | Done | Abandoned => { + unreachable!("signalled completion on inactive worker") + } + } + } + + fn await_done(&self) { + use MailboxInner::*; + let mut inner = self.inner.lock(); + loop { + match &*inner { + Done => { + *inner = Empty; + return; + } + Task(_) | Working => self.flag.wait(&mut inner), + Empty | Abandoned => { + unreachable!("awaited completion from inactive worker") + } + } + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index a90fec82273..3418876e4a6 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -176,7 +176,7 @@ impl PyByteArray { /// /// // This explicitly yields control back to the Python interpreter... /// // ...but it's not always this obvious. Many things do this implicitly. - /// py.allow_threads(|| { + /// py.allow_threads().with(|| { /// // Python code could be mutating through its handle to `bytes`, /// // which makes reading it a data race, which is undefined behavior. /// println!("{:?}", slice[0]); @@ -338,7 +338,7 @@ pub trait PyByteArrayMethods<'py> { /// /// // This explicitly yields control back to the Python interpreter... /// // ...but it's not always this obvious. Many things do this implicitly. - /// py.allow_threads(|| { + /// py.allow_threads().with(|| { /// // Python code could be mutating through its handle to `bytes`, /// // which makes reading it a data race, which is undefined behavior. /// println!("{:?}", slice[0]); diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index bbf37a2d66b..c349ae248fa 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -572,7 +572,7 @@ fn drop_unsendable_elsewhere() { ) .unwrap(); - py.allow_threads(|| { + py.allow_threads().with(|| { spawn(move || { Python::with_gil(move |_py| { drop(unsendable); diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 0c278bdfa62..dfefd18b64f 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -36,7 +36,6 @@ fn test_compile_errors() { #[cfg(not(any(windows, feature = "eyre", feature = "anyhow")))] t.compile_fail("tests/ui/invalid_result_conversion.rs"); t.compile_fail("tests/ui/not_send.rs"); - t.compile_fail("tests/ui/not_send2.rs"); t.compile_fail("tests/ui/get_set_all.rs"); t.compile_fail("tests/ui/traverse.rs"); } diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index a2f44be6bbd..ea23bfcebb7 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -491,7 +491,7 @@ fn return_value_borrows_from_arguments<'py>( key: &'py Key, value: &'py Value, ) -> HashMap<&'py str, i32> { - py.allow_threads(move || { + py.allow_threads().with(move || { let mut map = HashMap::new(); map.insert(key.0.as_str(), value.0); map diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index f1a429a5abd..b3e65517e36 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -5,9 +5,9 @@ error[E0277]: the trait bound `PyErr: From` is not satisfied | ^^^^^^^^^^^^^ the trait `From` is not implemented for `PyErr` | = help: the following other types implement trait `From`: + > > > - > >> >> >> diff --git a/tests/ui/not_send.rs b/tests/ui/not_send.rs index 6566f2d7de5..dfaf7fa1fb9 100644 --- a/tests/ui/not_send.rs +++ b/tests/ui/not_send.rs @@ -1,11 +1,23 @@ use pyo3::prelude::*; +use pyo3::types::PyString; -fn test_not_send_allow_threads(py: Python<'_>) { - py.allow_threads(|| { drop(py); }); +fn allow_thread_prevents_token() { + Python::with_gil(|py| { + py.allow_threads().with(|| { drop(py); }); + }) } -fn main() { +fn allow_thread_prevents_gil_ref() { Python::with_gil(|py| { - test_not_send_allow_threads(py); - }) + let string = PyString::new(py, "foo"); + + py.allow_threads().with(|| { + println!("{:?}", string); + }); + }); +} + +fn main() { + allow_thread_prevents_token(); + allow_thread_prevents_gil_ref(); } diff --git a/tests/ui/not_send.stderr b/tests/ui/not_send.stderr index da4f776b7f6..78936ba78a8 100644 --- a/tests/ui/not_send.stderr +++ b/tests/ui/not_send.stderr @@ -1,10 +1,10 @@ error[E0277]: `*mut pyo3::Python<'static>` cannot be shared between threads safely - --> tests/ui/not_send.rs:4:22 + --> tests/ui/not_send.rs:6:33 | -4 | py.allow_threads(|| { drop(py); }); - | ------------- ^^^^^^^^^^^^^^^^ `*mut pyo3::Python<'static>` cannot be shared between threads safely - | | - | required by a bound introduced by this call +6 | py.allow_threads().with(|| { drop(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>>` @@ -30,15 +30,53 @@ 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/not_send.rs:4:22 + --> tests/ui/not_send.rs:6:33 | -4 | py.allow_threads(|| { drop(py); }); - | ^^ -note: required by a bound in `pyo3::Python::<'py>::allow_threads` - --> src/marker.rs +6 | py.allow_threads().with(|| { drop(py); }); + | ^^ +note: required by a bound in `RemoteAllowThreads::<'py>::with` + --> src/sync.rs | - | pub fn allow_threads(self, f: F) -> T - | ------------- required by a bound in this associated function + | pub fn with(self, f: F) -> T + | ---- required by a bound in this associated function | where | F: Send + FnOnce() -> T, - | ^^^^ required by this bound in `Python::<'py>::allow_threads` + | ^^^^ required by this bound in `RemoteAllowThreads::<'py>::with` + +error[E0277]: `UnsafeCell` cannot be shared between threads safely + --> tests/ui/not_send.rs:14:33 + | +14 | py.allow_threads().with(|| { + | ____________________________----_^ + | | | + | | required by a bound introduced by this call +15 | | println!("{:?}", string); +16 | | }); + | |_________^ `UnsafeCell` cannot be shared between threads safely + | + = help: within `&PyString`, the trait `Sync` is not implemented for `UnsafeCell` +note: required because it appears within the type `PyAny` + --> src/types/any.rs + | + | pub struct PyAny(UnsafeCell); + | ^^^^^ +note: required because it appears within the type `PyString` + --> src/types/string.rs + | + | pub struct PyString(PyAny); + | ^^^^^^^^ + = note: required because it appears within the type `&PyString` + = note: required for `&&PyString` to implement `Send` +note: required because it's used within this closure + --> tests/ui/not_send.rs:14:33 + | +14 | py.allow_threads().with(|| { + | ^^ +note: required by a bound in `RemoteAllowThreads::<'py>::with` + --> src/sync.rs + | + | pub fn with(self, f: F) -> T + | ---- required by a bound in this associated function + | where + | F: Send + FnOnce() -> T, + | ^^^^ required by this bound in `RemoteAllowThreads::<'py>::with` diff --git a/tests/ui/not_send2.rs b/tests/ui/not_send2.rs deleted file mode 100644 index 4eb0a9f0f09..00000000000 --- a/tests/ui/not_send2.rs +++ /dev/null @@ -1,12 +0,0 @@ -use pyo3::prelude::*; -use pyo3::types::PyString; - -fn main() { - Python::with_gil(|py| { - let string = PyString::new(py, "foo"); - - py.allow_threads(|| { - println!("{:?}", string); - }); - }); -} \ No newline at end of file diff --git a/tests/ui/not_send2.stderr b/tests/ui/not_send2.stderr deleted file mode 100644 index 075b28d8cc5..00000000000 --- a/tests/ui/not_send2.stderr +++ /dev/null @@ -1,37 +0,0 @@ -error[E0277]: `UnsafeCell` cannot be shared between threads safely - --> tests/ui/not_send2.rs:8:26 - | -8 | py.allow_threads(|| { - | ____________-------------_^ - | | | - | | required by a bound introduced by this call -9 | | println!("{:?}", string); -10 | | }); - | |_________^ `UnsafeCell` cannot be shared between threads safely - | - = help: within `&PyString`, the trait `Sync` is not implemented for `UnsafeCell` -note: required because it appears within the type `PyAny` - --> src/types/any.rs - | - | pub struct PyAny(UnsafeCell); - | ^^^^^ -note: required because it appears within the type `PyString` - --> src/types/string.rs - | - | pub struct PyString(PyAny); - | ^^^^^^^^ - = note: required because it appears within the type `&PyString` - = note: required for `&&PyString` to implement `Send` -note: required because it's used within this closure - --> tests/ui/not_send2.rs:8:26 - | -8 | py.allow_threads(|| { - | ^^ -note: required by a bound in `pyo3::Python::<'py>::allow_threads` - --> src/marker.rs - | - | pub fn allow_threads(self, f: F) -> T - | ------------- required by a bound in this associated function - | where - | F: Send + FnOnce() -> T, - | ^^^^ required by this bound in `Python::<'py>::allow_threads` From 34d5fd68904731c4cbc4d1e521d3679589a1ad00 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Fri, 22 Dec 2023 18:54:57 +0100 Subject: [PATCH 8/8] WIP: Add changelog and migration guide entries. --- guide/src/migration.md | 4 ++++ newsfragments/3646.changed.md | 1 + 2 files changed, 5 insertions(+) create mode 100644 newsfragments/3646.changed.md diff --git a/guide/src/migration.md b/guide/src/migration.md index ca3a2172576..fa144432d83 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -75,6 +75,10 @@ Python::with_gil(|py| { }); ``` +### `Python::allow_threads` was split into `Python::safe_allow_threads` and `Python::unsafe_allow_threads` + +TODO + ### `Iter(A)NextOutput` are deprecated The `__next__` and `__anext__` magic methods can now return any type convertible into Python objects directly just like all other `#[pymethods]`. The `IterNextOutput` used by `__next__` and `IterANextOutput` used by `__anext__` are subsequently deprecated. Most importantly, this change allows returning an awaitable from `__anext__` without non-sensically wrapping it into `Yield` or `Some`. Only the return types `Option` and `Result, E>` are still handled in a special manner where `Some(val)` yields `val` and `None` stops iteration. diff --git a/newsfragments/3646.changed.md b/newsfragments/3646.changed.md new file mode 100644 index 00000000000..f547b1ed989 --- /dev/null +++ b/newsfragments/3646.changed.md @@ -0,0 +1 @@ +`Python::allow_threads` now returns a builder-like object which allows choosing a strategy for running the closure, as the default one which now closes TLS-based soundness loopholes by dispatching the closure to a worker thread has significantly changed performance characteristics.