Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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(all(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 ((), Some((err, object))) = UnraisableCapture::enter(py, || {
let env = [("ob", instance.clone_ref(py))].into_py_dict(py).unwrap();

py_assert!(py, *env, "bytes(ob) == b'hello world'");
py_assert!(py, *env, "bytes(ob) == b'hello world'");
Ok(())
})
.unwrap() else {
panic!("no unraisable error captured");
};

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
22 changes: 13 additions & 9 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(all(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,33 @@ 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) = capture.borrow_mut(py).capture.take().unwrap();
assert!(object.is_none(py));

let ((), Some((err, object))) = UnraisableCapture::enter(py, || {
// Accessing the type will attempt to initialize the class attributes
assert!(std::panic::catch_unwind(|| py.get_type::<BrokenClass>()).is_err());
Ok(())
})
.unwrap() else {
panic!("no unraisable error captured");
};

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
52 changes: 27 additions & 25 deletions tests/test_class_basics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,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(all(Py_3_8))]
#[cfg_attr(target_arch = "wasm32", ignore)]
fn drop_unsendable_elsewhere() {
use std::sync::{
Expand All @@ -637,35 +637,37 @@ fn drop_unsendable_elsewhere() {
}

Python::attach(|py| {
let capture = UnraisableCapture::install(py);
let ((), Some((err, object))) = UnraisableCapture::enter(py, || {
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);
});
})
.join()
.unwrap();
});

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

Ok(())
})
.unwrap() else {
panic!("no unraisable error captured");
};

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
37 changes: 20 additions & 17 deletions tests/test_exceptions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,31 +98,34 @@ fn test_exception_nosegfault() {
}

#[test]
#[cfg(all(Py_3_8, not(Py_GIL_DISABLED)))]
#[cfg(all(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);
let ((), Some((err, object))) = UnraisableCapture::enter(py, || {
let err = PyRuntimeError::new_err("foo");
err.write_unraisable(py, None);
Ok(())
})
.unwrap() else {
panic!("no unraisable error captured");
};

assert!(capture.borrow(py).capture.is_none());

let err = PyRuntimeError::new_err("foo");
err.write_unraisable(py, None);

let (err, object) = capture.borrow_mut(py).capture.take().unwrap();
assert_eq!(err.to_string(), "RuntimeError: foo");
assert!(object.is_none(py));
assert!(object.is_none());

let err = PyRuntimeError::new_err("bar");
err.write_unraisable(py, Some(&PyNotImplemented::get(py)));
let ((), Some((err, object))) = UnraisableCapture::enter(py, || {
let err = PyRuntimeError::new_err("bar");
err.write_unraisable(py, Some(&PyNotImplemented::get(py)));
Ok(())
})
.unwrap() else {
panic!("no unraisable error captured");
};

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!(object.is(PyNotImplemented::get(py)));
});
}
58 changes: 51 additions & 7 deletions tests/test_utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,33 +117,75 @@ mod inner {
};
}

/// unraisablehook is a global, so only one thread can be using this struct at a time.
static UNRAISABLE_HOOK_MUTEX: Mutex<()> = Mutex::new(());

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

#[cfg(all(feature = "macros", Py_3_8))]
impl<'py> UnraisableCapture<'py> {
pub fn enter<R>(
py: Python<'py>,
f: impl FnOnce() -> PyResult<R>,
) -> PyResult<(R, Option<(PyErr, Bound<'py, PyAny>)>)> {
// 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 hook = UnraisableCaptureHook::install(py).into_bound(py);
let guard = Self { hook: hook.clone() };
let result = f()?;
drop(guard);

let capture = hook
.borrow_mut()
.capture
.take()
.map(|(e, o)| (e, o.into_bound(py)));

Ok((result, capture))
}
}

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

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

#[cfg(all(feature = "macros", Py_3_8, not(Py_GIL_DISABLED)))]
#[cfg(all(feature = "macros", Py_3_8))]
#[pymethods(crate = "pyo3")]
impl UnraisableCapture {
impl UnraisableCaptureHook {
pub fn hook(&mut 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()));
}
}

#[cfg(all(feature = "macros", Py_3_8, not(Py_GIL_DISABLED)))]
impl UnraisableCapture {
#[cfg(all(feature = "macros", Py_3_8))]
impl UnraisableCaptureHook {
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 {
UnraisableCaptureHook {
capture: None,
old_hook: Some(old_hook),
},
Expand All @@ -170,6 +212,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 +221,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