Various fixes to edge cases with GILGuard

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<str>` 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<PyIterator>` instead of `Result<PyIterator, PyDowncastError>`. [#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)

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;
     }
 }
 ```

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.

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,

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<u32> = Cell::new(0);
+    pub(crate) static GIL_COUNT: Cell<usize> = Cell::new(0);
 
     /// Temporally hold objects that will be released when the GILPool drops.
     static OWNED_OBJECTS: RefCell<Vec<NonNull<ffi::PyObject>>> = 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<ffi::PyObject>) {
 #[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);
     }

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<PyString>`,
     /// 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,12 +171,12 @@ impl<'p> Python<'p> {
     pub fn allow_threads<T, F>(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.
-        unsafe {
         let count = gil::GIL_COUNT.with(|c| c.replace(0));
-            let save = ffi::PyEval_SaveThread();
+        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.
@@ -167,12 +184,16 @@ impl<'p> Python<'p> {
         // 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);
+
+        // Restore GIL state
         gil::GIL_COUNT.with(|c| c.set(count));
+        unsafe {
+            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.
     ///

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.");

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);

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(),
         }
     }
 }

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::<PyValueError>())
     }
@@ -426,16 +425,15 @@ struct Test {}
 
 #[pyproto]
 impl<'p> PyMappingProtocol<'p> for Test {
-    fn __getitem__(&self, idx: &PyAny) -> PyResult<PyObject> {
-        let gil = GILGuard::acquire();
+    fn __getitem__(&self, idx: &PyAny) -> PyResult<&'static str> {
         if let Ok(slice) = idx.cast_as::<PySlice>() {
             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::<isize>() {
             if idx == 1 {
-                return Ok("int".into_py(gil.python()));
+                return Ok("int");
             }
         }
         Err(PyErr::new::<PyValueError, _>("error"))

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();
     }
 }

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);