Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Add GILProtected synchronization primitive and use it for LazyTypeObjectInner. #2975

Merged
merged 1 commit into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion guide/src/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

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`]: {{#PYO3_DOCS_URL}}/pyo3/once_cell/struct.GILOnceCell.html
[`GILOnceCell`]: {{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html

## I can't run `cargo test`; or I can't build in a Cargo workspace: I'm having linker issues like "Symbol not found" or "Undefined reference to _PyExc_SystemError"!

Expand Down
1 change: 1 addition & 0 deletions newsfragments/2975.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add `GILProtected<T>` to mediate concurrent access to a value using Python's global interpreter lock (GIL).
4 changes: 2 additions & 2 deletions src/exceptions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ macro_rules! import_exception {

impl $name {
fn type_object_raw(py: $crate::Python<'_>) -> *mut $crate::ffi::PyTypeObject {
use $crate::once_cell::GILOnceCell;
use $crate::sync::GILOnceCell;
use $crate::AsPyPointer;
static TYPE_OBJECT: GILOnceCell<$crate::Py<$crate::types::PyType>> =
GILOnceCell::new();
Expand Down Expand Up @@ -241,7 +241,7 @@ macro_rules! create_exception_type_object {

impl $name {
fn type_object_raw(py: $crate::Python<'_>) -> *mut $crate::ffi::PyTypeObject {
use $crate::once_cell::GILOnceCell;
use $crate::sync::GILOnceCell;
use $crate::AsPyPointer;
static TYPE_OBJECT: GILOnceCell<$crate::Py<$crate::types::PyType>> =
GILOnceCell::new();
Expand Down
2 changes: 1 addition & 1 deletion src/impl_/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ unsafe fn bpo_35810_workaround(_py: Python<'_>, ty: *mut ffi::PyTypeObject) {
{
// Must check version at runtime for abi3 wheels - they could run against a higher version
// than the build config suggests.
use crate::once_cell::GILOnceCell;
use crate::sync::GILOnceCell;
static IS_PYTHON_3_8: GILOnceCell<bool> = GILOnceCell::new();

if *IS_PYTHON_3_8.get_or_init(_py, || _py.version_info() >= (3, 8)) {
Expand Down
26 changes: 15 additions & 11 deletions src/impl_/pyclass/lazy_type_object.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
use std::{
borrow::Cow,
cell::RefCell,
ffi::CStr,
marker::PhantomData,
thread::{self, ThreadId},
};

use parking_lot::{const_mutex, Mutex};

use crate::{
exceptions::PyRuntimeError, ffi, once_cell::GILOnceCell, pyclass::create_type_object,
types::PyType, AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject,
PyResult, Python,
exceptions::PyRuntimeError,
ffi,
pyclass::create_type_object,
sync::{GILOnceCell, GILProtected},
types::PyType,
AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject, PyResult, Python,
};

use super::PyClassItemsIter;
Expand All @@ -24,7 +26,7 @@ struct LazyTypeObjectInner {
value: GILOnceCell<Py<PyType>>,
// Threads which have begun initialization of the `tp_dict`. Used for
// reentrant initialization detection.
initializing_threads: Mutex<Vec<ThreadId>>,
initializing_threads: GILProtected<RefCell<Vec<ThreadId>>>,
tp_dict_filled: GILOnceCell<()>,
}

Expand All @@ -34,7 +36,7 @@ impl<T> LazyTypeObject<T> {
LazyTypeObject(
LazyTypeObjectInner {
value: GILOnceCell::new(),
initializing_threads: const_mutex(Vec::new()),
initializing_threads: GILProtected::new(RefCell::new(Vec::new())),
tp_dict_filled: GILOnceCell::new(),
},
PhantomData,
Expand Down Expand Up @@ -109,7 +111,7 @@ impl LazyTypeObjectInner {

let thread_id = thread::current().id();
{
let mut threads = self.initializing_threads.lock();
let mut threads = self.initializing_threads.get(py).borrow_mut();
if threads.contains(&thread_id) {
// Reentrant call: just return the type object, even if the
// `tp_dict` is not filled yet.
Expand All @@ -119,18 +121,20 @@ impl LazyTypeObjectInner {
}

struct InitializationGuard<'a> {
initializing_threads: &'a Mutex<Vec<ThreadId>>,
initializing_threads: &'a GILProtected<RefCell<Vec<ThreadId>>>,
py: Python<'a>,
thread_id: ThreadId,
}
impl Drop for InitializationGuard<'_> {
fn drop(&mut self) {
let mut threads = self.initializing_threads.lock();
let mut threads = self.initializing_threads.get(self.py).borrow_mut();
threads.retain(|id| *id != self.thread_id);
}
}

let guard = InitializationGuard {
initializing_threads: &self.initializing_threads,
py,
thread_id,
};

Expand Down Expand Up @@ -170,7 +174,7 @@ impl LazyTypeObjectInner {
// Initialization successfully complete, can clear the thread list.
// (No further calls to get_or_init() will try to init, on any thread.)
std::mem::forget(guard);
*self.initializing_threads.lock() = Vec::new();
self.initializing_threads.get(py).replace(Vec::new());
result
});

Expand Down
11 changes: 10 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ mod instance;
pub mod marker;
pub mod marshal;
#[macro_use]
pub mod once_cell;
pub mod sync;
pub mod panic;
pub mod prelude;
pub mod pycell;
Expand All @@ -420,6 +420,15 @@ pub mod type_object;
pub mod types;
mod version;

#[doc(hidden)]
#[deprecated(since = "0.19.0", note = "Please use the `sync` module instead.")]
pub mod once_cell {
// FIXME: We want to deprecate these,
// but that does not yet work for re-exports,
// c.f. https://github.com/rust-lang/rust/issues/30827
pub use crate::sync::{GILOnceCell, Interned};
}

pub use crate::conversions::*;

#[cfg(feature = "macros")]
Expand Down
45 changes: 42 additions & 3 deletions src/once_cell.rs → src/sync.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,46 @@
//! A write-once cell mediated by the Python GIL.
//! Synchronization mechanisms based on the Python GIL.
use crate::{types::PyString, Py, Python};
use std::cell::UnsafeCell;

/// Value with concurrent access protected by the GIL.
///
/// This is a synchronization primitive based on Python's global interpreter lock (GIL).
/// It ensures that only one thread at a time can access the inner value via shared references.
/// It can be combined with interior mutability to obtain mutable references.
///
/// # Example
///
/// Combining `GILProtected` with `RefCell` enables mutable access to static data:
///
/// ```
/// # use pyo3::prelude::*;
/// use pyo3::sync::GILProtected;
/// use std::cell::RefCell;
///
/// static NUMBERS: GILProtected<RefCell<Vec<i32>>> = GILProtected::new(RefCell::new(Vec::new()));
///
/// Python::with_gil(|py| {
/// NUMBERS.get(py).borrow_mut().push(42);
/// });
/// ```
pub struct GILProtected<T> {
value: T,
}

impl<T> GILProtected<T> {
/// Place the given value under the protection of the GIL.
pub const fn new(value: T) -> Self {
Self { value }
}

/// Gain access to the inner value by giving proof of having acquired the GIL.
pub fn get<'py>(&'py self, _py: Python<'py>) -> &'py T {
&self.value
}
}

unsafe impl<T> Sync for GILProtected<T> where T: Send {}

/// A write-once cell similar to [`once_cell::OnceCell`](https://docs.rs/once_cell/latest/once_cell/).
///
/// Unlike `once_cell::sync` which blocks threads to achieve thread safety, this implementation
Expand All @@ -25,7 +64,7 @@ use std::cell::UnsafeCell;
/// between threads:
///
/// ```
/// use pyo3::once_cell::GILOnceCell;
/// use pyo3::sync::GILOnceCell;
/// use pyo3::prelude::*;
/// use pyo3::types::PyList;
///
Expand Down Expand Up @@ -170,7 +209,7 @@ impl<T> GILOnceCell<T> {
#[macro_export]
macro_rules! intern {
($py: expr, $text: expr) => {{
static INTERNED: $crate::once_cell::Interned = $crate::once_cell::Interned::new($text);
static INTERNED: $crate::sync::Interned = $crate::sync::Interned::new($text);
INTERNED.get($py)
}};
}
Expand Down
2 changes: 1 addition & 1 deletion src/types/mapping.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) 2017-present PyO3 Project and Contributors

use crate::err::{PyDowncastError, PyErr, PyResult};
use crate::once_cell::GILOnceCell;
use crate::sync::GILOnceCell;
use crate::type_object::PyTypeInfo;
use crate::types::{PyAny, PyDict, PySequence, PyType};
use crate::{ffi, AsPyPointer, IntoPyPointer, Py, PyNativeType, PyTryFrom, Python, ToPyObject};
Expand Down
2 changes: 1 addition & 1 deletion src/types/sequence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::exceptions::PyTypeError;
#[cfg(feature = "experimental-inspect")]
use crate::inspect::types::TypeInfo;
use crate::internal_tricks::get_ssize_index;
use crate::once_cell::GILOnceCell;
use crate::sync::GILOnceCell;
use crate::type_object::PyTypeInfo;
use crate::types::{PyAny, PyList, PyString, PyTuple, PyType};
use crate::{ffi, PyNativeType, ToPyObject};
Expand Down