Make gil tests not racy (#2190)

* Make tests not racy

* be a bit more resilient to other tests

src/gil.rs

@@ -701,71 +701,119 @@ mod tests {
 
     #[test]
     fn test_clone_without_gil() {
-        let gil = Python::acquire_gil();
+        use crate::{Py, PyAny};
-        let py = gil.python();
+        use std::sync::atomic::{AtomicBool, Ordering};
-        let obj = get_object(py);
+        use std::{sync::Arc, thread};
-        let count = obj.get_refcnt(py);
 
-        // Cloning without GIL should not update reference count
+        // Some spinlocks for synchronizing
-        drop(gil);
+        static GIL_ACQUIRED: AtomicBool = AtomicBool::new(false);
-        let c = obj.clone();
+        static OBJECT_CLONED: AtomicBool = AtomicBool::new(false);
-        assert_eq!(
+        static REFCNT_CHECKED: AtomicBool = AtomicBool::new(false);
-            count,
-            obj.get_refcnt(unsafe { Python::assume_gil_acquired() })
-        );
 
-        // Acquring GIL will clear this pending change
+        Python::with_gil(|py| {
-        let gil = Python::acquire_gil();
+            let obj: Arc<Py<PyAny>> = Arc::new(get_object(py));
-        let py = gil.python();
+            let thread_obj = Arc::clone(&obj);
 
-        // Total reference count should be one higher
+            let count = (&*obj).get_refcnt(py);
-        assert_eq!(count + 1, obj.get_refcnt(py));
+            println!(
+                "1: The object has been created and its reference count is {}",
+                count
+            );
 
-        // Clone dropped
+            let handle = thread::spawn(move || {
-        drop(c);
+                Python::with_gil(move |py| {
+                    println!("3. The GIL has been acquired on another thread.");
+                    GIL_ACQUIRED.store(true, Ordering::Release);
 
-        // Overall count is now back to the original, and should be no pending change
+                    // Spin a bit while the main thread registers obj in POOL
-        assert_eq!(count, obj.get_refcnt(py));
+                    while !OBJECT_CLONED.load(Ordering::Acquire) {}
+                    println!("5. Checking refcnt");
+                    assert_eq!(thread_obj.get_refcnt(py), count);
+
+                    REFCNT_CHECKED.store(true, Ordering::Release);
+                })
+            });
+
+            let cloned = py.allow_threads(|| {
+                println!("2. The GIL has been released.");
+
+                // spin until the gil has been acquired on the thread.
+                while !GIL_ACQUIRED.load(Ordering::Acquire) {}
+
+                println!("4. The other thread is now hogging the GIL, we clone without it held");
+                // Cloning without GIL should not update reference count
+                let cloned = Py::clone(&*obj);
+                OBJECT_CLONED.store(true, Ordering::Release);
+                cloned
+            });
+
+            while !REFCNT_CHECKED.load(Ordering::Acquire) {}
+
+            // Returning from allow_threads doesn't clear the pool
+            py.allow_threads(|| {
+                // Acquiring GIL will clear the pending change
+                Python::with_gil(|_| {});
+            });
+
+            println!("6. The main thread has acquired the GIL again and processed the pool.");
+
+            // Total reference count should be one higher
+            assert_eq!(obj.get_refcnt(py), count + 1);
+
+            // Clone dropped
+            drop(cloned);
+            // Ensure refcount of the arc is 1
+            handle.join().unwrap();
+
+            // Overall count is now back to the original, and should be no pending change
+            assert_eq!(Arc::try_unwrap(obj).unwrap().get_refcnt(py), count);
+        });
     }
 
     #[test]
     fn test_clone_in_other_thread() {
-        let gil = Python::acquire_gil();
+        use crate::Py;
-        let py = gil.python();
+        use std::sync::atomic::{AtomicBool, Ordering};
-        let obj = get_object(py);
+        use std::{sync::Arc, thread};
-        let count = obj.get_refcnt(py);
 
-        // Move obj to a thread which does not have the GIL, and clone it
+        // Some spinlocks for synchronizing
-        let t = std::thread::spawn(move || {
+        static OBJECT_CLONED: AtomicBool = AtomicBool::new(false);
-            // Cloning without GIL should not update reference count
-            #[allow(clippy::redundant_clone)]
-            let _ = obj.clone();
-            assert_eq!(
-                count,
-                obj.get_refcnt(unsafe { Python::assume_gil_acquired() })
-            );
 
-            // Return obj so original thread can continue to use
+        let (obj, count, ptr) = Python::with_gil(|py| {
-            obj
+            let obj = Arc::new(get_object(py));
+            let count = obj.get_refcnt(py);
+            let thread_obj = Arc::clone(&obj);
+
+            // Start a thread which does not have the GIL, and clone it
+            let t = thread::spawn(move || {
+                // Cloning without GIL should not update reference count
+                #[allow(clippy::redundant_clone)]
+                let _ = Py::clone(&*thread_obj);
+                OBJECT_CLONED.store(true, Ordering::Release);
+            });
+
+            while !OBJECT_CLONED.load(Ordering::Acquire) {}
+            assert_eq!(count, obj.get_refcnt(py));
+
+            t.join().unwrap();
+            let ptr = NonNull::new(obj.as_ptr()).unwrap();
+
+            // The pointer should appear once in the incref pool, and once in the
+            // decref pool (for the clone being created and also dropped)
+            assert!(POOL.pointer_ops.lock().0.contains(&ptr));
+            assert!(POOL.pointer_ops.lock().1.contains(&ptr));
+
+            (obj, count, ptr)
         });
 
-        let obj = t.join().unwrap();
+        Python::with_gil(|py| {
-        let ptr = NonNull::new(obj.as_ptr()).unwrap();
+            // Acquiring the gil clears the pool
+            assert!(!POOL.pointer_ops.lock().0.contains(&ptr));
+            assert!(!POOL.pointer_ops.lock().1.contains(&ptr));
 
-        // The pointer should appear once in the incref pool, and once in the
+            // Overall count is still unchanged
-        // decref pool (for the clone being created and also dropped)
+            assert_eq!(count, obj.get_refcnt(py));
-        assert_eq!(&POOL.pointer_ops.lock().0, &vec![ptr]);
+        });
-        assert_eq!(&POOL.pointer_ops.lock().1, &vec![ptr]);
-
-        // Re-acquring GIL will clear these pending changes
-        drop(gil);
-        let gil = Python::acquire_gil();
-
-        assert!(POOL.pointer_ops.lock().0.is_empty());
-        assert!(POOL.pointer_ops.lock().1.is_empty());
-
-        // Overall count is still unchanged
-        assert_eq!(count, obj.get_refcnt(gil.python()));
     }
 
     #[test]