Various fixes to edge cases with GILGuard

CHANGELOG.md

@@ -16,12 +16,14 @@ 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)
 - `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)
 
 ### 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`. [#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)

src/err.rs

@@ -26,12 +26,12 @@ use std::ptr::NonNull;
 pub enum PyErrValue {
     None,
     Value(PyObject),
-    ToArgs(Box<dyn PyErrArguments>),
-    ToObject(Box<dyn ToPyObject>),
+    ToArgs(Box<dyn PyErrArguments + Send>),
+    ToObject(Box<dyn ToPyObject + Send>),
 }
 
 impl PyErrValue {
-    pub fn from_err_args<T: 'static + PyErrArguments>(value: T) -> Self {
+    pub fn from_err_args<T: 'static + PyErrArguments + Send>(value: T) -> Self {
         let _ = Python::acquire_gil();
         PyErrValue::ToArgs(Box::new(value))
     }
@@ -101,7 +101,7 @@ impl PyErr {
     pub fn new<T, V>(value: V) -> PyErr
     where
         T: PyTypeObject,
-        V: ToPyObject + 'static,
+        V: ToPyObject + Send + 'static,
     {
         let gil = ensure_gil();
         let py = unsafe { gil.python() };
@@ -123,7 +123,7 @@ impl PyErr {
     /// `args` is the a tuple of arguments to pass to the exception constructor.
     pub fn from_type<A>(exc: &PyType, args: A) -> PyErr
     where
-        A: ToPyObject + 'static,
+        A: ToPyObject + Send + 'static,
     {
         PyErr {
             ptype: exc.into(),

src/exceptions.rs

@@ -26,10 +26,10 @@ macro_rules! impl_exception_boilerplate {
         }
 
         impl $name {
-            pub fn py_err<V: $crate::ToPyObject + 'static>(args: V) -> $crate::PyErr {
+            pub fn py_err<V: $crate::ToPyObject + Send + 'static>(args: V) -> $crate::PyErr {
                 $crate::PyErr::new::<$name, V>(args)
             }
-            pub fn into<R, V: $crate::ToPyObject + 'static>(args: V) -> $crate::PyResult<R> {
+            pub fn into<R, V: $crate::ToPyObject + Send + 'static>(args: V) -> $crate::PyResult<R> {
                 $crate::PyErr::new::<$name, V>(args).into()
             }
         }

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
 /// ```
@@ -124,19 +125,21 @@ pub fn prepare_freethreaded_python() {
 #[must_use]
 pub struct GILGuard {
     gstate: ffi::PyGILState_STATE,
-    pool: ManuallyDrop<Option<GILPool>>,
+    pool: ManuallyDrop<Option<GILPool>>
 }
 
 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,28 +149,45 @@ 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
         };
 
         GILGuard {
             gstate,
-            pool: ManuallyDrop::new(pool),
+            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,24 +171,28 @@ 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.
+        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.