diff --git a/Cargo.toml b/Cargo.toml index 921e551751c..4de78df063a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,6 +56,7 @@ chrono-tz = ">= 0.6, < 0.10" trybuild = ">=1.0.70" proptest = { version = "1.0", default-features = false, features = ["std"] } send_wrapper = "0.6" +scoped-tls-hkt = "0.1" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.61" rayon = "1.6.1" diff --git a/examples/word-count/src/lib.rs b/examples/word-count/src/lib.rs index 5bc73df97a4..c76af8b3de5 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 27574181804..8d38b1ef89f 100644 --- a/guide/src/async-await.md +++ b/guide/src/async-await.md @@ -67,7 +67,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/features.md b/guide/src/features.md index 6a25d40cedc..ef49f317804 100644 --- a/guide/src/features.md +++ b/guide/src/features.md @@ -107,10 +107,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/guide/src/migration.md b/guide/src/migration.md index f56db2a5fc7..dac08d0bad1 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -135,6 +135,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
Click to expand diff --git a/guide/src/parallelism.md b/guide/src/parallelism.md index a288b14be19..4d88facc469 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/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. diff --git a/pyo3-benches/benches/bench_gil.rs b/pyo3-benches/benches/bench_gil.rs index cede8836f35..fcb7a278cf0 100644 --- a/pyo3-benches/benches/bench_gil.rs +++ b/pyo3-benches/benches/bench_gil.rs @@ -1,4 +1,5 @@ use codspeed_criterion_compat::{criterion_group, criterion_main, Bencher, Criterion}; +use std::hint::black_box; use pyo3::prelude::*; @@ -13,9 +14,24 @@ fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { b.iter(|| Python::with_gil(|py| obj.clone_ref(py))); } +fn bench_allow_threads(b: &mut Bencher<'_>) { + Python::with_gil(|py| { + py.allow_threads().with(|| ()); + b.iter(|| py.allow_threads().with(|| black_box(42))); + }); +} + +fn bench_local_allow_threads(b: &mut Bencher<'_>) { + Python::with_gil(|py| { + b.iter(|| unsafe { py.allow_threads().local() }.with(|| black_box(42))); + }); +} + 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("local_allow_threads", bench_local_allow_threads); } criterion_group!(benches, criterion_benchmark); diff --git a/src/err/mod.rs b/src/err/mod.rs index 6bfe1a6cc99..f87952091ad 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -39,8 +39,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/gil.rs b/src/gil.rs index 6f97011b71c..17deca7c7f1 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -735,23 +735,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().with(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()); + } } #[cfg(feature = "py-clone")] @@ -763,7 +765,7 @@ mod tests { let obj = get_object(py); assert!(obj.get_refcnt(py) == 1); // Clone the object without the GIL which should panic - py.allow_threads(|| obj.clone()); + py.allow_threads().with(|| obj.clone()); }); } diff --git a/src/instance.rs b/src/instance.rs index 82b05e782ff..bb1a677b069 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -922,8 +922,6 @@ impl IntoPy 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 a9c5bd0b731..52875146936 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. @@ -115,10 +114,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. @@ -314,7 +309,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, IntoPy, ToPyObject}; #[cfg(feature = "gil-refs")] diff --git a/src/marker.rs b/src/marker.rs index 1f4655d9656..d706c79fb3b 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -14,113 +14,89 @@ //! 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: +//! That API is provided by [`Python::allow_threads`] and enforced via the [`Send`] bound on the +//! closure and the return type. //! -//! ```rust -//! # #![allow(dead_code)] -//! 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. -//! -//! 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 +//! 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. //! -//! 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. -//! -//! ```rust, compile_fail -//! # #[cfg(feature = "nightly")] -//! # compile_error!("this actually works on nightly") +//! ```rust //! use pyo3::prelude::*; //! use std::rc::Rc; //! -//! fn main() { -//! Python::with_gil(|py| { -//! let rc = Rc::new(5); +//! Python::with_gil(|py| { +//! let rc = Rc::new(5); //! -//! py.allow_threads(|| { -//! // This would actually be fine... -//! println!("{:?}", *rc); -//! }); +//! unsafe { py.allow_threads().local_unconstrained() }.with(|| { +//! // This could 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: +//! 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 send_wrapper::SendWrapper; +//! use scoped_tls_hkt::scoped_thread_local; +//! +//! scoped_thread_local!(static WRAPPED: for<'py> &'py Bound<'py, PyString>); +//! +//! fn callback() { +//! WRAPPED.with(|smuggled: &Bound<'_, PyString>| { +//! println!("{:?}", smuggled); +//! }); +//! } //! //! Python::with_gil(|py| { //! let string = PyString::new_bound(py, "foo"); //! -//! let wrapped = SendWrapper::new(string); -//! -//! py.allow_threads(|| { -//! # #[cfg(not(feature = "nightly"))] -//! # { -//! // 💥 Unsound! 💥 -//! let smuggled: &Bound<'_, PyString> = &*wrapped; -//! println!("{:?}", smuggled); -//! # } +//! WRAPPED.set(&string, || { +//! py.allow_threads().with(callback); //! }); //! }); //! ``` //! -//! For now the answer to that is "don't do that". +//! 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`. //! -//! # A proper implementation using an auto trait +//! 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. //! -//! However on nightly Rust and when PyO3's `nightly` feature is -//! enabled, `Ungil` is defined as the following: +//! These long-lived background threads are named `pyo3 allow_threads runtime` +//! to facilitate diagnosing any performance issues they might cause on the process level. //! -//! ```rust -//! # #[cfg(any())] -//! # { -//! #![feature(auto_traits, negative_impls)] +//! 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`. //! -//! pub unsafe auto trait Ungil {} +//! 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. //! -//! // It is unimplemented for the `Python` struct and Python objects. -//! impl !Ungil for Python<'_> {} -//! impl !Ungil for ffi::PyObject {} +//! However, note establishing the required invariants to soundly call this function +//! requires highly non-local reasoning as thread-local storage allows "smuggling" GIL-bound data +//! using what is essentially global state. //! -//! // `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, PyErr, PyResult}; use crate::ffi_ptr_ext::FfiPtrExt; -use crate::gil::{GILGuard, SuspendGIL}; +use crate::gil::GILGuard; use crate::impl_::not_send::NotSend; use crate::py_result_ext::PyResultExt; +use crate::sync::RemoteAllowThreads; use crate::types::any::PyAnyMethods; use crate::types::{ PyAny, PyDict, PyEllipsis, PyModule, PyNone, PyNotImplemented, PyString, PyType, @@ -134,175 +110,6 @@ 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_bound(py, "foo"); -/// -/// let wrapped = SendWrapper::new(string); -/// -/// py.allow_threads(|| { -/// let sneaky: &Bound<'_, 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_bound(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_bound(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 - #[allow(deprecated)] - 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; - /// A marker token that represents holding the GIL. /// /// It serves three main purposes: @@ -453,17 +260,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 [`Ungil`] 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 data 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 /// /// ``` @@ -472,7 +281,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(); /// @@ -501,27 +310,73 @@ impl<'py> Python<'py> { /// /// fn parallel_print(py: Python<'_>) { /// let s = PyString::new_bound(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. /// }); /// } /// ``` /// + /// # 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_bound(py, "foo"); + /// + /// let wrapped = SendWrapper::new(string); + /// + /// py.allow_threads().with(|| { + /// // panicks because this is not the thread which created `wrapped` + /// let sneaky: &Bound<'_, 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, - { - // 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() + /// + /// 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_bound(py, "foo"); + /// + /// let wrapped = SendWrapper::new(string); + /// + /// unsafe { py.allow_threads().local() }.with(|| { + /// // 💥 Unsound! 💥 + /// let sneaky: &Bound<'_, PyString> = &*wrapped; + /// println!("{:?}", sneaky); + /// }); + /// }); + /// ``` + /// + /// See the [module level documentation](self) for more information. + /// + /// # 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 data using what is essentially global state. + pub fn allow_threads(self) -> RemoteAllowThreads<'py> { + RemoteAllowThreads(self) } /// Deprecated version of [`Python::eval_bound`] @@ -1073,81 +928,6 @@ impl<'py> Python<'py> { } } -impl Python<'_> { - /// Creates a scope using a new pool for managing PyO3's GIL Refs. This has no functional - /// use for code which does not use the deprecated GIL Refs API. - /// - /// This is a safe alterantive to [`new_pool`][Self::new_pool] as - /// it limits the closure to using the new GIL token at the cost of - /// 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. - /// #[allow(deprecated)] // `with_pool` is not needed in code not using the GIL Refs API - /// 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 - /// # #![allow(deprecated)] - /// # 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] - #[cfg(feature = "gil-refs")] - #[deprecated( - since = "0.21.0", - note = "code not using the GIL Refs API can safely remove use of `Python::with_pool`" - )] - #[allow(deprecated)] - pub fn with_pool(&self, f: F) -> R - where - F: for<'py> FnOnce(Python<'py>) -> R + Ungil, - { - // 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. /// @@ -1225,7 +1005,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(); @@ -1245,7 +1025,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!"); }); }); @@ -1268,7 +1048,7 @@ mod tests { let a = std::sync::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 856ba84d1e3..ccd0d19c152 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -4,11 +4,21 @@ //! are likely to undergo significant developments in the future. //! //! [PEP 703]: https://peps.python.org/pep-703/ +use std::{ + cell::UnsafeCell, + mem::{replace, transmute}, + panic::{catch_unwind, resume_unwind, AssertUnwindSafe}, + sync::{Arc, Condvar, Mutex}, + thread::Builder, + time::Duration, +}; + use crate::{ - types::{any::PyAnyMethods, PyString, PyType}, + gil::SuspendGIL, + impl_::panic::PanicTrap, + types::{PyAnyMethods, PyString, PyType}, Bound, Py, PyResult, PyVisit, Python, }; -use std::cell::UnsafeCell; /// Value with concurrent access protected by the GIL. /// @@ -281,6 +291,248 @@ 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().unwrap(); + 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().unwrap(); + 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().unwrap(); + loop { + match &*inner { + Empty | Done => { + let (guard, result) = self + .flag + .wait_timeout(inner, Duration::from_secs(60)) + .unwrap(); + inner = guard; + if result.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().unwrap(); + 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().unwrap(); + loop { + match &*inner { + Done => { + *inner = Empty; + return; + } + Task(_) | Working => inner = self.flag.wait(inner).unwrap(), + 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 ec3d7eafbfd..bec0120136d 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -225,7 +225,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]); @@ -387,7 +387,7 @@ pub trait PyByteArrayMethods<'py>: crate::sealed::Sealed { /// /// // 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 d0e745377c8..6f19eafbef0 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -590,7 +590,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 2b32de2fcfa..bc787f353a9 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -49,7 +49,6 @@ fn test_compile_errors() { )))] 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"); #[cfg(feature = "experimental-declarative-modules")] diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 4a90f3f9d99..aee9dcf8911 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -502,7 +502,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/not_send.rs b/tests/ui/not_send.rs index 6566f2d7de5..28b03769d2a 100644 --- a/tests/ui/not_send.rs +++ b/tests/ui/not_send.rs @@ -1,11 +1,25 @@ 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_bound_data() { Python::with_gil(|py| { - test_not_send_allow_threads(py); - }) + let string = PyString::new_bound(py, "foo"); + + py.allow_threads().with(|| { + println!("{:?}", string); + }); + }); +} + +fn main() { + allow_thread_prevents_token(); + allow_thread_prevents_gil_bound_data(); } diff --git a/tests/ui/not_send.stderr b/tests/ui/not_send.stderr index 8007c2f4e54..c2bbbb31403 100644 --- a/tests/ui/not_send.stderr +++ b/tests/ui/not_send.stderr @@ -1,12 +1,15 @@ 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(|| { + | ____________________________----_^ + | | | + | | required by a bound introduced by this call +7 | | drop(py); +8 | | }); + | |_________^ `*mut pyo3::Python<'static>` cannot be shared between threads safely | - = help: within `pyo3::Python<'_>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>`, which is required by `{closure@$DIR/tests/ui/not_send.rs:4:22: 4:24}: Ungil` + = help: within `pyo3::Python<'_>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>`, which is required by `{closure@$DIR/tests/ui/not_send.rs:6:33: 6:35}: Send` note: required because it appears within the type `PhantomData<*mut pyo3::Python<'static>>` --> $RUST/core/src/marker.rs | @@ -30,16 +33,68 @@ note: required because it appears within the type `pyo3::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 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 +6 | py.allow_threads().with(|| { + | ^^ +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: Ungil + FnOnce() -> T, - | ^^^^^ required by this bound in `Python::<'py>::allow_threads` + | F: Send + FnOnce() -> T, + | ^^^^ required by this bound in `RemoteAllowThreads::<'py>::with` + +error[E0277]: `*mut pyo3::Python<'static>` cannot be shared between threads safely + --> tests/ui/not_send.rs:16:33 + | +16 | py.allow_threads().with(|| { + | ____________________________----_^ + | | | + | | required by a bound introduced by this call +17 | | println!("{:?}", string); +18 | | }); + | |_________^ `*mut pyo3::Python<'static>` cannot be shared between threads safely + | + = help: within `pyo3::Bound<'_, PyString>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>`, which is required by `{closure@$DIR/tests/ui/not_send.rs:16:33: 16:35}: Send` +note: required because it appears within the type `PhantomData<*mut pyo3::Python<'static>>` + --> $RUST/core/src/marker.rs + | + | pub struct PhantomData; + | ^^^^^^^^^^^ +note: required because it appears within the type `impl_::not_send::NotSend` + --> src/impl_/not_send.rs + | + | pub(crate) struct NotSend(PhantomData<*mut Python<'static>>); + | ^^^^^^^ + = note: required because it appears within the type `(&pyo3::gil::GILGuard, impl_::not_send::NotSend)` +note: required because it appears within the type `PhantomData<(&pyo3::gil::GILGuard, impl_::not_send::NotSend)>` + --> $RUST/core/src/marker.rs + | + | pub struct PhantomData; + | ^^^^^^^^^^^ +note: required because it appears within the type `pyo3::Python<'_>` + --> src/marker.rs + | + | pub struct Python<'py>(PhantomData<(&'py GILGuard, NotSend)>); + | ^^^^^^ +note: required because it appears within the type `pyo3::Bound<'_, PyString>` + --> src/instance.rs + | + | pub struct Bound<'py, T>(Python<'py>, ManuallyDrop>); + | ^^^^^ + = note: required for `&pyo3::Bound<'_, PyString>` to implement `Send` +note: required because it's used within this closure + --> tests/ui/not_send.rs:16:33 + | +16 | 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 fa99e602ba0..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_bound(py, "foo"); - - py.allow_threads(|| { - println!("{:?}", string); - }); - }); -} diff --git a/tests/ui/not_send2.stderr b/tests/ui/not_send2.stderr deleted file mode 100644 index 52a4e6a7208..00000000000 --- a/tests/ui/not_send2.stderr +++ /dev/null @@ -1,53 +0,0 @@ -error[E0277]: `*mut pyo3::Python<'static>` 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 | | }); - | |_________^ `*mut pyo3::Python<'static>` cannot be shared between threads safely - | - = help: within `pyo3::Bound<'_, PyString>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>`, which is required by `{closure@$DIR/tests/ui/not_send2.rs:8:26: 8:28}: Ungil` -note: required because it appears within the type `PhantomData<*mut pyo3::Python<'static>>` - --> $RUST/core/src/marker.rs - | - | pub struct PhantomData; - | ^^^^^^^^^^^ -note: required because it appears within the type `impl_::not_send::NotSend` - --> src/impl_/not_send.rs - | - | pub(crate) struct NotSend(PhantomData<*mut Python<'static>>); - | ^^^^^^^ - = note: required because it appears within the type `(&pyo3::gil::GILGuard, impl_::not_send::NotSend)` -note: required because it appears within the type `PhantomData<(&pyo3::gil::GILGuard, impl_::not_send::NotSend)>` - --> $RUST/core/src/marker.rs - | - | pub struct PhantomData; - | ^^^^^^^^^^^ -note: required because it appears within the type `pyo3::Python<'_>` - --> src/marker.rs - | - | pub struct Python<'py>(PhantomData<(&'py GILGuard, NotSend)>); - | ^^^^^^ -note: required because it appears within the type `pyo3::Bound<'_, PyString>` - --> src/instance.rs - | - | pub struct Bound<'py, T>(Python<'py>, ManuallyDrop>); - | ^^^^^ - = note: required for `&pyo3::Bound<'_, 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 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`