Merge pull request #1990 from davidhewitt/allow-threads-unwind

allow_threads: switch from `catch_unwind` to guard pattern

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fix creating `#[classattr]` by functions with the name of a known magic method. [#1969](https://github.com/PyO3/pyo3/pull/1969)
+- Fix use of `catch_unwind` in `allow_threads` which can cause fatal crashes. [#1989](https://github.com/PyO3/pyo3/pull/1989)
 - Fix build failure on PyPy when abi3 features are activated. [#1991](https://github.com/PyO3/pyo3/pull/1991)
 - Fix mingw platform detection. [#1993](https://github.com/PyO3/pyo3/pull/1993)
 

src/python.rs

@@ -326,7 +326,7 @@ impl<'py> Python<'py> {
     ///     py.allow_threads(move || {
     ///         // An example of an "expensive" Rust calculation
     ///         let sum = numbers.iter().sum();
-    ///   
+    ///
     ///         Ok(sum)
     ///     })
     /// }
@@ -367,26 +367,30 @@ impl<'py> Python<'py> {
         F: Send + FnOnce() -> T,
         T: Send,
     {
+        // Use a guard pattern to handle reacquiring the GIL, so that the GIL will be reacquired
+        // even if `f` panics.
+
+        struct RestoreGuard {
+            count: usize,
+            tstate: *mut ffi::PyThreadState,
+        }
+
+        impl Drop for RestoreGuard {
+            fn drop(&mut self) {
+                gil::GIL_COUNT.with(|c| c.set(self.count));
+                unsafe {
+                    ffi::PyEval_RestoreThread(self.tstate);
+                }
+            }
+        }
+
         // 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 {
-            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))
+        let _guard = RestoreGuard { count, tstate };
+        f()
     }
 
     /// Evaluates a Python expression in the given context and returns the result.
@@ -840,6 +844,29 @@ mod tests {
         });
     }
 
+    #[test]
+    fn test_allow_threads_releases_and_acquires_gil() {
+        Python::with_gil(|py| {
+            let b = std::sync::Arc::new(std::sync::Barrier::new(2));
+
+            let b2 = b.clone();
+            std::thread::spawn(move || Python::with_gil(|_| b2.wait()));
+
+            py.allow_threads(|| {
+                // If allow_threads does not release the GIL, this will deadlock because
+                // the thread spawned above will never be able to acquire the GIL.
+                b.wait();
+            });
+
+            unsafe {
+                // If the GIL is not reacquired at the end of allow_threads, this call
+                // will crash the Python interpreter.
+                let tstate = ffi::PyEval_SaveThread();
+                ffi::PyEval_RestoreThread(tstate);
+            }
+        });
+    }
+
     #[test]
     fn test_allow_threads_panics_safely() {
         Python::with_gil(|py| {