Merge pull request #1724 from davidhewitt/err-new-no-gil

err: don't use with_gil internally in PyErr::new()

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)

Cargo.toml

@@ -77,6 +77,10 @@ nightly = []
 name = "bench_call"
 harness = false
 
+[[bench]]
+name = "bench_err"
+harness = false
+
 [[bench]]
 name = "bench_dict"
 harness = false

benches/bench_err.rs

@@ -0,0 +1,24 @@
+use criterion::{criterion_group, criterion_main, Bencher, Criterion};
+
+use pyo3::{exceptions::PyValueError, prelude::*};
+
+fn err_new_restore_and_fetch(b: &mut Bencher) {
+    Python::with_gil(|py| {
+        b.iter(|| {
+            PyValueError::new_err("some exception message").restore(py);
+            PyErr::fetch(py)
+        })
+    })
+}
+
+fn err_new_without_gil(b: &mut Bencher) {
+    b.iter(|| PyValueError::new_err("some exception message"))
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+    c.bench_function("err_new_restore_and_fetch", err_new_restore_and_fetch);
+    c.bench_function("err_new_without_gil", err_new_without_gil);
+}
+
+criterion_group!(benches, criterion_benchmark);
+criterion_main!(benches);

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<dyn FnOnce(Python) -> PyObject + Send + Sync>,
+    },
+    LazyValue {
         ptype: Py<PyType>,
         pvalue: Box<dyn FnOnce(Python) -> 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"),
+        }
+    }
 }

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::<exceptions::PyValueError>(py));
+            err.restore(py);
+            assert!(PyErr::occurred(py));
+            let err = PyErr::fetch(py);
+            assert!(err.is_instance::<exceptions::PyValueError>(py));
+            assert_eq!(err.to_string(), "ValueError: some exception message");
+        })
+    }
+
+    #[test]
+    fn invalid_error_type() {
+        Python::with_gil(|py| {
+            let err: PyErr = PyErr::new::<crate::types::PyString, _>(());
+            assert!(err.is_instance::<exceptions::PyTypeError>(py));
+            err.restore(py);
+            let err = PyErr::fetch(py);
+            assert!(err.is_instance::<exceptions::PyTypeError>(py));
+            assert_eq!(
+                err.to_string(),
+                "TypeError: exceptions must derive from BaseException"
+            );
+        })
     }
 
     #[test]