From c09947a61c5c5dc947d37126753efa6f3db8bdbe Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Tue, 13 Jul 2021 21:26:10 +0100 Subject: [PATCH] err: don't use with_gil internally in PyErr::new() --- CHANGELOG.md | 4 ++++ src/err/err_state.rs | 34 ++++++++++++++++++++++++--- src/err/mod.rs | 55 ++++++++++++++++++++++++++++++-------------- 3 files changed, 73 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 725ea24b..080fd1b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Add `indexmap` feature to add `ToPyObject`, `IntoPy` and `FromPyObject` implementations for `indexmap::IndexMap`. [#1728](https://github.com/PyO3/pyo3/pull/1728) +### Changed + +- `PyErr::new` no longer acquires the Python GIL internally. [#1724](https://github.com/PyO3/pyo3/pull/1724) + ### Fixed - Fix regression in 0.14.0 rejecting usage of `#[doc(hidden)]` on structs and functions annotated with PyO3 macros. [#1722](https://github.com/PyO3/pyo3/pull/1722) diff --git a/src/err/err_state.rs b/src/err/err_state.rs index a6ae1260..6947446d 100644 --- a/src/err/err_state.rs +++ b/src/err/err_state.rs @@ -1,5 +1,9 @@ use crate::{ - exceptions::PyBaseException, ffi, types::PyType, IntoPy, IntoPyPointer, Py, PyObject, Python, + exceptions::{PyBaseException, PyTypeError}, + ffi, + type_object::PyTypeObject, + types::PyType, + AsPyPointer, IntoPy, IntoPyPointer, Py, PyObject, Python, }; #[derive(Clone)] @@ -10,7 +14,11 @@ pub(crate) struct PyErrStateNormalized { } pub(crate) enum PyErrState { - Lazy { + LazyTypeAndValue { + ptype: fn(Python) -> &PyType, + pvalue: Box PyObject + Send + Sync>, + }, + LazyValue { ptype: Py, pvalue: Box PyObject + Send + Sync>, }, @@ -49,7 +57,19 @@ impl PyErrState { py: Python, ) -> (*mut ffi::PyObject, *mut ffi::PyObject, *mut ffi::PyObject) { match self { - PyErrState::Lazy { ptype, pvalue } => ( + PyErrState::LazyTypeAndValue { ptype, pvalue } => { + let ty = ptype(py); + if unsafe { ffi::PyExceptionClass_Check(ty.as_ptr()) } == 0 { + Self::exceptions_must_derive_from_base_exception(py).into_ffi_tuple(py) + } else { + ( + ptype(py).into_ptr(), + pvalue(py).into_ptr(), + std::ptr::null_mut(), + ) + } + } + PyErrState::LazyValue { ptype, pvalue } => ( ptype.into_ptr(), pvalue(py).into_ptr(), std::ptr::null_mut(), @@ -66,4 +86,12 @@ impl PyErrState { }) => (ptype.into_ptr(), pvalue.into_ptr(), ptraceback.into_ptr()), } } + + #[inline] + pub(crate) fn exceptions_must_derive_from_base_exception(py: Python) -> Self { + PyErrState::LazyValue { + ptype: PyTypeError::type_object(py).into(), + pvalue: boxed_args("exceptions must derive from BaseException"), + } + } } diff --git a/src/err/mod.rs b/src/err/mod.rs index 1634ee2f..fe5fd1c7 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -81,7 +81,10 @@ impl PyErr { T: PyTypeObject, A: PyErrArguments + Send + Sync + 'static, { - Python::with_gil(|py| PyErr::from_type(T::type_object(py), args)) + PyErr::from_state(PyErrState::LazyTypeAndValue { + ptype: T::type_object, + pvalue: boxed_args(args), + }) } /// Constructs a new error, with the usual lazy initialization of Python exceptions. @@ -97,7 +100,7 @@ impl PyErr { return exceptions_must_derive_from_base_exception(ty.py()); } - PyErr::from_state(PyErrState::Lazy { + PyErr::from_state(PyErrState::LazyValue { ptype: ty.into(), pvalue: boxed_args(args), }) @@ -319,7 +322,7 @@ impl PyErr { T: ToBorrowedObject, { exc.with_borrowed_ptr(py, |exc| unsafe { - ffi::PyErr_GivenExceptionMatches(self.ptype_ptr(), exc) != 0 + ffi::PyErr_GivenExceptionMatches(self.ptype_ptr(py), exc) != 0 }) } @@ -329,7 +332,7 @@ impl PyErr { T: PyTypeObject, { unsafe { - ffi::PyErr_GivenExceptionMatches(self.ptype_ptr(), T::type_object(py).as_ptr()) != 0 + ffi::PyErr_GivenExceptionMatches(self.ptype_ptr(py), T::type_object(py).as_ptr()) != 0 } } @@ -419,9 +422,12 @@ impl PyErr { } /// Returns borrowed reference to this Err's type - fn ptype_ptr(&self) -> *mut ffi::PyObject { + fn ptype_ptr(&self, py: Python) -> *mut ffi::PyObject { match unsafe { &*self.state.get() } { - Some(PyErrState::Lazy { ptype, .. }) => ptype.as_ptr(), + // In lazy type case, normalize before returning ptype in case the type is not a valid + // exception type. + Some(PyErrState::LazyTypeAndValue { .. }) => self.normalized(py).ptype.as_ptr(), + Some(PyErrState::LazyValue { ptype, .. }) => ptype.as_ptr(), Some(PyErrState::FfiTuple { ptype, .. }) => ptype.as_ptr(), Some(PyErrState::Normalized(n)) => n.ptype.as_ptr(), None => panic!("Cannot access exception type while normalizing"), @@ -554,10 +560,7 @@ pub fn error_on_minusone(py: Python, result: c_int) -> PyResult<()> { #[inline] fn exceptions_must_derive_from_base_exception(py: Python) -> PyErr { - PyErr::from_state(PyErrState::Lazy { - ptype: exceptions::PyTypeError::type_object(py).into(), - pvalue: boxed_args("exceptions must derive from BaseException"), - }) + PyErr::from_state(PyErrState::exceptions_must_derive_from_base_exception(py)) } #[cfg(test)] @@ -567,13 +570,31 @@ mod tests { use crate::{PyErr, Python}; #[test] - fn set_typeerror() { - let gil = Python::acquire_gil(); - let py = gil.python(); - let err: PyErr = exceptions::PyTypeError::new_err(()); - err.restore(py); - assert!(PyErr::occurred(py)); - drop(PyErr::fetch(py)); + fn set_valueerror() { + Python::with_gil(|py| { + let err: PyErr = exceptions::PyValueError::new_err("some exception message"); + assert!(err.is_instance::(py)); + err.restore(py); + assert!(PyErr::occurred(py)); + let err = PyErr::fetch(py); + assert!(err.is_instance::(py)); + assert_eq!(err.to_string(), "ValueError: some exception message"); + }) + } + + #[test] + fn invalid_error_type() { + Python::with_gil(|py| { + let err: PyErr = PyErr::new::(()); + assert!(err.is_instance::(py)); + err.restore(py); + let err = PyErr::fetch(py); + assert!(err.is_instance::(py)); + assert_eq!( + err.to_string(), + "TypeError: exceptions must derive from BaseException" + ); + }) } #[test]