Add pyo3::once_cell::GILOnceCell

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Added
 - Add FFI definition `PyObject_AsFileDescriptor` [#938](https://github.com/PyO3/pyo3/pull/938)
 - Add `PyByteArray::data`, `PyByteArray::as_bytes`, and `PyByteArray::as_bytes_mut`. [#967](https://github.com/PyO3/pyo3/pull/967)
+- Add `GILOnceCell` to workaround situations where `lazy_static` or `once_cell` can deadlock. [#975](https://github.com/PyO3/pyo3/pull/975)
 - Add `Py::borrow`, `Py::borrow_mut`, `Py::try_borrow`, and `Py::try_borrow_mut` for accessing `#[pyclass]` values. [#976](https://github.com/PyO3/pyo3/pull/976)
 
 ### Changed
@@ -19,11 +20,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Change signature of `PyTypeObject::type_object()` - now takes `Python` argument and returns `&PyType`. [#970](https://github.com/PyO3/pyo3/pull/970)
 - Change return type of `PyTuple::slice()` and `PyTuple::split_from()` from `Py<PyTuple>` to `&PyTuple`. [#970](https://github.com/PyO3/pyo3/pull/970)
 - Change return type of `PyTuple::as_slice` to `&[&PyAny]`. [#971](https://github.com/PyO3/pyo3/pull/971)
+- Rename `PyTypeInfo::type_object` to `type_object_raw`, and add `Python` argument. [#975](https://github.com/PyO3/pyo3/pull/975)
 - Update `num-complex` optional dependendency from `0.2` to `0.3`. [#977](https://github.com/PyO3/pyo3/pull/977)
 - Update `num-bigint` optional dependendency from `0.2` to `0.3`. [#978](https://github.com/PyO3/pyo3/pull/978)
 
 ### Removed
 - Remove `ManagedPyRef` (unused, and needs specialization) [#930](https://github.com/PyO3/pyo3/pull/930)
+- Disable `#[classattr]` where the class attribute is the same type as the class. (This may be re-enabled in the future; the previous implemenation was unsound.) [#975](https://github.com/PyO3/pyo3/pull/975)
 
 ### Fixed
 - Fix passing explicit `None` to `Option<T>` argument `#[pyfunction]` with a default value. [#936](https://github.com/PyO3/pyo3/pull/936)

guide/src/SUMMARY.md

@@ -12,6 +12,7 @@
 - [Advanced Topics](advanced.md)
 - [Building and Distribution](building_and_distribution.md)
 - [PyPy support](pypy.md)
+- [FAQ & Troubleshooting](faq.md)
 - [Appendix A: PyO3 and rust-cpython](rust_cpython.md)
 - [Appendix B: Migration Guide](migration.md)
 - [Appendix C: Trait bounds](trait_bounds.md)

guide/src/class.md

@@ -565,6 +565,11 @@ impl MyClass {
 }
 ```
 
+Note that defining a class attribute of the same type as the class will make the class unusable.
+Attempting to use the class will cause a panic reading `Recursive evaluation of type_object`.
+As an alternative, if having the attribute on instances is acceptable, create a `#[getter]` which
+uses a `GILOnceCell` to cache the attribute value. Or add the attribute to a module instead.
+
 ## Callable objects
 
 To specify a custom `__call__` method for a custom class, the method needs to be annotated with
@@ -922,10 +927,10 @@ unsafe impl pyo3::PyTypeInfo for MyClass {
     const FLAGS: usize = 0;
 
     #[inline]
-    fn type_object() -> &'static pyo3::ffi::PyTypeObject {
+    fn type_object_raw(py: pyo3::Python) -> &'static pyo3::ffi::PyTypeObject {
         use pyo3::type_object::LazyStaticType;
         static TYPE_OBJECT: LazyStaticType = LazyStaticType::new();
-        TYPE_OBJECT.get_or_init::<Self>()
+        TYPE_OBJECT.get_or_init::<Self>(py)
     }
 }
 

guide/src/faq.md

@@ -0,0 +1,16 @@
+# Frequently Asked Questions / Troubleshooting
+
+## I'm experiencing deadlocks using PyO3 with lazy_static or once_cell!
+
+`lazy_static` and `once_cell::sync` both use locks to ensure that initialization is performed only by a single thread. Because the Python GIL is an additional lock this can lead to deadlocks in the following way:
+
+1. A thread (thread A) which has acquired the Python GIL starts initialization of a `lazy_static` value.
+2. The initialization code calls some Python API which temporarily releases the GIL e.g. `Python::import`.
+3. Another thread (thread B) acquires the Python GIL and attempts to access the same `lazy_static` value.
+4. Thread B is blocked, because it waits for `lazy_static`'s initialization to lock to release.
+5. Thread A is blocked, because it waits to re-aquire the GIL which thread B still holds.
+6. Deadlock.
+
+PyO3 provides a struct [`GILOnceCell`] which works equivalently to `OnceCell` but relies solely on the Python GIL for thread safety. This means it can be used in place of `lazy_static` or `once_cell` where you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] for an example how to use it.
+
+[`GILOnceCell`]: https://docs.rs/pyo3/latest/pyo3/once_cell/struct.GILOnceCell.html

pyo3-derive-backend/src/pyclass.rs

@@ -387,10 +387,10 @@ fn impl_class(
             const FLAGS: usize = #(#flags)|* | #extended;
 
             #[inline]
-            fn type_object() -> &'static pyo3::ffi::PyTypeObject {
+            fn type_object_raw(py: pyo3::Python) -> &'static pyo3::ffi::PyTypeObject {
                 use pyo3::type_object::LazyStaticType;
                 static TYPE_OBJECT: LazyStaticType = LazyStaticType::new();
-                TYPE_OBJECT.get_or_init::<Self>()
+                TYPE_OBJECT.get_or_init::<Self>(py)
             }
         }
 

src/exceptions.rs

@@ -90,28 +90,26 @@ macro_rules! import_exception_type_object {
     ($module: expr, $name: ident) => {
         unsafe impl $crate::type_object::PyTypeObject for $name {
             fn type_object(py: $crate::Python) -> &$crate::types::PyType {
-                use $crate::type_object::LazyHeapType;
-                static TYPE_OBJECT: LazyHeapType = LazyHeapType::new();
-
-                let ptr = TYPE_OBJECT.get_or_init(|py| {
-                    let imp = py
-                        .import(stringify!($module))
-                        .expect(concat!("Can not import module: ", stringify!($module)));
-                    let cls = imp.get(stringify!($name)).expect(concat!(
-                        "Can not load exception class: {}.{}",
-                        stringify!($module),
-                        ".",
-                        stringify!($name)
-                    ));
-
-                    unsafe {
-                        std::ptr::NonNull::new_unchecked(
-                            $crate::IntoPyPointer::into_ptr(cls) as *mut _
-                        )
-                    }
-                });
-
-                unsafe { py.from_borrowed_ptr(ptr.as_ptr() as *mut $crate::ffi::PyObject) }
+                use $crate::once_cell::GILOnceCell;
+                use $crate::AsPyRef;
+                static TYPE_OBJECT: GILOnceCell<$crate::Py<$crate::types::PyType>> = GILOnceCell::new();
+
+                TYPE_OBJECT
+                    .get_or_init(py, || {
+                        let imp = py
+                            .import(stringify!($module))
+                            .expect(concat!("Can not import module: ", stringify!($module)));
+                        let cls = imp.get(stringify!($name)).expect(concat!(
+                            "Can not load exception class: {}.{}",
+                            stringify!($module),
+                            ".",
+                            stringify!($name)
+                        ));
+
+                        cls.extract()
+                            .expect("Imported exception should be a type object")
+                    })
+                    .as_ref(py)
             }
         }
     };
@@ -174,19 +172,24 @@ macro_rules! create_exception_type_object {
     ($module: ident, $name: ident, $base: ty) => {
         unsafe impl $crate::type_object::PyTypeObject for $name {
             fn type_object(py: $crate::Python) -> &$crate::types::PyType {
-                use $crate::type_object::LazyHeapType;
-                static TYPE_OBJECT: LazyHeapType = LazyHeapType::new();
-
-                let ptr = TYPE_OBJECT.get_or_init(|py| {
-                    $crate::PyErr::new_type(
-                        py,
-                        concat!(stringify!($module), ".", stringify!($name)),
-                        Some(py.get_type::<$base>()),
-                        None,
-                    )
-                });
-
-                unsafe { py.from_borrowed_ptr(ptr.as_ptr() as *mut $crate::ffi::PyObject) }
+                use $crate::once_cell::GILOnceCell;
+                use $crate::AsPyRef;
+                static TYPE_OBJECT: GILOnceCell<$crate::Py<$crate::types::PyType>> = GILOnceCell::new();
+
+                TYPE_OBJECT
+                    .get_or_init(py, || unsafe {
+                        $crate::Py::from_owned_ptr(
+                            py,
+                            $crate::PyErr::new_type(
+                                py,
+                                concat!(stringify!($module), ".", stringify!($name)),
+                                Some(py.get_type::<$base>()),
+                                None,
+                            )
+                            .as_ptr() as *mut $crate::ffi::PyObject,
+                        )
+                    })
+                    .as_ref(py)
             }
         }
     };

src/ffi/datetime.rs

@@ -14,10 +14,10 @@
 use crate::ffi::Py_hash_t;
 use crate::ffi::{PyObject, PyTypeObject};
 use crate::ffi::{PyObject_TypeCheck, Py_TYPE};
+use crate::once_cell::GILOnceCell;
+use crate::Python;
 use std::ops::Deref;
 use std::os::raw::{c_char, c_int, c_uchar};
-use std::ptr;
-use std::sync::Once;
 #[cfg(not(PyPy))]
 use {crate::ffi::PyCapsule_Import, std::ffi::CString};
 
@@ -196,29 +196,9 @@ pub struct PyDateTime_Delta {
     pub microseconds: c_int,
 }
 
-// C API Capsule
-// Note: This is "roll-your-own" lazy-static implementation is necessary because
-// of the interaction between the call_once locks and the GIL. It turns out that
-// calling PyCapsule_Import releases and re-acquires the GIL during the import,
-// so if you have two threads attempting to use the PyDateTimeAPI singleton
-// under the GIL, it causes a deadlock; what happens is:
-//
-// Thread 1 acquires GIL
-// Thread 1 acquires the call_once lock
-// Thread 1 calls PyCapsule_Import, thus releasing the GIL
-// Thread 2 acquires the GIL
-// Thread 2 blocks waiting for the call_once lock
-// Thread 1 blocks waiting for the GIL
-//
-// However, Python's import mechanism acquires a module-specific lock around
-// each import call, so all call importing datetime will return the same
-// object, making the call_once lock superfluous. As such, we can weaken
-// the guarantees of the cache, such that PyDateTime_IMPORT can be called
-// until __PY_DATETIME_API_UNSAFE_CACHE is populated, which will happen exactly
-// one time. So long as PyDateTime_IMPORT has no side effects (it should not),
-// this will be at most a slight waste of resources.
-static PY_DATETIME_API_ONCE: Once = Once::new();
-static mut PY_DATETIME_API_UNSAFE_CACHE: *const PyDateTime_CAPI = ptr::null();
+// Python already shares this object between threads, so it's no more evil for us to do it too!
+unsafe impl Sync for PyDateTime_CAPI {}
+static PY_DATETIME_API: GILOnceCell<&'static PyDateTime_CAPI> = GILOnceCell::new();
 
 #[derive(Debug)]
 pub struct PyDateTimeAPI {
@@ -233,13 +213,7 @@ impl Deref for PyDateTimeAPI {
     type Target = PyDateTime_CAPI;
 
     fn deref(&self) -> &'static PyDateTime_CAPI {
-        unsafe {
-            if !PY_DATETIME_API_UNSAFE_CACHE.is_null() {
-                &(*PY_DATETIME_API_UNSAFE_CACHE)
-            } else {
-                PyDateTime_IMPORT()
-            }
-        }
+        unsafe { PyDateTime_IMPORT() }
     }
 }
 
@@ -251,25 +225,27 @@ impl Deref for PyDateTimeAPI {
 /// Use this function only if you want to eagerly load the datetime module,
 /// such as if you do not want the first call to a datetime function to be
 /// slightly slower than subsequent calls.
+///
+/// # Safety
+/// The Python GIL must be held.
 pub unsafe fn PyDateTime_IMPORT() -> &'static PyDateTime_CAPI {
-    // PyPy expects the C-API to be initialized via PyDateTime_Import, so trying to use
-    // `PyCapsule_Import` will behave unexpectedly in pypy.
-    #[cfg(PyPy)]
-    let py_datetime_c_api = PyDateTime_Import();
-
-    #[cfg(not(PyPy))]
-    let py_datetime_c_api = {
-        // PyDateTime_CAPSULE_NAME is a macro in C
-        let PyDateTime_CAPSULE_NAME = CString::new("datetime.datetime_CAPI").unwrap();
-
-        PyCapsule_Import(PyDateTime_CAPSULE_NAME.as_ptr(), 1) as *const PyDateTime_CAPI
-    };
-
-    PY_DATETIME_API_ONCE.call_once(move || {
-        PY_DATETIME_API_UNSAFE_CACHE = py_datetime_c_api;
-    });
-
-    &(*PY_DATETIME_API_UNSAFE_CACHE)
+    let py = Python::assume_gil_acquired();
+    PY_DATETIME_API.get_or_init(py, || {
+        // PyPy expects the C-API to be initialized via PyDateTime_Import, so trying to use
+        // `PyCapsule_Import` will behave unexpectedly in pypy.
+        #[cfg(PyPy)]
+        let py_datetime_c_api = PyDateTime_Import();
+
+        #[cfg(not(PyPy))]
+        let py_datetime_c_api = {
+            // PyDateTime_CAPSULE_NAME is a macro in C
+            let PyDateTime_CAPSULE_NAME = CString::new("datetime.datetime_CAPI").unwrap();
+
+            &*(PyCapsule_Import(PyDateTime_CAPSULE_NAME.as_ptr(), 1) as *const PyDateTime_CAPI)
+        };
+
+        py_datetime_c_api
+    })
 }
 
 /// Type Check macros

src/freelist.rs

@@ -72,12 +72,12 @@ impl<T> PyClassAlloc for T
 where
     T: PyTypeInfo + PyClassWithFreeList,
 {
-    unsafe fn alloc(_py: Python) -> *mut Self::Layout {
+    unsafe fn alloc(py: Python) -> *mut Self::Layout {
         if let Some(obj) = <Self as PyClassWithFreeList>::get_free_list().pop() {
-            ffi::PyObject_Init(obj, <Self as PyTypeInfo>::type_object() as *const _ as _);
+            ffi::PyObject_Init(obj, Self::type_object_raw(py) as *const _ as _);
             obj as _
         } else {
-            crate::pyclass::default_alloc::<Self>() as _
+            crate::pyclass::default_alloc::<Self>(py) as _
         }
     }
 
@@ -90,7 +90,7 @@ where
         }
 
         if let Some(obj) = <Self as PyClassWithFreeList>::get_free_list().insert(obj) {
-            match Self::type_object().tp_free {
+            match Self::type_object_raw(py).tp_free {
                 Some(free) => free(obj as *mut c_void),
                 None => tp_free_fallback(obj),
             }

src/lib.rs

@@ -183,6 +183,7 @@ mod instance;
 mod internal_tricks;
 pub mod marshal;
 mod object;
+pub mod once_cell;
 pub mod panic;
 pub mod prelude;
 pub mod pycell;