diff --git a/newsfragments/3176.fixed.md b/newsfragments/3176.fixed.md new file mode 100644 index 00000000000..9c7decf7f04 --- /dev/null +++ b/newsfragments/3176.fixed.md @@ -0,0 +1 @@ +Avoid running the `Drop` implementations of unsendable classes on other threads diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 56472e3e2a1..312144a69c0 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1,5 +1,5 @@ use crate::{ - exceptions::{PyAttributeError, PyNotImplementedError, PyValueError}, + exceptions::{PyAttributeError, PyNotImplementedError, PyRuntimeError, PyValueError}, ffi, impl_::freelist::FreeList, impl_::pycell::{GetBorrowChecker, PyClassMutability}, @@ -884,6 +884,7 @@ impl PyClassNewTextSignature for &'_ PyClassImplCollector { #[doc(hidden)] pub trait PyClassThreadChecker: Sized { fn ensure(&self); + fn can_drop(&self, py: Python<'_>) -> bool; fn new() -> Self; private_decl! {} } @@ -894,6 +895,9 @@ pub struct ThreadCheckerStub(PhantomData); impl PyClassThreadChecker for ThreadCheckerStub { fn ensure(&self) {} + fn can_drop(&self, _py: Python<'_>) -> bool { + true + } #[inline] fn new() -> Self { ThreadCheckerStub(PhantomData) @@ -903,6 +907,9 @@ impl PyClassThreadChecker for ThreadCheckerStub { impl PyClassThreadChecker for ThreadCheckerStub { fn ensure(&self) {} + fn can_drop(&self, _py: Python<'_>) -> bool { + true + } #[inline] fn new() -> Self { ThreadCheckerStub(PhantomData) @@ -924,6 +931,18 @@ impl PyClassThreadChecker for ThreadCheckerImpl { std::any::type_name::() ); } + fn can_drop(&self, py: Python<'_>) -> bool { + if thread::current().id() != self.0 { + PyRuntimeError::new_err(format!( + "{} is unsendbale, but is dropped on another thread!", + std::any::type_name::() + )) + .write_unraisable(py, None); + return false; + } + + true + } fn new() -> Self { ThreadCheckerImpl(thread::current().id(), PhantomData) } @@ -944,6 +963,9 @@ impl PyClassThreadChecker fn ensure(&self) { self.1.ensure(); } + fn can_drop(&self, py: Python<'_>) -> bool { + self.1.can_drop(py) + } fn new() -> Self { ThreadCheckerInherited(PhantomData, U::ThreadChecker::new()) } diff --git a/src/pycell.rs b/src/pycell.rs index 7e301c3e2d7..44ee822fffb 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -937,7 +937,9 @@ where unsafe fn tp_dealloc(py: Python<'_>, slf: *mut ffi::PyObject) { // Safety: Python only calls tp_dealloc when no references to the object remain. let cell = &mut *(slf as *mut PyCell); - ManuallyDrop::drop(&mut cell.contents.value); + if cell.contents.thread_checker.can_drop(py) { + ManuallyDrop::drop(&mut cell.contents.value); + } cell.contents.dict.clear_dict(py); cell.contents.weakref.clear_weakrefs(slf, py); ::LayoutAsBase::tp_dealloc(py, slf) diff --git a/tests/common.rs b/tests/common.rs index 75a246a9e7d..1003d54658e 100644 --- a/tests/common.rs +++ b/tests/common.rs @@ -1,5 +1,8 @@ //! Some common macros for tests +#[cfg(all(feature = "macros", Py_3_8))] +use pyo3::prelude::*; + #[macro_export] macro_rules! py_assert { ($py:expr, $($val:ident)+, $assertion:literal) => { @@ -41,3 +44,50 @@ macro_rules! py_expect_exception { err }}; } + +// sys.unraisablehook not available until Python 3.8 +#[cfg(all(feature = "macros", Py_3_8))] +#[pyclass] +pub struct UnraisableCapture { + pub capture: Option<(PyErr, PyObject)>, + old_hook: Option, +} + +#[cfg(all(feature = "macros", Py_3_8))] +#[pymethods] +impl UnraisableCapture { + pub fn hook(&mut self, unraisable: &PyAny) { + let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap()); + let instance = unraisable.getattr("object").unwrap(); + self.capture = Some((err, instance.into())); + } +} + +#[cfg(all(feature = "macros", Py_3_8))] +impl UnraisableCapture { + pub fn install(py: Python<'_>) -> Py { + let sys = py.import("sys").unwrap(); + let old_hook = sys.getattr("unraisablehook").unwrap().into(); + + let capture = Py::new( + py, + UnraisableCapture { + capture: None, + old_hook: Some(old_hook), + }, + ) + .unwrap(); + + sys.setattr("unraisablehook", capture.getattr(py, "hook").unwrap()) + .unwrap(); + + capture + } + + pub fn uninstall(&mut self, py: Python<'_>) { + let old_hook = self.old_hook.take().unwrap(); + + let sys = py.import("sys").unwrap(); + sys.setattr("unraisablehook", old_hook).unwrap(); + } +} diff --git a/tests/test_buffer_protocol.rs b/tests/test_buffer_protocol.rs index c3843909c8f..a9c0ff8ea17 100644 --- a/tests/test_buffer_protocol.rs +++ b/tests/test_buffer_protocol.rs @@ -96,6 +96,7 @@ fn test_buffer_referenced() { #[test] #[cfg(Py_3_8)] // sys.unraisablehook not available until Python 3.8 fn test_releasebuffer_unraisable_error() { + use common::UnraisableCapture; use pyo3::exceptions::PyValueError; #[pyclass] @@ -117,27 +118,8 @@ fn test_releasebuffer_unraisable_error() { } } - #[pyclass] - struct UnraisableCapture { - capture: Option<(PyErr, PyObject)>, - } - - #[pymethods] - impl UnraisableCapture { - fn hook(&mut self, unraisable: &PyAny) { - let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap()); - let instance = unraisable.getattr("object").unwrap(); - self.capture = Some((err, instance.into())); - } - } - Python::with_gil(|py| { - let sys = py.import("sys").unwrap(); - let old_hook = sys.getattr("unraisablehook").unwrap(); - let capture = Py::new(py, UnraisableCapture { capture: None }).unwrap(); - - sys.setattr("unraisablehook", capture.getattr(py, "hook").unwrap()) - .unwrap(); + let capture = UnraisableCapture::install(py); let instance = Py::new(py, ReleaseBufferError {}).unwrap(); let env = [("ob", instance.clone())].into_py_dict(py); @@ -150,7 +132,7 @@ fn test_releasebuffer_unraisable_error() { assert_eq!(err.to_string(), "ValueError: oh dear"); assert!(object.is(&instance)); - sys.setattr("unraisablehook", old_hook).unwrap(); + capture.borrow_mut(py).uninstall(py); }); } diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index ac3c98d0844..a0ec4b0c1b2 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -228,31 +228,40 @@ impl UnsendableChild { } fn test_unsendable() -> PyResult<()> { - let obj = std::thread::spawn(|| -> PyResult<_> { + let obj = Python::with_gil(|py| -> PyResult<_> { + let obj: Py = PyType::new::(py).call1((5,))?.extract()?; + + // Accessing the value inside this thread should not panic + let caught_panic = + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| -> PyResult<_> { + assert_eq!(obj.as_ref(py).getattr("value")?.extract::()?, 5); + Ok(()) + })) + .is_err(); + + assert!(!caught_panic); + Ok(obj) + })?; + + let keep_obj_here = obj.clone(); + + let caught_panic = std::thread::spawn(move || { + // This access must panic Python::with_gil(|py| { - let obj: Py = PyType::new::(py).call1((5,))?.extract()?; - - // Accessing the value inside this thread should not panic - let caught_panic = - std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| -> PyResult<_> { - assert_eq!(obj.as_ref(py).getattr("value")?.extract::()?, 5); - Ok(()) - })) - .is_err(); - - assert!(!caught_panic); - Ok(obj) - }) + obj.borrow(py); + }); }) - .join() - .unwrap()?; + .join(); - // This access must panic - Python::with_gil(|py| { - obj.borrow(py); - }); + Python::with_gil(|_py| drop(keep_obj_here)); - panic!("Borrowing unsendable from receiving thread did not panic."); + if let Err(err) = caught_panic { + if let Some(msg) = err.downcast_ref::() { + panic!("{}", msg); + } + } + + Ok(()) } /// If a class is marked as `unsendable`, it panics when accessed by another thread. @@ -526,3 +535,58 @@ fn access_frozen_class_without_gil() { assert_eq!(py_counter.get().value.load(Ordering::Relaxed), 1); } + +#[test] +#[cfg(Py_3_8)] // sys.unraisablehook not available until Python 3.8 +#[cfg_attr(target_arch = "wasm32", ignore)] +fn drop_unsendable_elsewhere() { + use common::UnraisableCapture; + use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, + }; + use std::thread::spawn; + + #[pyclass(unsendable)] + struct Unsendable { + dropped: Arc, + } + + impl Drop for Unsendable { + fn drop(&mut self) { + self.dropped.store(true, Ordering::SeqCst); + } + } + + Python::with_gil(|py| { + let capture = UnraisableCapture::install(py); + + let dropped = Arc::new(AtomicBool::new(false)); + + let unsendable = Py::new( + py, + Unsendable { + dropped: dropped.clone(), + }, + ) + .unwrap(); + + py.allow_threads(|| { + spawn(move || { + Python::with_gil(move |_py| { + drop(unsendable); + }); + }) + .join() + .unwrap(); + }); + + assert!(!dropped.load(Ordering::SeqCst)); + + let (err, object) = capture.borrow_mut(py).capture.take().unwrap(); + assert_eq!(err.to_string(), "RuntimeError: test_class_basics::drop_unsendable_elsewhere::Unsendable is unsendbale, but is dropped on another thread!"); + assert!(object.is_none(py)); + + capture.borrow_mut(py).uninstall(py); + }); +} diff --git a/tests/test_exceptions.rs b/tests/test_exceptions.rs index cbbbd6368d5..c0cf57a9148 100644 --- a/tests/test_exceptions.rs +++ b/tests/test_exceptions.rs @@ -100,29 +100,11 @@ fn test_exception_nosegfault() { #[test] #[cfg(Py_3_8)] fn test_write_unraisable() { + use common::UnraisableCapture; use pyo3::{exceptions::PyRuntimeError, ffi, AsPyPointer}; - #[pyclass] - struct UnraisableCapture { - capture: Option<(PyErr, PyObject)>, - } - - #[pymethods] - impl UnraisableCapture { - fn hook(&mut self, unraisable: &PyAny) { - let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap()); - let instance = unraisable.getattr("object").unwrap(); - self.capture = Some((err, instance.into())); - } - } - Python::with_gil(|py| { - let sys = py.import("sys").unwrap(); - let old_hook = sys.getattr("unraisablehook").unwrap(); - let capture = Py::new(py, UnraisableCapture { capture: None }).unwrap(); - - sys.setattr("unraisablehook", capture.getattr(py, "hook").unwrap()) - .unwrap(); + let capture = UnraisableCapture::install(py); assert!(capture.borrow(py).capture.is_none()); @@ -140,6 +122,6 @@ fn test_write_unraisable() { assert_eq!(err.to_string(), "RuntimeError: bar"); assert!(object.as_ptr() == unsafe { ffi::Py_NotImplemented() }); - sys.setattr("unraisablehook", old_hook).unwrap(); + capture.borrow_mut(py).uninstall(py); }); }