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/test_class_basics.rs b/tests/test_class_basics.rs index ac3c98d0844..deff52dc98b 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -526,3 +526,41 @@ fn access_frozen_class_without_gil() { assert_eq!(py_counter.get().value.load(Ordering::Relaxed), 1); } + +#[test] +fn drop_unsendable_elsewhere() { + 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); + } + } + + let dropped = Arc::new(AtomicBool::new(false)); + + let unsendable = Python::with_gil(|py| { + let dropped = dropped.clone(); + + Py::new(py, Unsendable { dropped }).unwrap() + }); + + spawn(move || { + Python::with_gil(move |_py| { + drop(unsendable); + }); + }) + .join() + .unwrap(); + + assert!(!dropped.load(Ordering::SeqCst)); +}