Prevent dropping unsendable classes on other threads.

newsfragments/3176.fixed.md

@@ -0,0 +1 @@
+Avoid running the `Drop` implementations of unsendable classes on other threads

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<T> PyClassNewTextSignature<T> for &'_ PyClassImplCollector<T> {
 #[doc(hidden)]
 pub trait PyClassThreadChecker<T>: Sized {
     fn ensure(&self);
+    fn can_drop(&self, py: Python<'_>) -> bool;
     fn new() -> Self;
     private_decl! {}
 }
@@ -894,6 +895,9 @@ pub struct ThreadCheckerStub<T: Send>(PhantomData<T>);
 
 impl<T: Send> PyClassThreadChecker<T> for ThreadCheckerStub<T> {
     fn ensure(&self) {}
+    fn can_drop(&self, _py: Python<'_>) -> bool {
+        true
+    }
     #[inline]
     fn new() -> Self {
         ThreadCheckerStub(PhantomData)
@@ -903,6 +907,9 @@ impl<T: Send> PyClassThreadChecker<T> for ThreadCheckerStub<T> {
 
 impl<T: PyNativeType> PyClassThreadChecker<T> for ThreadCheckerStub<crate::PyObject> {
     fn ensure(&self) {}
+    fn can_drop(&self, _py: Python<'_>) -> bool {
+        true
+    }
     #[inline]
     fn new() -> Self {
         ThreadCheckerStub(PhantomData)
@@ -924,6 +931,18 @@ impl<T> PyClassThreadChecker<T> for ThreadCheckerImpl<T> {
             std::any::type_name::<T>()
         );
     }
+    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::<T>()
+            ))
+            .write_unraisable(py, None);
+            return false;
+        }
+
+        true
+    }
     fn new() -> Self {
         ThreadCheckerImpl(thread::current().id(), PhantomData)
     }
@@ -944,6 +963,9 @@ impl<T: PyClass + Send, U: PyClassBaseType> PyClassThreadChecker<T>
     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())
     }

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<T>);
-        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);
         <T::BaseType as PyClassBaseType>::LayoutAsBase::tp_dealloc(py, slf)

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<PyObject>,
+}
+
+#[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<Self> {
+        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();
+    }
+}

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);
     });
 }
 

tests/test_class_basics.rs

@@ -228,31 +228,40 @@ impl UnsendableChild {
 }
 
 fn test_unsendable<T: PyClass + 'static>() -> PyResult<()> {
-    let obj = std::thread::spawn(|| -> PyResult<_> {
+    let obj = Python::with_gil(|py| -> PyResult<_> {
+        let obj: Py<T> = PyType::new::<T>(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::<usize>()?, 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<T> = PyType::new::<T>(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::<usize>()?, 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::<String>() {
+            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<AtomicBool>,
+    }
+
+    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);
+    });
+}

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);
     });
 }