From adb8fbd083c0ec3de8e95a1fc7e21102b3d5dbc8 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sun, 14 Jun 2020 16:29:40 +0100 Subject: [PATCH] Add pyo3::once_cell::GILOnceCell --- CHANGELOG.md | 3 + guide/src/SUMMARY.md | 1 + guide/src/class.md | 9 ++- guide/src/faq.md | 16 ++++ pyo3-derive-backend/src/pyclass.rs | 4 +- src/exceptions.rs | 73 +++++++++--------- src/ffi/datetime.rs | 76 +++++++------------ src/freelist.rs | 8 +- src/lib.rs | 1 + src/once_cell.rs | 92 ++++++++++++++++++++++ src/pyclass.rs | 14 ++-- src/type_object.rs | 118 +++++++++++++---------------- src/types/mod.rs | 2 +- tests/test_class_attributes.rs | 33 ++++---- 14 files changed, 266 insertions(+), 184 deletions(-) create mode 100644 guide/src/faq.md create mode 100644 src/once_cell.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index e90482636e9..36258edbc2b 100644 --- a/CHANGELOG.md +++ b/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 use in 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` 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` argument `#[pyfunction]` with a default value. [#936](https://github.com/PyO3/pyo3/pull/936) diff --git a/guide/src/SUMMARY.md b/guide/src/SUMMARY.md index e8bc665dbec..f4bdea66c8e 100644 --- a/guide/src/SUMMARY.md +++ b/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) diff --git a/guide/src/class.md b/guide/src/class.md index 2a05ded9076..a98f6842a72 100644 --- a/guide/src/class.md +++ b/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::() + TYPE_OBJECT.get_or_init::(py) } } diff --git a/guide/src/faq.md b/guide/src/faq.md new file mode 100644 index 00000000000..6aaa8a32688 --- /dev/null +++ b/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 diff --git a/pyo3-derive-backend/src/pyclass.rs b/pyo3-derive-backend/src/pyclass.rs index 33a45ba61c7..19930c51517 100644 --- a/pyo3-derive-backend/src/pyclass.rs +++ b/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::() + TYPE_OBJECT.get_or_init::(py) } } diff --git a/src/exceptions.rs b/src/exceptions.rs index 4237287a194..f2c146aa608 100644 --- a/src/exceptions.rs +++ b/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) } } }; diff --git a/src/ffi/datetime.rs b/src/ffi/datetime.rs index 827dfde6240..e26dc0345f5 100644 --- a/src/ffi/datetime.rs +++ b/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 diff --git a/src/freelist.rs b/src/freelist.rs index d3a4ae4e413..f7a6e4c6ed1 100644 --- a/src/freelist.rs +++ b/src/freelist.rs @@ -72,12 +72,12 @@ impl 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) = ::get_free_list().pop() { - ffi::PyObject_Init(obj, ::type_object() as *const _ as _); + ffi::PyObject_Init(obj, Self::type_object_raw(py) as *const _ as _); obj as _ } else { - crate::pyclass::default_alloc::() as _ + crate::pyclass::default_alloc::(py) as _ } } @@ -90,7 +90,7 @@ where } if let Some(obj) = ::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), } diff --git a/src/lib.rs b/src/lib.rs index 0051461347b..f1892546888 100755 --- a/src/lib.rs +++ b/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; diff --git a/src/once_cell.rs b/src/once_cell.rs new file mode 100644 index 00000000000..ec75b144ac1 --- /dev/null +++ b/src/once_cell.rs @@ -0,0 +1,92 @@ +use crate::Python; +use std::cell::UnsafeCell; + +/// A write-once cell similar to `once_cell::OnceCell`. +/// +/// Unlike `once_cell::sync` which uses locks to achieve thread safety, this implementation uses +/// the Python GIL to mediate concurrent access. +/// +/// # Example +/// +/// The following example shows how to use `GILOnceCell` to share a reference to a Python list +/// between threads: +/// +/// ``` +/// use pyo3::prelude::*; +/// use pyo3::types::PyList; +/// use pyo3::once_cell::GILOnceCell; +/// +/// static LIST_CELL: GILOnceCell> = GILOnceCell::new(); +/// +/// pub fn get_shared_list(py: Python) -> &PyList { +/// LIST_CELL +/// .get_or_init(py, || PyList::empty(py).into()) +/// .as_ref(py) +/// } +/// # let gil = Python::acquire_gil(); +/// # let py = gil.python(); +/// # assert_eq!(get_shared_list(py).len(), 0 ); +/// ``` +pub struct GILOnceCell(UnsafeCell>); + +// T: Send is needed for Sync because the thread which drops the GILOnceCell can be different +// to the thread which fills it. +unsafe impl Sync for GILOnceCell {} +unsafe impl Send for GILOnceCell {} + +impl GILOnceCell { + /// Create a `GILOnceCell` which does not yet contain a value. + pub const fn new() -> Self { + Self(UnsafeCell::new(None)) + } + + /// Get a reference to the contained value, or `None` if the cell has not yet been written. + pub fn get(&self, _py: Python) -> Option<&T> { + // Safe because if the cell has not yet been written, None is returned. + unsafe { &*self.0.get() }.as_ref() + } + + /// Get a reference to the contained value, initializing it if needed using the provided + /// closure. + /// + /// Note that reentrant initialization can cause a stack overflow. + pub fn get_or_init(&self, py: Python, f: F) -> &T + where + F: FnOnce() -> T, + { + let inner = unsafe { &*self.0.get() }.as_ref(); + if let Some(value) = inner { + return value; + } + + // Note that f() could temporarily release the GIL, so it's possible that another thread + // writes to this GILOnceCell before f() finishes. That's fine; we'll just have to discard + // the value computed here and accept a bit of wasted computation. + let value = f(); + let _ = self.set(py, value); + + self.get(py).unwrap() + } + + /// Get the contents of the cell mutably. This is only possible if the reference to the cell is + /// unique. + pub fn get_mut(&mut self) -> Option<&mut T> { + // Safe because we have &mut self + unsafe { &mut *self.0.get() }.as_mut() + } + + /// Set the value in the cell. + /// + /// If the cell has already been written, `Err(value)` will be returned containing the new + /// value which was not written. + pub fn set(&self, _py: Python, value: T) -> Result<(), T> { + // Safe because GIL is held, so no other thread can be writing to this cell concurrently. + let inner = unsafe { &mut *self.0.get() }; + if inner.is_some() { + return Err(value); + } + + *inner = Some(value); + Ok(()) + } +} diff --git a/src/pyclass.rs b/src/pyclass.rs index 95351cf45e4..ae0b0a792cf 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -10,11 +10,11 @@ use std::os::raw::c_void; use std::ptr; #[inline] -pub(crate) unsafe fn default_alloc() -> *mut ffi::PyObject { - let type_obj = T::type_object(); +pub(crate) unsafe fn default_alloc(py: Python) -> *mut ffi::PyObject { + let type_obj = T::type_object_raw(py); // if the class derives native types(e.g., PyDict), call special new if T::FLAGS & type_flags::EXTENDED != 0 && T::BaseLayout::IS_NATIVE_TYPE { - let base_tp = ::type_object(); + let base_tp = T::BaseType::type_object_raw(py); if let Some(base_new) = base_tp.tp_new { return base_new(type_obj as *const _ as _, ptr::null_mut(), ptr::null_mut()); } @@ -29,8 +29,8 @@ pub trait PyClassAlloc: PyTypeInfo + Sized { /// /// # Safety /// This function must return a valid pointer to the Python heap. - unsafe fn alloc(_py: Python) -> *mut Self::Layout { - default_alloc::() as _ + unsafe fn alloc(py: Python) -> *mut Self::Layout { + default_alloc::(py) as _ } /// Deallocate `#[pyclass]` on the Python heap. @@ -44,7 +44,7 @@ pub trait PyClassAlloc: PyTypeInfo + Sized { return; } - 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), } @@ -105,7 +105,7 @@ where s => CString::new(s)?.into_raw(), }; - type_object.tp_base = ::type_object() as *const _ as _; + type_object.tp_base = T::BaseType::type_object_raw(py) as *const _ as _; type_object.tp_name = match module_name { Some(module_name) => CString::new(format!("{}.{}", module_name, T::NAME))?.into_raw(), diff --git a/src/type_object.rs b/src/type_object.rs index 19d2b8d2fd3..ea8cc31a6d4 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -1,13 +1,13 @@ // Copyright (c) 2017-present PyO3 Project and Contributors //! Python type object information +use crate::once_cell::GILOnceCell; use crate::pyclass::{initialize_type_object, PyClass}; use crate::pyclass_init::PyObjectInit; use crate::types::{PyAny, PyType}; -use crate::{ffi, AsPyPointer, Python}; -use std::cell::UnsafeCell; -use std::ptr::NonNull; -use std::sync::atomic::{AtomicBool, Ordering}; +use crate::{ffi, AsPyPointer, PyNativeType, Python}; +use parking_lot::{const_mutex, Mutex}; +use std::thread::{self, ThreadId}; /// `T: PyLayout` represents that `T` is a concrete representaion of `U` in Python heap. /// E.g., `PyCell` is a concrete representaion of all `pyclass`es, and `ffi::PyObject` @@ -101,18 +101,21 @@ pub unsafe trait PyTypeInfo: Sized { type AsRefTarget: crate::PyNativeType; /// PyTypeObject instance for this type. - fn type_object() -> &'static ffi::PyTypeObject; + fn type_object_raw(py: Python) -> &'static ffi::PyTypeObject; /// Check if `*mut ffi::PyObject` is instance of this type fn is_instance(object: &PyAny) -> bool { unsafe { - ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object() as *const _ as _) != 0 + ffi::PyObject_TypeCheck( + object.as_ptr(), + Self::type_object(object.py()) as *const _ as _, + ) != 0 } } /// Check if `*mut ffi::PyObject` is exact instance of this type fn is_exact_instance(object: &PyAny) -> bool { - unsafe { (*object.as_ptr()).ob_type == Self::type_object() as *const _ as _ } + unsafe { (*object.as_ptr()).ob_type == Self::type_object(object.py()) as *const _ as _ } } } @@ -132,78 +135,63 @@ where T: PyTypeInfo, { fn type_object(py: Python) -> &PyType { - unsafe { py.from_borrowed_ptr(::type_object() as *const _ as _) } + unsafe { py.from_borrowed_ptr(Self::type_object_raw(py) as *const _ as _) } } } -/// Lazy type object for Exceptions -#[doc(hidden)] -pub struct LazyHeapType { - value: UnsafeCell>>, - initialized: AtomicBool, -} - -impl LazyHeapType { - pub const fn new() -> Self { - LazyHeapType { - value: UnsafeCell::new(None), - initialized: AtomicBool::new(false), - } - } - - pub fn get_or_init(&self, constructor: F) -> NonNull - where - F: Fn(Python) -> NonNull, - { - if !self - .initialized - .compare_and_swap(false, true, Ordering::Acquire) - { - // We have to get the GIL before setting the value to the global!!! - let gil = Python::acquire_gil(); - unsafe { - *self.value.get() = Some(constructor(gil.python())); - } - } - unsafe { (*self.value.get()).unwrap() } - } -} - -// This is necessary for making static `LazyHeapType`s -// -// Type objects are shared between threads by the Python interpreter anyway, so it is no worse -// to allow sharing on the Rust side too. -unsafe impl Sync for LazyHeapType {} - /// Lazy type object for PyClass #[doc(hidden)] pub struct LazyStaticType { - value: UnsafeCell, - initialized: AtomicBool, + // Boxed because Python expects the type object to have a stable address. + value: GILOnceCell>, + // Threads which have begun initialization. Used for reentrant initialization detection. + initializing_threads: Mutex>, } impl LazyStaticType { pub const fn new() -> Self { LazyStaticType { - value: UnsafeCell::new(ffi::PyTypeObject_INIT), - initialized: AtomicBool::new(false), + value: GILOnceCell::new(), + initializing_threads: const_mutex(Vec::new()), } } - pub fn get_or_init(&self) -> &ffi::PyTypeObject { - if !self - .initialized - .compare_and_swap(false, true, Ordering::Acquire) - { - let gil = Python::acquire_gil(); - let py = gil.python(); - initialize_type_object::(py, T::MODULE, unsafe { &mut *self.value.get() }) - .unwrap_or_else(|e| { - e.print(py); - panic!("An error occurred while initializing class {}", T::NAME) - }); - } - unsafe { &*self.value.get() } + pub fn get_or_init(&self, py: Python) -> &ffi::PyTypeObject { + self.value + .get_or_init(py, || { + { + // Code evaluated at class init time, such as class attributes, might lead to + // recursive initalization of the type object if the class attribute is the same + // type as the class. + // + // That could lead to all sorts of unsafety such as using incomplete type objects + // to initialize class instances, so recursive initialization is prevented. + let thread_id = thread::current().id(); + let mut threads = self.initializing_threads.lock(); + if threads.contains(&thread_id) { + panic!("Recursive initialization of type_object for {}", T::NAME); + } else { + threads.push(thread_id) + } + } + + // Okay, not recursive initialization - can proceed safely. + let mut type_object = Box::new(ffi::PyTypeObject_INIT); + + initialize_type_object::(py, T::MODULE, type_object.as_mut()).unwrap_or_else( + |e| { + e.print(py); + panic!("An error occurred while initializing class {}", T::NAME) + }, + ); + + // Initialization successfully complete, can clear the thread list. + // (No futher calls to get_or_init() will try to init, on any thread.) + *self.initializing_threads.lock() = Vec::new(); + + type_object + }) + .as_ref() } } diff --git a/src/types/mod.rs b/src/types/mod.rs index 66e53e5d2d0..6e921a2e6ba 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -143,7 +143,7 @@ macro_rules! pyobject_native_type_convert( const MODULE: Option<&'static str> = $module; #[inline] - fn type_object() -> &'static $crate::ffi::PyTypeObject { + fn type_object_raw(_py: Python) -> &'static $crate::ffi::PyTypeObject { unsafe{ &$typeobject } } diff --git a/tests/test_class_attributes.rs b/tests/test_class_attributes.rs index 085e2faf43a..daef4bbb7fd 100644 --- a/tests/test_class_attributes.rs +++ b/tests/test_class_attributes.rs @@ -30,25 +30,12 @@ impl Foo { "bar".to_string() } - #[classattr] - fn foo() -> Foo { - Foo { x: 1 } - } - #[classattr] fn bar() -> Bar { Bar { x: 2 } } } -#[pymethods] -impl Bar { - #[classattr] - fn foo() -> Foo { - Foo { x: 3 } - } -} - #[test] fn class_attributes() { let gil = Python::acquire_gil(); @@ -67,13 +54,23 @@ fn class_attributes_are_immutable() { py_expect_exception!(py, foo_obj, "foo_obj.a = 6", TypeError); } +#[pyclass] +struct SelfClassAttribute { + #[pyo3(get)] + x: i32, +} + +#[pymethods] +impl SelfClassAttribute { + #[classattr] + const SELF: SelfClassAttribute = SelfClassAttribute { x: 1 }; +} + #[test] +#[should_panic(expected = "Recursive initialization of type_object for SelfClassAttribute")] fn recursive_class_attributes() { let gil = Python::acquire_gil(); let py = gil.python(); - let foo_obj = py.get_type::(); - let bar_obj = py.get_type::(); - py_assert!(py, foo_obj, "foo_obj.foo.x == 1"); - py_assert!(py, foo_obj, "foo_obj.bar.x == 2"); - py_assert!(py, bar_obj, "bar_obj.foo.x == 3"); + + py.get_type::(); }