Merge pull request #924 from davidhewitt/fix-deadlock

Fix deadlock in update_counts

.travis.yml

@@ -34,11 +34,6 @@ env:
     - TRAVIS_RUST_VERSION=nightly
     - RUST_BACKTRACE=1
 
-addons:
-  apt:
-    packages:
-      - libgoogle-perftools-dev
-
 before_install:
   - source ./ci/travis/setup.sh
   - curl -L https://github.com/mozilla/grcov/releases/latest/download/grcov-linux-x86_64.tar.bz2 | tar jxf -

CHANGELOG.md

@@ -5,8 +5,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Fixed
+- Fix deadlock in `Python::acquire_gil()` after dropping a `PyObject` or `Py<T>`. [#924](https://github.com/PyO3/pyo3/pull/924)
 
-## [0.10.0]
+## [0.10.0] - 2020-05-13
 ### Added
 - Add FFI definition `_PyDict_NewPresized`. [#849](https://github.com/PyO3/pyo3/pull/849)
 - Implement `IntoPy<PyObject>` for `HashSet` and `BTreeSet`. [#864](https://github.com/PyO3/pyo3/pull/864)

Cargo.toml

@@ -22,6 +22,7 @@ appveyor = { repository = "fafhrd91/pyo3" }
 indoc = { version = "0.3.4", optional = true }
 inventory = { version = "0.1.4", optional = true }
 libc = "0.2.62"
+parking_lot = "0.10.2"
 num-bigint = { version = "0.2", optional = true }
 num-complex = { version = "0.2", optional = true }
 paste = { version = "0.1.6", optional = true }

src/gil.rs

@@ -3,8 +3,8 @@
 //! Interaction with python's global interpreter lock
 
 use crate::{ffi, internal_tricks::Unsendable, Python};
-use std::cell::{Cell, RefCell, UnsafeCell};
+use parking_lot::{const_mutex, Mutex};
-use std::sync::atomic::{spin_loop_hint, AtomicBool, Ordering};
+use std::cell::{Cell, RefCell};
 use std::{any, mem::ManuallyDrop, ptr::NonNull, sync};
 
 static START: sync::Once = sync::Once::new();
@@ -168,75 +168,49 @@ impl Drop for GILGuard {
 
 /// Thread-safe storage for objects which were inc_ref / dec_ref while the GIL was not held.
 struct ReferencePool {
-    locked: AtomicBool,
+    pointers_to_incref: Mutex<Vec<NonNull<ffi::PyObject>>>,
-    pointers_to_incref: UnsafeCell<Vec<NonNull<ffi::PyObject>>>,
+    pointers_to_decref: Mutex<Vec<NonNull<ffi::PyObject>>>,
-    pointers_to_decref: UnsafeCell<Vec<NonNull<ffi::PyObject>>>,
-}
-
-struct Lock<'a> {
-    lock: &'a AtomicBool,
-}
-
-impl<'a> Lock<'a> {
-    fn new(lock: &'a AtomicBool) -> Self {
-        while lock.compare_and_swap(false, true, Ordering::Acquire) {
-            spin_loop_hint();
-        }
-        Self { lock }
-    }
-}
-
-impl<'a> Drop for Lock<'a> {
-    fn drop(&mut self) {
-        self.lock.store(false, Ordering::Release);
-    }
 }
 
 impl ReferencePool {
     const fn new() -> Self {
         Self {
-            locked: AtomicBool::new(false),
+            pointers_to_incref: const_mutex(Vec::new()),
-            pointers_to_incref: UnsafeCell::new(Vec::new()),
+            pointers_to_decref: const_mutex(Vec::new()),
-            pointers_to_decref: UnsafeCell::new(Vec::new()),
         }
     }
 
     fn register_incref(&self, obj: NonNull<ffi::PyObject>) {
-        let _lock = Lock::new(&self.locked);
+        self.pointers_to_incref.lock().push(obj)
-        let v = self.pointers_to_incref.get();
-        unsafe { (*v).push(obj) };
     }
 
     fn register_decref(&self, obj: NonNull<ffi::PyObject>) {
-        let _lock = Lock::new(&self.locked);
+        self.pointers_to_decref.lock().push(obj)
-        let v = self.pointers_to_decref.get();
-        unsafe { (*v).push(obj) };
     }
 
     fn update_counts(&self, _py: Python) {
-        let _lock = Lock::new(&self.locked);
+        macro_rules! swap_vec_with_lock {
+            // Get vec from one of ReferencePool's mutexes via lock, swap vec if needed, unlock.
+            ($cell:expr) => {{
+                let mut locked = $cell.lock();
+                let mut out = Vec::new();
+                if !locked.is_empty() {
+                    std::mem::swap(&mut out, &mut *locked);
+                }
+                drop(locked);
+                out
+            }};
+        };
 
         // Always increase reference counts first - as otherwise objects which have a
         // nonzero total reference count might be incorrectly dropped by Python during
         // this update.
-        {
+        for ptr in swap_vec_with_lock!(self.pointers_to_incref) {
-            let v = self.pointers_to_incref.get();
+            unsafe { ffi::Py_INCREF(ptr.as_ptr()) };
-            unsafe {
-                for ptr in &(*v) {
-                    ffi::Py_INCREF(ptr.as_ptr());
-                }
-                (*v).clear();
-            }
         }
 
-        {
+        for ptr in swap_vec_with_lock!(self.pointers_to_decref) {
-            let v = self.pointers_to_decref.get();
+            unsafe { ffi::Py_DECREF(ptr.as_ptr()) };
-            unsafe {
-                for ptr in &(*v) {
-                    ffi::Py_DECREF(ptr.as_ptr());
-                }
-                (*v).clear();
-            }
         }
     }
 }
@@ -637,17 +611,48 @@ mod test {
 
         // The pointer should appear once in the incref pool, and once in the
         // decref pool (for the clone being created and also dropped)
-        assert_eq!(unsafe { &*POOL.pointers_to_incref.get() }, &vec![ptr]);
+        assert_eq!(&*POOL.pointers_to_incref.lock(), &vec![ptr]);
-        assert_eq!(unsafe { &*POOL.pointers_to_decref.get() }, &vec![ptr]);
+        assert_eq!(&*POOL.pointers_to_decref.lock(), &vec![ptr]);
 
         // Re-acquring GIL will clear these pending changes
         drop(gil);
         let _gil = Python::acquire_gil();
 
-        assert!(unsafe { (*POOL.pointers_to_incref.get()).is_empty() });
+        assert!(POOL.pointers_to_incref.lock().is_empty());
-        assert!(unsafe { (*POOL.pointers_to_decref.get()).is_empty() });
+        assert!(POOL.pointers_to_decref.lock().is_empty());
 
         // Overall count is still unchanged
         assert_eq!(count, obj.get_refcnt());
     }
+
+    #[test]
+    fn test_update_counts_does_not_deadlock() {
+        // update_counts can run arbitrary Python code during Py_DECREF.
+        // if the locking is implemented incorrectly, it will deadlock.
+
+        let gil = Python::acquire_gil();
+        let obj = get_object(gil.python());
+
+        unsafe {
+            unsafe extern "C" fn capsule_drop(capsule: *mut ffi::PyObject) {
+                // This line will implicitly call update_counts
+                // -> and so cause deadlock if update_counts is not handling recursion correctly.
+                let pool = GILPool::new();
+
+                // Rebuild obj so that it can be dropped
+                PyObject::from_owned_ptr(
+                    pool.python(),
+                    ffi::PyCapsule_GetPointer(capsule, std::ptr::null()) as _,
+                );
+            }
+
+            let ptr = obj.into_ptr();
+            let capsule = ffi::PyCapsule_New(ptr as _, std::ptr::null(), Some(capsule_drop));
+
+            POOL.register_decref(NonNull::new(capsule).unwrap());
+
+            // Updating the counts will call decref on the capsule, which calls capsule_drop
+            POOL.update_counts(gil.python())
+        }
+    }
 }