From 2cd466dc46e118b3d7a5a7c7ff1e6e71a9fc1790 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Sun, 12 Apr 2020 16:31:35 +0900 Subject: [PATCH 1/2] Bound 'py lifetime by GILPool when it's possible --- CHANGELOG.md | 5 +- pyo3-derive-backend/src/module.rs | 4 +- pyo3-derive-backend/src/pymethod.rs | 32 ++--- src/class/basic.rs | 11 +- src/class/buffer.rs | 10 +- src/class/gc.rs | 12 +- src/class/macros.rs | 57 ++++---- src/class/sequence.rs | 14 +- src/derive_utils.rs | 7 +- src/gil.rs | 197 ++++++++++++++-------------- src/lib.rs | 2 +- src/pyclass.rs | 6 +- src/python.rs | 4 + src/types/dict.rs | 2 +- src/types/iterator.rs | 4 +- src/types/list.rs | 6 +- tests/test_compile_error.rs | 6 + tests/test_datetime.rs | 0 tests/ui/static_ref.rs | 17 +++ tests/ui/static_ref.stderr | 11 ++ 20 files changed, 219 insertions(+), 188 deletions(-) mode change 100755 => 100644 tests/test_datetime.rs create mode 100644 tests/ui/static_ref.rs create mode 100644 tests/ui/static_ref.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index d02a3a57..d4f99244 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Changed * `PyObject` and `Py` reference counts are now decremented sooner after `drop()`. [#851](https://github.com/PyO3/pyo3/pull/851) - * When the GIL is held, the refcount is now decreased immediately on drop. (Previously would wait until just before releasing the GIL.) - * When the GIL is not held, the refcount is now decreased when the GIL is next acquired. (Previously would wait until next time the GIL was released.) +* When the GIL is held, the refcount is now decreased immediately on drop. (Previously would wait until just before releasing the GIL.) +* When the GIL is not held, the refcount is now decreased when the GIL is next acquired. (Previously would wait until next time the GIL was released.) ### Added * `_PyDict_NewPresized`. [#849](https://github.com/PyO3/pyo3/pull/849) @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed * `__radd__` and other `__r*__` methods now correctly work with operators. [#839](https://github.com/PyO3/pyo3/pull/839) * Garbage Collector causing random panics when traversing objects that were mutably borrowed. [#855](https://github.com/PyO3/pyo3/pull/855) +* `&'static Py~` is not allowed as arguments. [#869](https://github.com/PyO3/pyo3/pull/869) ## [0.9.2] diff --git a/pyo3-derive-backend/src/module.rs b/pyo3-derive-backend/src/module.rs index d6180c5b..9cd37bd7 100644 --- a/pyo3-derive-backend/src/module.rs +++ b/pyo3-derive-backend/src/module.rs @@ -220,9 +220,9 @@ fn function_c_wrapper(name: &Ident, spec: &method::FnSpec<'_>) -> TokenStream { { const _LOCATION: &'static str = concat!(stringify!(#name), "()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); let _args = _py.from_borrowed_ptr::(_args); let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs); diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index 729384c0..3d19983e 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -105,9 +105,9 @@ fn impl_wrap_common( { const _LOCATION: &'static str = concat!( stringify!(#cls), ".", stringify!(#python_name), "()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); #slf pyo3::callback::convert(_py, #body) }) @@ -124,9 +124,9 @@ fn impl_wrap_common( { const _LOCATION: &'static str = concat!( stringify!(#cls), ".", stringify!(#python_name), "()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); #slf let _args = _py.from_borrowed_ptr::(_args); let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs); @@ -155,9 +155,9 @@ pub fn impl_proto_wrap(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { _kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject { const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); let _slf = _py.from_borrowed_ptr::>(_slf); #borrow_self let _args = _py.from_borrowed_ptr::(_args); @@ -193,9 +193,9 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { use pyo3::type_object::PyTypeInfo; const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); let _args = _py.from_borrowed_ptr::(_args); let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs); @@ -225,9 +225,9 @@ pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { _kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject { const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); let _cls = pyo3::types::PyType::from_type_ptr(_py, _cls as *mut pyo3::ffi::PyTypeObject); let _args = _py.from_borrowed_ptr::(_args); let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs); @@ -257,9 +257,9 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { _kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject { const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); let _args = _py.from_borrowed_ptr::(_args); let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs); @@ -315,9 +315,9 @@ pub(crate) fn impl_wrap_getter( { const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); let _slf = _py.from_borrowed_ptr::>(_slf); #borrow_self pyo3::callback::convert(_py, #getter_impl) @@ -372,9 +372,9 @@ pub(crate) fn impl_wrap_setter( _value: *mut pyo3::ffi::PyObject, _: *mut ::std::os::raw::c_void) -> pyo3::libc::c_int { const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()"); - let _py = pyo3::Python::assume_gil_acquired(); + let _pool = pyo3::GILPool::new(); + let _py = _pool.python(); pyo3::run_callback(_py, || { - let _pool = pyo3::GILPool::new(_py); let _slf = _py.from_borrowed_ptr::>(_slf); #borrow_self let _value = _py.from_borrowed_ptr::(_value); diff --git a/src/class/basic.rs b/src/class/basic.rs index 2f1a16b5..74057bb5 100644 --- a/src/class/basic.rs +++ b/src/class/basic.rs @@ -12,7 +12,7 @@ use crate::callback::HashCallbackOutput; use crate::class::methods::PyMethodDef; use crate::{ callback, exceptions, ffi, run_callback, FromPyObject, GILPool, IntoPy, ObjectProtocol, PyAny, - PyCell, PyClass, PyErr, PyObject, PyResult, Python, + PyCell, PyClass, PyErr, PyObject, PyResult, }; use std::os::raw::c_int; @@ -218,10 +218,9 @@ where where T: for<'p> PyObjectGetAttrProtocol<'p>, { - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); run_callback(py, || { - let _pool = GILPool::new(py); - // Behave like python's __getattr__ (as opposed to __getattribute__) and check // for existing fields and methods first let existing = ffi::PyObject_GenericGetAttr(slf, arg); @@ -485,9 +484,9 @@ where where T: for<'p> PyObjectRichcmpProtocol<'p>, { - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); run_callback(py, || { - let _pool = GILPool::new(py); let slf = py.from_borrowed_ptr::>(slf); let arg = py.from_borrowed_ptr::(arg); diff --git a/src/class/buffer.rs b/src/class/buffer.rs index 2177974f..d812cbfb 100644 --- a/src/class/buffer.rs +++ b/src/class/buffer.rs @@ -6,7 +6,7 @@ //! c-api use crate::err::PyResult; use crate::gil::GILPool; -use crate::{callback, ffi, run_callback, PyCell, PyClass, PyRefMut, Python}; +use crate::{callback, ffi, run_callback, PyCell, PyClass, PyRefMut}; use std::os::raw::c_int; /// Buffer protocol interface @@ -91,9 +91,9 @@ where where T: for<'p> PyBufferGetBufferProtocol<'p>, { - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); run_callback(py, || { - let _pool = GILPool::new(py); let slf = py.from_borrowed_ptr::>(slf); let result = T::bf_getbuffer(slf.try_borrow_mut()?, arg1, arg2).into(); callback::convert(py, result) @@ -126,9 +126,9 @@ where where T: for<'p> PyBufferReleaseBufferProtocol<'p>, { - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); run_callback(py, || { - let _pool = GILPool::new(py); let slf = py.from_borrowed_ptr::>(slf); let result = T::bf_releasebuffer(slf.try_borrow_mut()?, arg1).into(); crate::callback::convert(py, result) diff --git a/src/class/gc.rs b/src/class/gc.rs index e6fe3cf7..040a6352 100644 --- a/src/class/gc.rs +++ b/src/class/gc.rs @@ -89,8 +89,8 @@ where where T: for<'p> PyGCTraverseProtocol<'p>, { - let py = Python::assume_gil_acquired(); - let _pool = crate::GILPool::new(py); + let pool = crate::GILPool::new(); + let py = pool.python(); let slf = py.from_borrowed_ptr::>(slf); let visit = PyVisit { @@ -98,8 +98,8 @@ where arg, _py: py, }; - - if let Ok(borrow) = slf.try_borrow() { + let borrow = slf.try_borrow(); + if let Ok(borrow) = borrow { match borrow.__traverse__(visit) { Ok(()) => 0, Err(PyTraverseError(code)) => code, @@ -136,8 +136,8 @@ where where T: for<'p> PyGCClearProtocol<'p>, { - let py = Python::assume_gil_acquired(); - let _pool = crate::GILPool::new(py); + let pool = crate::GILPool::new(); + let py = pool.python(); let slf = py.from_borrowed_ptr::>(slf); slf.borrow_mut().__clear__(); diff --git a/src/class/macros.rs b/src/class/macros.rs index 8d26cd03..0d570e79 100644 --- a/src/class/macros.rs +++ b/src/class/macros.rs @@ -8,9 +8,9 @@ macro_rules! py_unary_func { where T: for<'p> $trait<'p>, { - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf = py.from_borrowed_ptr::<$crate::PyCell>(slf); $crate::callback::convert(py, $call!(slf, $f)$(.map($conv))?) }) @@ -34,9 +34,9 @@ macro_rules! py_unary_refmut_func { where T: for<'p> $trait<'p>, { - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf = py.from_borrowed_ptr::<$crate::PyCell>(slf); let res = $class::$f(slf.borrow_mut()).into(); $crate::callback::convert(py, res $(.map($conv))?) @@ -69,9 +69,9 @@ macro_rules! py_binary_func { T: for<'p> $trait<'p>, { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf = py.from_borrowed_ptr::<$crate::PyCell>(slf); let arg = py.from_borrowed_ptr::<$crate::PyAny>(arg); $crate::callback::convert(py, $call!(slf, $f, arg)$(.map($conv))?) @@ -99,9 +99,9 @@ macro_rules! py_binary_num_func { T: for<'p> $trait<'p>, { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let lhs = py.from_borrowed_ptr::<$crate::PyAny>(lhs); let rhs = py.from_borrowed_ptr::<$crate::PyAny>(rhs); @@ -125,9 +125,9 @@ macro_rules! py_binary_reverse_num_func { T: for<'p> $trait<'p>, { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); // Swap lhs <-> rhs let slf = py.from_borrowed_ptr::<$crate::PyCell>(rhs); let arg = py.from_borrowed_ptr::<$crate::PyAny>(lhs); @@ -155,9 +155,9 @@ macro_rules! py_binary_self_func { { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf_ = py.from_borrowed_ptr::<$crate::PyCell>(slf); let arg = py.from_borrowed_ptr::<$crate::PyAny>(arg); call_mut!(slf_, $f, arg)?; @@ -184,9 +184,9 @@ macro_rules! py_ssizearg_func { where T: for<'p> $trait<'p>, { - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf = py.from_borrowed_ptr::<$crate::PyCell>(slf); $crate::callback::convert(py, $call!(slf, $f; arg.into())) }) @@ -209,9 +209,9 @@ macro_rules! py_ternary_func { { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf = py.from_borrowed_ptr::<$crate::PyCell>(slf); let arg1 = py .from_borrowed_ptr::<$crate::types::PyAny>(arg1) @@ -245,9 +245,9 @@ macro_rules! py_ternary_num_func { { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let arg1 = py .from_borrowed_ptr::<$crate::types::PyAny>(arg1) .extract()?; @@ -280,9 +280,9 @@ macro_rules! py_ternary_reverse_num_func { T: for<'p> $trait<'p>, { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); // Swap lhs <-> rhs let slf = py.from_borrowed_ptr::<$crate::PyCell>(arg2); let slf = slf.try_borrow()?; @@ -312,9 +312,9 @@ macro_rules! py_dummy_ternary_self_func { { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf_cell = py.from_borrowed_ptr::<$crate::PyCell>(slf); let arg1 = py.from_borrowed_ptr::<$crate::PyAny>(arg1); call_mut!(slf_cell, $f, arg1)?; @@ -338,9 +338,9 @@ macro_rules! py_func_set { { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf = py.from_borrowed_ptr::<$crate::PyCell<$generic>>(slf); if value.is_null() { @@ -374,10 +374,9 @@ macro_rules! py_func_del { { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); - if value.is_null() { let slf = py.from_borrowed_ptr::<$crate::PyCell>(slf); let name = py @@ -408,9 +407,9 @@ macro_rules! py_func_set_del { { use $crate::ObjectProtocol; - let py = $crate::Python::assume_gil_acquired(); + let pool = $crate::GILPool::new(); + let py = pool.python(); $crate::run_callback(py, || { - let _pool = $crate::GILPool::new(py); let slf = py.from_borrowed_ptr::<$crate::PyCell<$generic>>(slf); let name = py.from_borrowed_ptr::<$crate::PyAny>(name); diff --git a/src/class/sequence.rs b/src/class/sequence.rs index eeb23600..4d31ce57 100644 --- a/src/class/sequence.rs +++ b/src/class/sequence.rs @@ -7,7 +7,7 @@ use crate::conversion::{FromPyObject, IntoPy}; use crate::err::{PyErr, PyResult}; use crate::gil::GILPool; use crate::objectprotocol::ObjectProtocol; -use crate::{callback, exceptions, ffi, run_callback, PyAny, PyCell, PyClass, PyObject, Python}; +use crate::{callback, exceptions, ffi, run_callback, PyAny, PyCell, PyClass, PyObject}; use std::os::raw::c_int; /// Sequence interface @@ -256,9 +256,9 @@ mod sq_ass_item_impl { where T: for<'p> PySequenceSetItemProtocol<'p>, { - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); run_callback(py, || { - let _pool = GILPool::new(py); let slf = py.from_borrowed_ptr::>(slf); if value.is_null() { @@ -305,9 +305,9 @@ mod sq_ass_item_impl { where T: for<'p> PySequenceDelItemProtocol<'p>, { - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); run_callback(py, || { - let _pool = GILPool::new(py); let slf = py.from_borrowed_ptr::>(slf); let result = if value.is_null() { @@ -352,9 +352,9 @@ mod sq_ass_item_impl { where T: for<'p> PySequenceSetItemProtocol<'p> + for<'p> PySequenceDelItemProtocol<'p>, { - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); run_callback(py, || { - let _pool = GILPool::new(py); let slf = py.from_borrowed_ptr::>(slf); let result = if value.is_null() { diff --git a/src/derive_utils.rs b/src/derive_utils.rs index 019b49c5..ab180e87 100644 --- a/src/derive_utils.rs +++ b/src/derive_utils.rs @@ -6,7 +6,6 @@ use crate::err::PyResult; use crate::exceptions::TypeError; -use crate::init_once; use crate::instance::PyNativeType; use crate::pyclass::PyClass; use crate::pyclass_init::PyClassInitializer; @@ -138,8 +137,6 @@ impl ModuleDef { doc: &str, initializer: impl Fn(Python, &PyModule) -> PyResult<()>, ) -> PyResult<*mut ffi::PyObject> { - init_once(); - #[cfg(py_sys_config = "WITH_THREAD")] // > Changed in version 3.7: This function is now called by Py_Initialize(), so you don’t have // > to call it yourself anymore. @@ -147,11 +144,11 @@ impl ModuleDef { ffi::PyEval_InitThreads(); let module = ffi::PyModule_Create(self.0.get()); - let py = Python::assume_gil_acquired(); + let pool = GILPool::new(); + let py = pool.python(); if module.is_null() { return Err(crate::PyErr::fetch(py)); } - let _pool = GILPool::new(py); let module = py.from_owned_ptr_or_err::(module)?; module.add("__doc__", doc)?; initializer(py, module)?; diff --git a/src/gil.rs b/src/gil.rs index 5492e89e..72ff1781 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -3,12 +3,10 @@ //! Interaction with python's global interpreter lock use crate::{ffi, internal_tricks::Unsendable, PyAny, Python}; -use std::cell::Cell; -use std::ptr::NonNull; -use std::{any, sync}; +use std::cell::{Cell, UnsafeCell}; +use std::{any, mem::ManuallyDrop, ptr::NonNull, sync}; static START: sync::Once = sync::Once::new(); -static START_PYO3: sync::Once = sync::Once::new(); thread_local! { /// This is a internal counter in pyo3 monitoring whether this thread has the GIL. @@ -90,16 +88,6 @@ pub fn prepare_freethreaded_python() { // Note that the PyThreadState returned by PyEval_SaveThread is also held in TLS by the Python runtime, // and will be restored by PyGILState_Ensure. } - - init_once(); - }); -} - -#[doc(hidden)] -pub fn init_once() { - START_PYO3.call_once(|| unsafe { - // initialize release pool - POOL = Box::into_raw(Box::new(ReleasePool::new())); }); } @@ -116,28 +104,46 @@ pub fn init_once() { /// ``` #[must_use] pub struct GILGuard { - owned: usize, - borrowed: usize, gstate: ffi::PyGILState_STATE, - // Stable solution for impl !Send - no_send: Unsendable, + pool: ManuallyDrop, +} + +impl GILGuard { + /// Acquires the global interpreter lock, which allows access to the Python runtime. + /// + /// If the Python runtime is not already initialized, this function will initialize it. + /// See [prepare_freethreaded_python()](fn.prepare_freethreaded_python.html) for details. + pub fn acquire() -> GILGuard { + prepare_freethreaded_python(); + + unsafe { + let gstate = ffi::PyGILState_Ensure(); // acquire GIL + GILGuard { + gstate, + pool: ManuallyDrop::new(GILPool::new()), + } + } + } + + /// Retrieves the marker type that proves that the GIL was acquired. + #[inline] + pub fn python(&self) -> Python { + unsafe { Python::assume_gil_acquired() } + } } /// The Drop implementation for `GILGuard` will release the GIL. impl Drop for GILGuard { fn drop(&mut self) { unsafe { - let pool: &'static mut ReleasePool = &mut *POOL; - pool.drain(self.python(), self.owned, self.borrowed); + ManuallyDrop::drop(&mut self.pool); ffi::PyGILState_Release(self.gstate); } - - decrement_gil_count(); } } -/// Release pool -struct ReleasePool { +/// Implementation of release pool +struct ReleasePoolImpl { owned: ArrayList>, borrowed: ArrayList>, pointers: *mut Vec>, @@ -145,9 +151,9 @@ struct ReleasePool { p: parking_lot::Mutex<*mut Vec>>, } -impl ReleasePool { - fn new() -> ReleasePool { - ReleasePool { +impl ReleasePoolImpl { + fn new() -> Self { + Self { owned: ArrayList::new(), borrowed: ArrayList::new(), pointers: Box::into_raw(Box::new(Vec::with_capacity(256))), @@ -187,42 +193,69 @@ impl ReleasePool { } } -static mut POOL: *mut ReleasePool = ::std::ptr::null_mut(); - -#[doc(hidden)] -pub struct GILPool<'p> { - py: Python<'p>, - owned: usize, - borrowed: usize, - no_send: Unsendable, +/// Sync wrapper of ReleasePoolImpl +struct ReleasePool { + value: UnsafeCell>, } -impl<'p> GILPool<'p> { - #[inline] - pub fn new(py: Python) -> GILPool { - increment_gil_count(); - let p: &'static mut ReleasePool = unsafe { &mut *POOL }; - GILPool { - py, - owned: p.owned.len(), - borrowed: p.borrowed.len(), - no_send: Unsendable::default(), +impl ReleasePool { + const fn new() -> Self { + Self { + value: UnsafeCell::new(None), } } + /// # Safety + /// This function is not thread safe. Thus, the caller has to have GIL. + #[allow(clippy::mut_from_ref)] + unsafe fn get_or_init(&self) -> &mut ReleasePoolImpl { + (*self.value.get()).get_or_insert_with(ReleasePoolImpl::new) + } } -impl<'p> Drop for GILPool<'p> { +static POOL: ReleasePool = ReleasePool::new(); + +unsafe impl Sync for ReleasePool {} + +#[doc(hidden)] +pub struct GILPool { + owned: usize, + borrowed: usize, + // Stable solution for impl !Send + no_send: Unsendable, +} + +impl GILPool { + /// # Safety + /// This function requires that GIL is already acquired. + #[inline] + pub unsafe fn new() -> GILPool { + increment_gil_count(); + // Release objects that were dropped since last GIL acquisition + let pool = POOL.get_or_init(); + pool.release_pointers(); + GILPool { + owned: pool.owned.len(), + borrowed: pool.borrowed.len(), + no_send: Unsendable::default(), + } + } + pub unsafe fn python(&self) -> Python { + Python::assume_gil_acquired() + } +} + +impl Drop for GILPool { fn drop(&mut self) { unsafe { - let pool: &'static mut ReleasePool = &mut *POOL; - pool.drain(self.py, self.owned, self.borrowed); + let pool = POOL.get_or_init(); + pool.drain(self.python(), self.owned, self.borrowed); } decrement_gil_count(); } } pub unsafe fn register_any<'p, T: 'static>(obj: T) -> &'p T { - let pool: &'static mut ReleasePool = &mut *POOL; + let pool = POOL.get_or_init(); pool.obj.push(Box::new(obj)); pool.obj @@ -234,7 +267,7 @@ pub unsafe fn register_any<'p, T: 'static>(obj: T) -> &'p T { } pub unsafe fn register_pointer(obj: NonNull) { - let pool = &mut *POOL; + let pool = POOL.get_or_init(); if gil_is_acquired() { ffi::Py_DECREF(obj.as_ptr()) } else { @@ -243,47 +276,15 @@ pub unsafe fn register_pointer(obj: NonNull) { } pub unsafe fn register_owned(_py: Python, obj: NonNull) -> &PyAny { - let pool = &mut *POOL; + let pool = POOL.get_or_init(); &*(pool.owned.push_back(obj) as *const _ as *const PyAny) } pub unsafe fn register_borrowed(_py: Python, obj: NonNull) -> &PyAny { - let pool = &mut *POOL; + let pool = POOL.get_or_init(); &*(pool.borrowed.push_back(obj) as *const _ as *const PyAny) } -impl GILGuard { - /// Acquires the global interpreter lock, which allows access to the Python runtime. - /// - /// If the Python runtime is not already initialized, this function will initialize it. - /// See [prepare_freethreaded_python()](fn.prepare_freethreaded_python.html) for details. - pub fn acquire() -> GILGuard { - prepare_freethreaded_python(); - - unsafe { - let gstate = ffi::PyGILState_Ensure(); // acquire GIL - increment_gil_count(); - - // Release objects that were dropped since last GIL acquisition - let pool: &'static mut ReleasePool = &mut *POOL; - pool.release_pointers(); - - GILGuard { - owned: pool.owned.len(), - borrowed: pool.borrowed.len(), - gstate, - no_send: Unsendable::default(), - } - } - } - - /// Retrieves the marker type that proves that the GIL was acquired. - #[inline] - pub fn python(&self) -> Python { - unsafe { Python::assume_gil_acquired() } - } -} - /// Increment pyo3's internal GIL count - to be called whenever GILPool or GILGuard is created. #[inline(always)] fn increment_gil_count() { @@ -361,7 +362,7 @@ mod array_list { #[cfg(test)] mod test { - use super::{GILPool, NonNull, ReleasePool, GIL_COUNT, POOL}; + use super::{GILPool, NonNull, GIL_COUNT, POOL}; use crate::object::PyObject; use crate::AsPyPointer; use crate::Python; @@ -388,7 +389,7 @@ mod test { let _ref = obj.clone_ref(py); unsafe { - let p: &'static mut ReleasePool = &mut *POOL; + let p = POOL.get_or_init(); { let gil = Python::acquire_gil(); @@ -416,10 +417,10 @@ mod test { let obj_ptr = obj.as_ptr(); unsafe { - let p: &'static mut ReleasePool = &mut *POOL; + let p = POOL.get_or_init(); { - let _pool = GILPool::new(py); + let _pool = GILPool::new(); assert_eq!(p.owned.len(), 0); let _ = gil::register_owned(py, obj.into_nonnull()); @@ -427,7 +428,7 @@ mod test { assert_eq!(p.owned.len(), 1); assert_eq!(ffi::Py_REFCNT(obj_ptr), 2); { - let _pool = GILPool::new(py); + let _pool = GILPool::new(); let obj = get_object(); let _ = gil::register_owned(py, obj.into_nonnull()); assert_eq!(p.owned.len(), 2); @@ -443,9 +444,8 @@ mod test { #[test] fn test_borrowed() { - gil::init_once(); unsafe { - let p: &'static mut ReleasePool = &mut *POOL; + let p = POOL.get_or_init(); let obj = get_object(); let obj_ptr = obj.as_ptr(); @@ -469,9 +469,8 @@ mod test { #[test] fn test_borrowed_nested() { - gil::init_once(); unsafe { - let p: &'static mut ReleasePool = &mut *POOL; + let p = POOL.get_or_init(); let obj = get_object(); let obj_ptr = obj.as_ptr(); @@ -486,7 +485,7 @@ mod test { assert_eq!(ffi::Py_REFCNT(obj_ptr), 1); { - let _pool = GILPool::new(py); + let _pool = GILPool::new(); assert_eq!(p.borrowed.len(), 1); gil::register_borrowed(py, NonNull::new(obj_ptr).unwrap()); assert_eq!(p.borrowed.len(), 2); @@ -513,7 +512,7 @@ mod test { let obj_ptr = obj.as_ptr(); unsafe { - let p: &'static mut ReleasePool = &mut *POOL; + let p = POOL.get_or_init(); { assert_eq!(p.owned.len(), 0); @@ -536,7 +535,7 @@ mod test { let obj_ptr = obj.as_ptr(); unsafe { - let p: &'static mut ReleasePool = &mut *POOL; + let p = POOL.get_or_init(); { assert_eq!(p.owned.len(), 0); @@ -565,13 +564,11 @@ mod test { let gil = Python::acquire_gil(); assert_eq!(get_gil_count(), 1); - let py = gil.python(); - assert_eq!(get_gil_count(), 1); - let pool = GILPool::new(py); + let pool = unsafe { GILPool::new() }; assert_eq!(get_gil_count(), 2); - let pool2 = GILPool::new(py); + let pool2 = unsafe { GILPool::new() }; assert_eq!(get_gil_count(), 3); drop(pool); diff --git a/src/lib.rs b/src/lib.rs index 3b501a22..883b4362 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -140,7 +140,7 @@ pub use crate::conversion::{ ToBorrowedObject, ToPyObject, }; pub use crate::err::{PyDowncastError, PyErr, PyErrArguments, PyErrValue, PyResult}; -pub use crate::gil::{init_once, GILGuard, GILPool}; +pub use crate::gil::{GILGuard, GILPool}; pub use crate::instance::{AsPyRef, ManagedPyRef, Py, PyNativeType}; pub use crate::object::PyObject; pub use crate::objectprotocol::ObjectProtocol; diff --git a/src/pyclass.rs b/src/pyclass.rs index 7f5eceef..4f0f9ec1 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -2,7 +2,7 @@ use crate::class::methods::{PyMethodDefType, PyMethodsProtocol}; use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; use crate::type_object::{type_flags, PyLayout}; -use crate::{class, ffi, gil, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; +use crate::{class, ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; use std::ffi::CString; use std::os::raw::c_void; use std::ptr; @@ -111,8 +111,8 @@ where where T: PyClassAlloc, { - let py = Python::assume_gil_acquired(); - let _pool = gil::GILPool::new(py); + let pool = crate::GILPool::new(); + let py = pool.python(); ::dealloc(py, (obj as *mut T::Layout) as _) } type_object.tp_dealloc = Some(tp_dealloc_callback::); diff --git a/src/python.rs b/src/python.rs index 6c4f4d89..67cf22ea 100644 --- a/src/python.rs +++ b/src/python.rs @@ -57,6 +57,10 @@ impl<'p> Python<'p> { /// Because the output lifetime `'p` is not connected to any input parameter, /// care must be taken that the compiler infers an appropriate lifetime for `'p` /// when calling this function. + /// + /// # Safety + /// The lifetime `'p` must be shorter than the period you *assume* that you have GIL. + /// I.e., `Python<'static>` is always *really* unsafe. #[inline] pub unsafe fn assume_gil_acquired() -> Python<'p> { Python(PhantomData) diff --git a/src/types/dict.rs b/src/types/dict.rs index 8c3dc6a1..caee04ec 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -472,7 +472,7 @@ mod test { let cnt; { - let _pool = crate::GILPool::new(py); + let _pool = unsafe { crate::GILPool::new() }; let none = py.None(); cnt = none.get_refcnt(); let _dict = [(10, none)].into_py_dict(py); diff --git a/src/types/iterator.rs b/src/types/iterator.rs index 1d342cd9..88f5fc78 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -137,7 +137,7 @@ mod tests { let none; let count; { - let _pool = GILPool::new(py); + let _pool = unsafe { GILPool::new() }; let l = PyList::empty(py); none = py.None(); l.append(10).unwrap(); @@ -147,7 +147,7 @@ mod tests { } { - let _pool = GILPool::new(py); + let _pool = unsafe { GILPool::new() }; let inst = obj.as_ref(py); let mut it = inst.iter().unwrap(); diff --git a/src/types/list.rs b/src/types/list.rs index b3f97e43..1ecfa1ff 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -296,7 +296,7 @@ mod test { let cnt; { - let _pool = crate::GILPool::new(py); + let _pool = unsafe { crate::GILPool::new() }; let v = vec![2]; let ob = v.to_object(py); let list = ::try_from(ob.as_ref(py)).unwrap(); @@ -331,7 +331,7 @@ mod test { let cnt; { - let _pool = crate::GILPool::new(py); + let _pool = unsafe { crate::GILPool::new() }; let list = PyList::empty(py); let none = py.None(); cnt = none.get_refcnt(); @@ -360,7 +360,7 @@ mod test { let cnt; { - let _pool = crate::GILPool::new(py); + let _pool = unsafe { crate::GILPool::new() }; let list = PyList::empty(py); let none = py.None(); cnt = none.get_refcnt(); diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 1d6358dc..8021bd33 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -7,4 +7,10 @@ fn test_compile_errors() { t.compile_fail("tests/ui/invalid_pymethod_names.rs"); t.compile_fail("tests/ui/missing_clone.rs"); t.compile_fail("tests/ui/reject_generics.rs"); + // Since the current minimum nightly(2020-01-20) has a different error message, + // we skip a test. + // TODO(kngwyu): Remove this when we update minimum nightly. + if option_env!("TRAVIS_JOB_NAME") != Some("Minimum nightly") { + t.compile_fail("tests/ui/static_ref.rs"); + } } diff --git a/tests/test_datetime.rs b/tests/test_datetime.rs old mode 100755 new mode 100644 diff --git a/tests/ui/static_ref.rs b/tests/ui/static_ref.rs new file mode 100644 index 00000000..5202920a --- /dev/null +++ b/tests/ui/static_ref.rs @@ -0,0 +1,17 @@ +use pyo3::prelude::*; +use pyo3::types::PyList; + +#[pyclass] +struct MyClass { + list: &'static PyList, +} + +#[pymethods] +impl MyClass { + #[new] + fn new(list: &'static PyList) -> Self { + Self { list } + } +} + +fn main() {} diff --git a/tests/ui/static_ref.stderr b/tests/ui/static_ref.stderr new file mode 100644 index 00000000..82796334 --- /dev/null +++ b/tests/ui/static_ref.stderr @@ -0,0 +1,11 @@ +error[E0597]: `_pool` does not live long enough + --> $DIR/static_ref.rs:9:1 + | +9 | #[pymethods] + | ^^^^^^^^^^^- + | | | + | | `_pool` dropped here while still borrowed + | borrowed value does not live long enough + | cast requires that `_pool` is borrowed for `'static` + | + = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info) From ae8186523209a92d29aec8ecfa772f74ca933846 Mon Sep 17 00:00:00 2001 From: Yuji Kanagawa Date: Mon, 13 Apr 2020 10:38:06 +0900 Subject: [PATCH 2/2] Apply suggestions from David Co-Authored-By: David Hewitt <1939362+davidhewitt@users.noreply.github.com> --- CHANGELOG.md | 6 +++--- tests/test_compile_error.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4f99244..10de8e2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Changed * `PyObject` and `Py` reference counts are now decremented sooner after `drop()`. [#851](https://github.com/PyO3/pyo3/pull/851) -* When the GIL is held, the refcount is now decreased immediately on drop. (Previously would wait until just before releasing the GIL.) -* When the GIL is not held, the refcount is now decreased when the GIL is next acquired. (Previously would wait until next time the GIL was released.) + * When the GIL is held, the refcount is now decreased immediately on drop. (Previously would wait until just before releasing the GIL.) + * When the GIL is not held, the refcount is now decreased when the GIL is next acquired. (Previously would wait until next time the GIL was released.) ### Added * `_PyDict_NewPresized`. [#849](https://github.com/PyO3/pyo3/pull/849) @@ -20,7 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed * `__radd__` and other `__r*__` methods now correctly work with operators. [#839](https://github.com/PyO3/pyo3/pull/839) * Garbage Collector causing random panics when traversing objects that were mutably borrowed. [#855](https://github.com/PyO3/pyo3/pull/855) -* `&'static Py~` is not allowed as arguments. [#869](https://github.com/PyO3/pyo3/pull/869) +* `&'static Py~` being allowed as arguments. [#869](https://github.com/PyO3/pyo3/pull/869) ## [0.9.2] diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 8021bd33..e141853d 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -8,8 +8,8 @@ fn test_compile_errors() { t.compile_fail("tests/ui/missing_clone.rs"); t.compile_fail("tests/ui/reject_generics.rs"); // Since the current minimum nightly(2020-01-20) has a different error message, - // we skip a test. - // TODO(kngwyu): Remove this when we update minimum nightly. + // we skip this test. + // TODO(kngwyu): Remove this `if` when we update minimum nightly. if option_env!("TRAVIS_JOB_NAME") != Some("Minimum nightly") { t.compile_fail("tests/ui/static_ref.rs"); }