From b038f82b8df9a534e4b9fdd7c3ebce530866f91c Mon Sep 17 00:00:00 2001 From: scalexm Date: Mon, 22 Jun 2020 23:13:23 +0200 Subject: [PATCH] Re-enable recursive class attributes Use some kind of two-stage initialization as described in #975, by being very cautious about when to allow the GIL to be released. --- src/once_cell.rs | 4 ++ src/pyclass.rs | 32 +++++----- src/type_object.rs | 108 +++++++++++++++++++++++++-------- tests/test_class_attributes.rs | 24 +++++--- 4 files changed, 115 insertions(+), 53 deletions(-) diff --git a/src/once_cell.rs b/src/once_cell.rs index 1502184b4ad..726475bbdf3 100644 --- a/src/once_cell.rs +++ b/src/once_cell.rs @@ -58,6 +58,10 @@ impl GILOnceCell { /// calling `f()`. Even when this happens `GILOnceCell` guarantees that only **one** write /// to the cell ever occurs - other threads will simply discard the value they compute and /// return the result of the first complete computation. + /// 3) if f() does not release the GIL and does not panic, it is guaranteed to be called + /// exactly once, even if multiple threads attempt to call `get_or_init` + /// 4) if f() can panic but still does not release the GIL, it may be called multiple times, + /// but it is guaranteed that f() will never be called concurrently pub fn get_or_init(&self, py: Python, f: F) -> &T where F: FnOnce() -> T, diff --git a/src/pyclass.rs b/src/pyclass.rs index 796239f41cd..3badb309260 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -1,10 +1,10 @@ //! `PyClass` trait use crate::class::methods::{PyClassAttributeDef, PyMethodDefType, PyMethods}; use crate::class::proto_methods::PyProtoMethods; -use crate::conversion::{AsPyPointer, FromPyPointer, IntoPyPointer, ToPyObject}; +use crate::conversion::{AsPyPointer, FromPyPointer}; use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; use crate::type_object::{type_flags, PyLayout}; -use crate::types::{PyAny, PyDict}; +use crate::types::PyAny; use crate::{class, ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; use std::ffi::CString; use std::os::raw::c_void; @@ -188,7 +188,7 @@ where // buffer protocol type_object.tp_as_buffer = T::buffer_methods().map_or_else(ptr::null_mut, |p| p.as_ptr()); - let (new, call, mut methods, attrs) = py_class_method_defs::(); + let (new, call, mut methods) = py_class_method_defs::(); // normal methods if !methods.is_empty() { @@ -196,15 +196,6 @@ where type_object.tp_methods = Box::into_raw(methods.into_boxed_slice()) as _; } - // class attributes - if !attrs.is_empty() { - let dict = PyDict::new(py); - for attr in attrs { - dict.set_item(attr.name, (attr.meth)(py))?; - } - type_object.tp_dict = dict.to_object(py).into_ptr(); - } - // __new__ method type_object.tp_new = new; // __call__ method @@ -248,14 +239,22 @@ fn py_class_flags(type_object: &mut ffi::PyTypeObject) { } } +pub(crate) fn py_class_attributes() -> Vec { + let mut attrs = vec![]; + for def in T::py_methods() { + if let PyMethodDefType::ClassAttribute(attr) = *def { + attrs.push(attr); + } + } + attrs +} + fn py_class_method_defs() -> ( Option, Option, Vec, - Vec, ) { let mut defs = Vec::new(); - let mut attrs = Vec::new(); let mut call = None; let mut new = None; @@ -278,14 +277,11 @@ fn py_class_method_defs() -> ( | PyMethodDefType::Static(ref def) => { defs.push(def.as_method_def()); } - PyMethodDefType::ClassAttribute(def) => { - attrs.push(def); - } _ => (), } } - (new, call, defs, attrs) + (new, call, defs) } fn py_class_properties() -> Vec { diff --git a/src/type_object.rs b/src/type_object.rs index c0ecd888e72..3548ac0e9a4 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -1,11 +1,12 @@ // Copyright (c) 2017-present PyO3 Project and Contributors //! Python type object information +use crate::conversion::IntoPyPointer; use crate::once_cell::GILOnceCell; -use crate::pyclass::{initialize_type_object, PyClass}; +use crate::pyclass::{initialize_type_object, py_class_attributes, PyClass}; use crate::pyclass_init::PyObjectInit; use crate::types::{PyAny, PyType}; -use crate::{ffi, AsPyPointer, PyNativeType, Python}; +use crate::{ffi, AsPyPointer, PyErr, PyNativeType, PyObject, PyResult, Python}; use parking_lot::{const_mutex, Mutex}; use std::thread::{self, ThreadId}; @@ -139,8 +140,10 @@ where pub struct LazyStaticType { // Boxed because Python expects the type object to have a stable address. value: GILOnceCell<*mut ffi::PyTypeObject>, - // Threads which have begun initialization. Used for reentrant initialization detection. + // Threads which have begun initialization of the `tp_dict`. Used for + // reentrant initialization detection. initializing_threads: Mutex>, + tp_dict_filled: GILOnceCell>, } impl LazyStaticType { @@ -148,42 +151,97 @@ impl LazyStaticType { LazyStaticType { value: GILOnceCell::new(), initializing_threads: const_mutex(Vec::new()), + tp_dict_filled: GILOnceCell::new(), } } pub fn get_or_init(&self, py: Python) -> *mut 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 type_object = *self.value.get_or_init(py, || { 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) }); + Box::into_raw(type_object) + }); + + // We might want to fill the `tp_dict` with python instances of `T` + // itself. In order to do so, we must first initialize the type object + // with an empty `tp_dict`: now we can create instances of `T`. + // + // Then we fill the `tp_dict`. Multiple threads may try to fill it at + // the same time, but only one of them will succeed. + // + // More importantly, if a thread is performing initialization of the + // `tp_dict`, it can still request the type object through `get_or_init`, + // but the `tp_dict` may appear empty of course. + + if self.tp_dict_filled.get(py).is_some() { + // `tp_dict` is already filled: ok. + return type_object; + } + + { + let thread_id = thread::current().id(); + let mut threads = self.initializing_threads.lock(); + if threads.contains(&thread_id) { + // Reentrant call: just return the type object, even if the + // `tp_dict` is not filled yet. + return type_object; + } + threads.push(thread_id); + } + + // Pre-compute the class attribute objects: this can temporarily + // release the GIL since we're calling into arbitrary user code. It + // means that another thread can continue the initialization in the + // meantime: at worst, we'll just make a useless computation. + let mut items = vec![]; + for attr in py_class_attributes::() { + items.push((attr.name, (attr.meth)(py))); + } + + // Now we hold the GIL and we can assume it won't be released until we + // return from the function. + let result = self.tp_dict_filled.get_or_init(py, move || { + let tp_dict = unsafe { (*type_object).tp_dict }; + let result = initialize_tp_dict(py, tp_dict, items); + // See discussion on #982 for why we need this. + unsafe { ffi::PyType_Modified(type_object) }; // Initialization successfully complete, can clear the thread list. - // (No futher calls to get_or_init() will try to init, on any thread.) + // (No further calls to get_or_init() will try to init, on any thread.) *self.initializing_threads.lock() = Vec::new(); + result + }); - Box::into_raw(type_object) - }) + if let Err(err) = result { + err.clone_ref(py).print(py); + panic!("An error occured while initializing `{}.__dict__`", T::NAME); + } + + type_object + } +} + +fn initialize_tp_dict( + py: Python, + tp_dict: *mut ffi::PyObject, + items: Vec<(&'static str, PyObject)>, +) -> PyResult<()> { + use std::ffi::CString; + + // We hold the GIL: the dictionary update can be considered atomic from + // the POV of other threads. + for (key, val) in items { + let ret = unsafe { + ffi::PyDict_SetItemString(tp_dict, CString::new(key)?.as_ptr(), val.into_ptr()) + }; + if ret < 0 { + return Err(PyErr::fetch(py)); + } } + Ok(()) } // This is necessary for making static `LazyStaticType`s diff --git a/tests/test_class_attributes.rs b/tests/test_class_attributes.rs index daef4bbb7fd..a2e4fe8264f 100644 --- a/tests/test_class_attributes.rs +++ b/tests/test_class_attributes.rs @@ -34,6 +34,11 @@ impl Foo { fn bar() -> Bar { Bar { x: 2 } } + + #[classattr] + fn foo() -> Foo { + Foo { x: 1 } + } } #[test] @@ -54,23 +59,22 @@ 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 { +impl Bar { #[classattr] - const SELF: SelfClassAttribute = SelfClassAttribute { x: 1 }; + fn foo() -> Foo { + Foo { x: 3 } + } } #[test] -#[should_panic(expected = "Recursive initialization of type_object for SelfClassAttribute")] fn recursive_class_attributes() { let gil = Python::acquire_gil(); let py = gil.python(); - py.get_type::(); + 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"); }