Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions tests/test_buffer_protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ fn test_buffer_referenced() {
}

#[test]
#[cfg(all(Py_3_8, not(Py_GIL_DISABLED)))] // sys.unraisablehook not available until Python 3.8
#[cfg(Py_3_8)] // sys.unraisablehook not available until Python 3.8
fn test_releasebuffer_unraisable_error() {
use pyo3::exceptions::PyValueError;
use test_utils::UnraisableCapture;
Expand All @@ -120,20 +120,20 @@ fn test_releasebuffer_unraisable_error() {
}

Python::attach(|py| {
let capture = UnraisableCapture::install(py);

let instance = Py::new(py, ReleaseBufferError {}).unwrap();
let env = [("ob", instance.clone_ref(py))].into_py_dict(py).unwrap();

assert!(capture.borrow(py).capture.is_none());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious to me why this got deleted, any particular reason? There's a similar pattern in test_exceptions.rs below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it was just awkward to assert that the unraisable hook hadn't been called yet.

I've found a better pattern where the closure gets access to the hook so it can retrieve the value, will push that and restore this.

let (err, object) = UnraisableCapture::enter(py, |capture| {
let env = [("ob", instance.clone_ref(py))].into_py_dict(py).unwrap();

assert!(capture.take_capture().is_none());

py_assert!(py, *env, "bytes(ob) == b'hello world'");
py_assert!(py, *env, "bytes(ob) == b'hello world'");

capture.take_capture().unwrap()
});

let (err, object) = capture.borrow_mut(py).capture.take().unwrap();
assert_eq!(err.to_string(), "ValueError: oh dear");
assert!(object.is(&instance));

capture.borrow_mut(py).uninstall(py);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even without the locking the new API is nicer without this explicit uninstall step.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, much less error prone.

});
}

Expand Down
16 changes: 9 additions & 7 deletions tests/test_class_attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ fn recursive_class_attributes() {
}

#[test]
#[cfg(all(Py_3_8, not(Py_GIL_DISABLED)))] // sys.unraisablehook not available until Python 3.8
#[cfg(Py_3_8)] // sys.unraisablehook not available until Python 3.8
fn test_fallible_class_attribute() {
use pyo3::exceptions::PyValueError;
use test_utils::UnraisableCapture;
Expand All @@ -168,29 +168,31 @@ fn test_fallible_class_attribute() {
}

Python::attach(|py| {
let capture = UnraisableCapture::install(py);
assert!(std::panic::catch_unwind(|| py.get_type::<BrokenClass>()).is_err());
let (err, object) = UnraisableCapture::enter(py, |capture| {
// Accessing the type will attempt to initialize the class attributes
assert!(std::panic::catch_unwind(|| py.get_type::<BrokenClass>()).is_err());

let (err, object) = capture.borrow_mut(py).capture.take().unwrap();
assert!(object.is_none(py));
capture.take_capture().unwrap()
});

assert!(object.is_none());
assert_eq!(
err.to_string(),
"RuntimeError: An error occurred while initializing class BrokenClass"
);

let cause = err.cause(py).unwrap();
assert_eq!(
cause.to_string(),
"RuntimeError: An error occurred while initializing `BrokenClass.fails_to_init`"
);

let cause = cause.cause(py).unwrap();
assert_eq!(
cause.to_string(),
"ValueError: failed to create class attribute"
);
assert!(cause.cause(py).is_none());

capture.borrow_mut(py).uninstall(py);
});
}

Expand Down
57 changes: 32 additions & 25 deletions tests/test_class_basics.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#![cfg(feature = "macros")]

#[cfg(Py_3_8)]
use pyo3::ffi::c_str;
use pyo3::prelude::*;
use pyo3::types::PyType;
use pyo3::{py_run, PyClass};
Expand Down Expand Up @@ -615,7 +617,7 @@ fn access_frozen_class_without_gil() {
}

#[test]
#[cfg(all(Py_3_8, not(Py_GIL_DISABLED)))] // sys.unraisablehook not available until Python 3.8
#[cfg(Py_3_8)]
#[cfg_attr(target_arch = "wasm32", ignore)]
fn drop_unsendable_elsewhere() {
use std::sync::{
Expand All @@ -637,35 +639,40 @@ fn drop_unsendable_elsewhere() {
}

Python::attach(|py| {
let capture = UnraisableCapture::install(py);
let (err, object) = UnraisableCapture::enter(py, |capture| {
let dropped = Arc::new(AtomicBool::new(false));

let dropped = Arc::new(AtomicBool::new(false));

let unsendable = Py::new(
py,
Unsendable {
dropped: dropped.clone(),
},
)
.unwrap();

py.detach(|| {
spawn(move || {
Python::attach(move |_py| {
drop(unsendable);
});
})
.join()
let unsendable = Py::new(
py,
Unsendable {
dropped: dropped.clone(),
},
)
.unwrap();
});

assert!(!dropped.load(Ordering::SeqCst));
py.detach(|| {
spawn(move || {
Python::attach(move |py| {
drop(unsendable);
// On the free-threaded build, dropping an object on its non-origin thread
// will not immediately drop it because the refcounts need to be merged.
//
// Force GC to ensure the drop happens now on the wrong thread.
py.run(c_str!("import gc; gc.collect()"), None, None)
.unwrap();
Comment on lines +657 to +662
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight this should have been obvious why this test was failing; for a while I was worried it was a refcounting bug on PyO3's end.

});
})
.join()
.unwrap();
});

assert!(!dropped.load(Ordering::SeqCst));

capture.take_capture().unwrap()
});

let (err, object) = capture.borrow_mut(py).capture.take().unwrap();
assert_eq!(err.to_string(), "RuntimeError: test_class_basics::drop_unsendable_elsewhere::Unsendable is unsendable, but is being dropped on another thread");
assert!(object.is_none(py));

capture.borrow_mut(py).uninstall(py);
assert!(object.is_none());
});
}

Expand Down
31 changes: 14 additions & 17 deletions tests/test_exceptions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,31 +98,28 @@ fn test_exception_nosegfault() {
}

#[test]
#[cfg(all(Py_3_8, not(Py_GIL_DISABLED)))]
#[cfg(Py_3_8)]
fn test_write_unraisable() {
use pyo3::{exceptions::PyRuntimeError, ffi, types::PyNotImplemented};
use std::ptr;
use pyo3::{exceptions::PyRuntimeError, types::PyNotImplemented};
use test_utils::UnraisableCapture;

Python::attach(|py| {
let capture = UnraisableCapture::install(py);
UnraisableCapture::enter(py, |capture| {
let err = PyRuntimeError::new_err("foo");
err.write_unraisable(py, None);

assert!(capture.borrow(py).capture.is_none());
let (err, object) = capture.take_capture().unwrap();

let err = PyRuntimeError::new_err("foo");
err.write_unraisable(py, None);
assert_eq!(err.to_string(), "RuntimeError: foo");
assert!(object.is_none());

let (err, object) = capture.borrow_mut(py).capture.take().unwrap();
assert_eq!(err.to_string(), "RuntimeError: foo");
assert!(object.is_none(py));
let err = PyRuntimeError::new_err("bar");
err.write_unraisable(py, Some(&PyNotImplemented::get(py)));

let err = PyRuntimeError::new_err("bar");
err.write_unraisable(py, Some(&PyNotImplemented::get(py)));
let (err, object) = capture.take_capture().unwrap();

let (err, object) = capture.borrow_mut(py).capture.take().unwrap();
assert_eq!(err.to_string(), "RuntimeError: bar");
assert!(unsafe { ptr::eq(object.as_ptr(), ffi::Py_NotImplemented()) });

capture.borrow_mut(py).uninstall(py);
assert_eq!(err.to_string(), "RuntimeError: bar");
assert!(object.is(PyNotImplemented::get(py)));
});
});
}
93 changes: 68 additions & 25 deletions tests/test_utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ mod inner {

use pyo3::prelude::*;

#[cfg(any(not(all(Py_GIL_DISABLED, Py_3_14)), all(feature = "macros", Py_3_8)))]
use pyo3::sync::MutexExt;
use pyo3::types::{IntoPyDict, PyList};

#[cfg(any(not(all(Py_GIL_DISABLED, Py_3_14)), all(feature = "macros", Py_3_8)))]
use std::sync::{Mutex, PoisonError};

use uuid::Uuid;
Expand Down Expand Up @@ -118,49 +120,88 @@ mod inner {
}

// sys.unraisablehook not available until Python 3.8
#[cfg(all(feature = "macros", Py_3_8, not(Py_GIL_DISABLED)))]
#[pyclass(crate = "pyo3")]
pub struct UnraisableCapture {
pub capture: Option<(PyErr, Py<PyAny>)>,
old_hook: Option<Py<PyAny>>,
#[cfg(all(feature = "macros", Py_3_8))]
pub struct UnraisableCapture<'py> {
hook: Bound<'py, UnraisableCaptureHook>,
}

#[cfg(all(feature = "macros", Py_3_8, not(Py_GIL_DISABLED)))]
#[cfg(all(feature = "macros", Py_3_8))]
impl<'py> UnraisableCapture<'py> {
/// Runs the closure `f` with a custom sys.unraisablehook installed.
///
/// `f`
pub fn enter<R>(py: Python<'py>, f: impl FnOnce(&Self) -> R) -> R {
// unraisablehook is a global, so only one thread can be using this struct at a time.
static UNRAISABLE_HOOK_MUTEX: Mutex<()> = Mutex::new(());

// NB this is best-effort, other tests could always modify sys.unraisablehook directly.
let mutex_guard = UNRAISABLE_HOOK_MUTEX
.lock_py_attached(py)
.unwrap_or_else(PoisonError::into_inner);

let guard = Self {
hook: UnraisableCaptureHook::install(py),
};

let result = f(&guard);

drop(guard);
drop(mutex_guard);

result
}

/// Takes the captured unraisable error, if any.
pub fn take_capture(&self) -> Option<(PyErr, Bound<'py, PyAny>)> {
let mut guard = self.hook.get().capture.lock().unwrap();
guard.take().map(|(e, o)| (e, o.into_bound(self.hook.py())))
}
}

#[cfg(all(feature = "macros", Py_3_8))]
impl Drop for UnraisableCapture<'_> {
fn drop(&mut self) {
let py = self.hook.py();
self.hook.get().uninstall(py);
}
}

#[cfg(all(feature = "macros", Py_3_8))]
#[pyclass(crate = "pyo3", frozen)]
struct UnraisableCaptureHook {
pub capture: Mutex<Option<(PyErr, Py<PyAny>)>>,
old_hook: Py<PyAny>,
}

#[cfg(all(feature = "macros", Py_3_8))]
#[pymethods(crate = "pyo3")]
impl UnraisableCapture {
pub fn hook(&mut self, unraisable: Bound<'_, PyAny>) {
impl UnraisableCaptureHook {
pub fn hook(&self, unraisable: Bound<'_, PyAny>) {
let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap());
let instance = unraisable.getattr("object").unwrap();
self.capture = Some((err, instance.into()));
self.capture.lock().unwrap().replace((err, instance.into()));
}
}

#[cfg(all(feature = "macros", Py_3_8, not(Py_GIL_DISABLED)))]
impl UnraisableCapture {
pub fn install(py: Python<'_>) -> Py<Self> {
#[cfg(all(feature = "macros", Py_3_8))]
impl UnraisableCaptureHook {
fn install(py: Python<'_>) -> Bound<'_, Self> {
let sys = py.import("sys").unwrap();

let old_hook = sys.getattr("unraisablehook").unwrap().into();
let capture = Mutex::new(None);

let capture = Py::new(
py,
UnraisableCapture {
capture: None,
old_hook: Some(old_hook),
},
)
.unwrap();
let capture = Bound::new(py, UnraisableCaptureHook { capture, old_hook }).unwrap();

sys.setattr("unraisablehook", capture.getattr(py, "hook").unwrap())
sys.setattr("unraisablehook", capture.getattr("hook").unwrap())
.unwrap();

capture
}

pub fn uninstall(&mut self, py: Python<'_>) {
let old_hook = self.old_hook.take().unwrap();

fn uninstall(&self, py: Python<'_>) {
let sys = py.import("sys").unwrap();
sys.setattr("unraisablehook", old_hook).unwrap();
sys.setattr("unraisablehook", &self.old_hook).unwrap();
}
}

Expand All @@ -170,6 +211,7 @@ mod inner {

/// catch_warnings is not thread-safe, so only one thread can be using this struct at
/// a time.
#[cfg(not(all(Py_GIL_DISABLED, Py_3_14)))] // Python 3.14t has thread-safe catch_warnings
static CATCH_WARNINGS_MUTEX: Mutex<()> = Mutex::new(());

impl<'py> CatchWarnings<'py> {
Expand All @@ -178,6 +220,7 @@ mod inner {
f: impl FnOnce(&Bound<'py, PyList>) -> PyResult<R>,
) -> PyResult<R> {
// NB this is best-effort, other tests could always call the warnings API directly.
#[cfg(not(all(Py_GIL_DISABLED, Py_3_14)))]
let _mutex_guard = CATCH_WARNINGS_MUTEX
.lock_py_attached(py)
.unwrap_or_else(PoisonError::into_inner);
Expand Down
Loading