diff --git a/CHANGELOG.md b/CHANGELOG.md index f5e2e24a5d6..cc44503d7c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Correct FFI definitions `Py_SetProgramName` and `Py_SetPythonHome` to take `*const` argument instead of `*mut`. [#1021](https://github.com/PyO3/pyo3/pull/1021) - Rename `PyString::to_string` to `to_str`, change return type `Cow` to `&str`. [#1023](https://github.com/PyO3/pyo3/pull/1023) - Correct FFI definition `_PyLong_AsByteArray` `*mut c_uchar` argument instead of `*const c_uchar`. [#1029](https://github.com/PyO3/pyo3/pull/1029) +- Add `T: Send` bound to the return value of `Python::allow_threads`. [#1036](https://github.com/PyO3/pyo3/pull/1036) - Rename `PYTHON_SYS_EXECUTABLE` to `PYO3_PYTHON`. The old name will continue to work but will be undocumented, and will be removed in a future release. [#1039](https://github.com/PyO3/pyo3/pull/1039) - `PyType::as_type_ptr` is no longer `unsafe`. [#1047](https://github.com/PyO3/pyo3/pull/1047) - Change `PyIterator::from_object` to return `PyResult` instead of `Result`. [#1051](https://github.com/PyO3/pyo3/pull/1051) @@ -26,6 +27,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Removed - Remove `PyString::as_bytes`. [#1023](https://github.com/PyO3/pyo3/pull/1023) - Remove `Python::register_any`. [#1023](https://github.com/PyO3/pyo3/pull/1023) +- Remove `GILGuard::acquire` from the public API. Use `Python::acquire_gil` or `Python::with_gil`. [#1036](https://github.com/PyO3/pyo3/pull/1036) ### Fixed - Conversion from types with an `__index__` method to Rust BigInts. [#1027](https://github.com/PyO3/pyo3/pull/1027) diff --git a/guide/src/class.md b/guide/src/class.md index a1c5c524e6c..b9f7947b838 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -879,12 +879,8 @@ impl PyGCProtocol for ClassWithGCSupport { } fn __clear__(&mut self) { - if let Some(obj) = self.obj.take() { - // Release reference, this decrements ref counter. - let gil = GILGuard::acquire(); - let py = gil.python(); - py.release(obj); - } + // Clear reference, this decrements ref counter. + self.obj = None; } } ``` diff --git a/guide/src/module.md b/guide/src/module.md index 63991ab39ad..4dea21b1b9b 100644 --- a/guide/src/module.md +++ b/guide/src/module.md @@ -77,14 +77,12 @@ fn supermodule(_py: Python, module: &PyModule) -> PyResult<()> { Ok(()) } -fn nested_call() { - let gil = GILGuard::acquire(); - let py = gil.python(); - let supermodule = wrap_pymodule!(supermodule)(py); - let ctx = [("supermodule", supermodule)].into_py_dict(py); - - py.run("assert supermodule.submodule.subfunction() == 'Subfunction'", None, Some(&ctx)).unwrap(); -} +# Python::with_gil(|py| { +# let supermodule = wrap_pymodule!(supermodule)(py); +# let ctx = [("supermodule", supermodule)].into_py_dict(py); +# +# py.run("assert supermodule.submodule.subfunction() == 'Subfunction'", None, Some(&ctx)).unwrap(); +# }) ``` This way, you can create a module hierarchy within a single extension module. diff --git a/src/ffi/pystate.rs b/src/ffi/pystate.rs index cc220b1363d..9dcb7e77536 100644 --- a/src/ffi/pystate.rs +++ b/src/ffi/pystate.rs @@ -52,7 +52,7 @@ extern "C" { } #[repr(C)] -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum PyGILState_STATE { PyGILState_LOCKED, PyGILState_UNLOCKED, diff --git a/src/gil.rs b/src/gil.rs index d7ce24a9df5..989f1540d0d 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -12,13 +12,13 @@ static START: sync::Once = sync::Once::new(); thread_local! { /// This is a internal counter in pyo3 monitoring whether this thread has the GIL. /// - /// It will be incremented whenever a GILPool is created, and decremented whenever they are - /// dropped. + /// It will be incremented whenever a GILGuard or GILPool is created, and decremented whenever + /// they are dropped. /// /// As a result, if this thread has the GIL, GIL_COUNT is greater than zero. /// /// pub(crate) because it is manipulated temporarily by Python::allow_threads - pub(crate) static GIL_COUNT: Cell = Cell::new(0); + pub(crate) static GIL_COUNT: Cell = Cell::new(0); /// Temporally hold objects that will be released when the GILPool drops. static OWNED_OBJECTS: RefCell>> = RefCell::new(Vec::with_capacity(256)); @@ -110,7 +110,8 @@ pub fn prepare_freethreaded_python() { }); } -/// RAII type that represents the Global Interpreter Lock acquisition. +/// RAII type that represents the Global Interpreter Lock acquisition. To get hold of a value +/// of this type, see [`Python::acquire_gil`](struct.Python.html#method.acquire_gil). /// /// # Example /// ``` @@ -128,15 +129,17 @@ pub struct GILGuard { } impl GILGuard { - /// Acquires the global interpreter lock, which allows access to the Python runtime. This is - /// safe to call multiple times without causing a deadlock. - /// - /// If the Python runtime is not already initialized, this function will initialize it. - /// See [prepare_freethreaded_python()](fn.prepare_freethreaded_python.html) for details. + /// Retrieves the marker type that proves that the GIL was acquired. + #[inline] + pub fn python(&self) -> Python { + unsafe { Python::assume_gil_acquired() } + } + + /// PyO3 internal API for acquiring the GIL. The public API is Python::acquire_gil. /// /// If PyO3 does not yet have a `GILPool` for tracking owned PyObject references, then this /// new `GILGuard` will also contain a `GILPool`. - pub fn acquire() -> GILGuard { + pub(crate) fn acquire() -> GILGuard { prepare_freethreaded_python(); let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL @@ -146,6 +149,8 @@ impl GILGuard { let pool = if !gil_is_acquired() { Some(unsafe { GILPool::new() }) } else { + // As no GILPool was created, need to update the gil count manually. + increment_gil_count(); None }; @@ -154,20 +159,35 @@ impl GILGuard { pool: ManuallyDrop::new(pool), } } - - /// 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) { + // First up, try to detect if the order of destruction is correct. + let _ = GIL_COUNT.try_with(|c| { + if self.gstate == ffi::PyGILState_STATE::PyGILState_UNLOCKED && c.get() != 1 { + // XXX: this panic commits to leaking all objects in the pool as well as + // potentially meaning the GIL never releases. Perhaps should be an abort? + // Unfortunately abort UX is much worse than panic. + panic!("The first GILGuard acquired must be the last one dropped."); + } + }); + + // If this GILGuard doesn't own a pool, then need to decrease the count after dropping + // all objects from the pool. + let should_decrement = self.pool.is_none(); + + // Drop the objects in the pool before attempting to release the thread state unsafe { - // Must drop the objects in the pool before releasing the GILGuard ManuallyDrop::drop(&mut self.pool); + } + + if should_decrement { + decrement_gil_count(); + } + + unsafe { ffi::PyGILState_Release(self.gstate); } } @@ -328,7 +348,7 @@ pub unsafe fn register_owned(_py: Python, obj: NonNull) { #[inline(always)] fn increment_gil_count() { // Ignores the error in case this function called from `atexit`. - let _ = GIL_COUNT.with(|c| c.set(c.get() + 1)); + let _ = GIL_COUNT.try_with(|c| c.set(c.get() + 1)); } /// Decrement pyo3's internal GIL count - to be called whenever GILPool or GILGuard is dropped. @@ -522,16 +542,15 @@ mod test { drop(pool); assert_eq!(get_gil_count(), 2); - // Creating a new GILGuard should not increment the gil count if a GILPool already exists let gil2 = Python::acquire_gil(); + assert_eq!(get_gil_count(), 3); + + drop(gil2); assert_eq!(get_gil_count(), 2); drop(pool2); assert_eq!(get_gil_count(), 1); - drop(gil2); - assert_eq!(get_gil_count(), 1); - drop(gil); assert_eq!(get_gil_count(), 0); } diff --git a/src/python.rs b/src/python.rs index dbbd5ed43d7..901a426bcf0 100644 --- a/src/python.rs +++ b/src/python.rs @@ -91,6 +91,18 @@ impl<'p> Python<'p> { /// /// If the Python runtime is not already initialized, this function will initialize it. /// See [prepare_freethreaded_python()](fn.prepare_freethreaded_python.html) for details. + /// + /// Most users should not need to use this API directly, and should prefer one of two options: + /// 1. When implementing `#[pymethods]` or `#[pyfunction]` add a function argument + /// `py: Python` to receive access to the GIL context in which the function is running. + /// 2. Use [`Python::with_gil`](#method.with_gil) to run a closure with the GIL, acquiring + /// only if needed. + /// + /// **Note:** This return type from this function, `GILGuard`, is implemented as a RAII guard + /// around the C-API Python_EnsureGIL. This means that multiple `acquire_gil()` calls are + /// allowed, and will not deadlock. However, `GILGuard`s must be dropped in the reverse order + /// to acquisition. If PyO3 detects this order is not maintained, it may be forced to begin + /// an irrecoverable panic. #[inline] pub fn acquire_gil() -> GILGuard { GILGuard::acquire() @@ -135,6 +147,11 @@ impl<'p> Python<'p> { /// cannot be used in the closure. This includes `&PyAny` and all the /// concrete-typed siblings, like `&PyString`. /// + /// This is achieved via the `Send` bound on the closure and the return type. This is slightly + /// more restrictive than necessary, but it's the most fitting solution available in stable + /// Rust. In the future this bound may be relaxed by a new "auto-trait", if auto-traits + /// become a stable feature of the Rust language. + /// /// You can convert such references to e.g. `PyObject` or `Py`, /// which makes them independent of the GIL lifetime. However, you cannot /// do much with those without a `Python<'p>` token, for which you'd need to @@ -154,24 +171,28 @@ impl<'p> Python<'p> { pub fn allow_threads(self, f: F) -> T where F: Send + FnOnce() -> T, + T: Send, { // The `Send` bound on the closure prevents the user from // transferring the `Python` token into the closure. + let count = gil::GIL_COUNT.with(|c| c.replace(0)); + let tstate = unsafe { ffi::PyEval_SaveThread() }; + // Unwinding right here corrupts the Python interpreter state and leads to weird + // crashes such as stack overflows. We will catch the unwind and resume as soon as + // we've restored the GIL state. + // + // Because we will resume unwinding as soon as the GIL state is fixed, we can assert + // that the closure is unwind safe. + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); + + // Restore GIL state + gil::GIL_COUNT.with(|c| c.set(count)); unsafe { - let count = gil::GIL_COUNT.with(|c| c.replace(0)); - let save = ffi::PyEval_SaveThread(); - // Unwinding right here corrupts the Python interpreter state and leads to weird - // crashes such as stack overflows. We will catch the unwind and resume as soon as - // we've restored the GIL state. - // - // Because we will resume unwinding as soon as the GIL state is fixed, we can assert - // that the closure is unwind safe. - let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); - ffi::PyEval_RestoreThread(save); - gil::GIL_COUNT.with(|c| c.set(count)); - // Now that the GIL state has been safely reset, we can unwind if a panic was caught. - result.unwrap_or_else(|payload| std::panic::resume_unwind(payload)) + ffi::PyEval_RestoreThread(tstate); } + + // Now that the GIL state has been safely reset, we can unwind if a panic was caught. + result.unwrap_or_else(|payload| std::panic::resume_unwind(payload)) } /// Evaluates a Python expression in the given context and returns the result. diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 71a2ded98d7..1609ece9811 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -130,7 +130,7 @@ impl PyByteArray { /// # use pyo3::prelude::*; /// # use pyo3::types::PyByteArray; /// # use pyo3::types::IntoPyDict; - /// # let gil = GILGuard::acquire(); + /// # let gil = Python::acquire_gil(); /// # let py = gil.python(); /// # /// let bytearray = PyByteArray::new(py, b"Hello World."); diff --git a/src/types/iterator.rs b/src/types/iterator.rs index c6061393ce3..4ec2ddb4c74 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -87,7 +87,6 @@ mod tests { use crate::gil::GILPool; use crate::instance::AsPyRef; use crate::types::{PyDict, PyList}; - use crate::GILGuard; use crate::Python; use crate::ToPyObject; use indoc::indoc; @@ -168,7 +167,7 @@ mod tests { "# ); - let gil = GILGuard::acquire(); + let gil = Python::acquire_gil(); let py = gil.python(); let context = PyDict::new(py); @@ -183,7 +182,7 @@ mod tests { #[test] fn int_not_iterable() { - let gil = GILGuard::acquire(); + let gil = Python::acquire_gil(); let py = gil.python(); let x = 5.to_object(py); diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index 2f3ffbcd974..98b931eb7f6 100644 --- a/tests/test_arithmetics.rs +++ b/tests/test_arithmetics.rs @@ -2,6 +2,7 @@ use pyo3::class::basic::CompareOp; use pyo3::class::*; use pyo3::prelude::*; use pyo3::py_run; +use pyo3::PyNativeType; mod common; @@ -361,13 +362,11 @@ impl PyObjectProtocol for RichComparisons2 { "RC2" } - fn __richcmp__(&self, _other: &PyAny, op: CompareOp) -> PyObject { - let gil = GILGuard::acquire(); - let py = gil.python(); + fn __richcmp__(&self, other: &PyAny, op: CompareOp) -> PyObject { match op { - CompareOp::Eq => true.into_py(py), - CompareOp::Ne => false.into_py(py), - _ => py.NotImplemented(), + CompareOp::Eq => true.into_py(other.py()), + CompareOp::Ne => false.into_py(other.py()), + _ => other.py().NotImplemented(), } } } diff --git a/tests/test_dunder.rs b/tests/test_dunder.rs index 7b1f9a52508..34828ca11cf 100644 --- a/tests/test_dunder.rs +++ b/tests/test_dunder.rs @@ -4,7 +4,7 @@ use pyo3::class::{ }; use pyo3::exceptions::{PyIndexError, PyValueError}; use pyo3::prelude::*; -use pyo3::types::{IntoPyDict, PyBytes, PySlice, PyType}; +use pyo3::types::{IntoPyDict, PySlice, PyType}; use pyo3::{ffi, py_run, AsPyPointer, PyCell}; use std::convert::TryFrom; use std::{isize, iter}; @@ -94,9 +94,8 @@ impl<'p> PyObjectProtocol<'p> for StringMethods { format!("format({})", format_spec) } - fn __bytes__(&self) -> PyObject { - let gil = GILGuard::acquire(); - PyBytes::new(gil.python(), b"bytes").into() + fn __bytes__(&self) -> &'static [u8] { + b"bytes" } } @@ -374,7 +373,7 @@ impl<'p> PyContextProtocol<'p> for ContextManager { _value: Option<&'p PyAny>, _traceback: Option<&'p PyAny>, ) -> bool { - let gil = GILGuard::acquire(); + let gil = Python::acquire_gil(); self.exit_called = true; ty == Some(gil.python().get_type::()) } @@ -426,16 +425,15 @@ struct Test {} #[pyproto] impl<'p> PyMappingProtocol<'p> for Test { - fn __getitem__(&self, idx: &PyAny) -> PyResult { - let gil = GILGuard::acquire(); + fn __getitem__(&self, idx: &PyAny) -> PyResult<&'static str> { if let Ok(slice) = idx.cast_as::() { let indices = slice.indices(1000)?; if indices.start == 100 && indices.stop == 200 && indices.step == 1 { - return Ok("slice".into_py(gil.python())); + return Ok("slice"); } } else if let Ok(idx) = idx.extract::() { if idx == 1 { - return Ok("int".into_py(gil.python())); + return Ok("int"); } } Err(PyErr::new::("error")) diff --git a/tests/test_gc.rs b/tests/test_gc.rs index d6a27a17f1a..6da23dc5fb1 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -94,7 +94,7 @@ impl PyGCProtocol for GCIntegration { } fn __clear__(&mut self) { - let gil = GILGuard::acquire(); + let gil = Python::acquire_gil(); self.self_ref = gil.python().None(); } } diff --git a/tests/test_module.rs b/tests/test_module.rs index 6539bdf9797..c4727a69c2a 100644 --- a/tests/test_module.rs +++ b/tests/test_module.rs @@ -238,7 +238,7 @@ fn supermodule(_py: Python, module: &PyModule) -> PyResult<()> { fn test_module_nesting() { use pyo3::wrap_pymodule; - let gil = GILGuard::acquire(); + let gil = Python::acquire_gil(); let py = gil.python(); let supermodule = wrap_pymodule!(supermodule)(py);