From 39534ba96d31fbeaf1033178d195f9ecccd61a98 Mon Sep 17 00:00:00 2001 From: Bruno Kolenbrander <59372212+mejrs@users.noreply.github.com> Date: Thu, 3 Mar 2022 12:33:21 +0100 Subject: [PATCH] Make gil tests not racy (#2190) * Make tests not racy * be a bit more resilient to other tests --- src/gil.rs | 148 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 98 insertions(+), 50 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index d2fe1e029f8..ebc75a50c5c 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -701,71 +701,119 @@ mod tests { #[test] fn test_clone_without_gil() { - let gil = Python::acquire_gil(); - let py = gil.python(); - let obj = get_object(py); - let count = obj.get_refcnt(py); + use crate::{Py, PyAny}; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::{sync::Arc, thread}; + + // Some spinlocks for synchronizing + static GIL_ACQUIRED: AtomicBool = AtomicBool::new(false); + static OBJECT_CLONED: AtomicBool = AtomicBool::new(false); + static REFCNT_CHECKED: AtomicBool = AtomicBool::new(false); + + Python::with_gil(|py| { + let obj: Arc> = Arc::new(get_object(py)); + let thread_obj = Arc::clone(&obj); + + let count = (&*obj).get_refcnt(py); + println!( + "1: The object has been created and its reference count is {}", + count + ); - // Cloning without GIL should not update reference count - drop(gil); - let c = obj.clone(); - assert_eq!( - count, - obj.get_refcnt(unsafe { Python::assume_gil_acquired() }) - ); + let handle = thread::spawn(move || { + Python::with_gil(move |py| { + println!("3. The GIL has been acquired on another thread."); + GIL_ACQUIRED.store(true, Ordering::Release); - // Acquring GIL will clear this pending change - let gil = Python::acquire_gil(); - let py = gil.python(); + // Spin a bit while the main thread registers obj in POOL + 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."); - // Total reference count should be one higher - assert_eq!(count + 1, obj.get_refcnt(py)); + // spin until the gil has been acquired on the thread. + while !GIL_ACQUIRED.load(Ordering::Acquire) {} - // Clone dropped - drop(c); + 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(|_| {}); + }); - // Overall count is now back to the original, and should be no pending change - assert_eq!(count, obj.get_refcnt(py)); + 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(); - let py = gil.python(); - let obj = get_object(py); - let count = obj.get_refcnt(py); - - // Move obj to a thread which does not have the GIL, and clone it - let t = std::thread::spawn(move || { - // 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() }) - ); + use crate::Py; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::{sync::Arc, thread}; + + // Some spinlocks for synchronizing + static OBJECT_CLONED: AtomicBool = AtomicBool::new(false); + + let (obj, count, ptr) = Python::with_gil(|py| { + 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); + }); - // Return obj so original thread can continue to use - obj - }); + while !OBJECT_CLONED.load(Ordering::Acquire) {} + assert_eq!(count, obj.get_refcnt(py)); - let obj = t.join().unwrap(); - let ptr = NonNull::new(obj.as_ptr()).unwrap(); + 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_eq!(&POOL.pointer_ops.lock().0, &vec![ptr]); - assert_eq!(&POOL.pointer_ops.lock().1, &vec![ptr]); + // 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)); - // Re-acquring GIL will clear these pending changes - drop(gil); - let gil = Python::acquire_gil(); + (obj, count, ptr) + }); - assert!(POOL.pointer_ops.lock().0.is_empty()); - assert!(POOL.pointer_ops.lock().1.is_empty()); + Python::with_gil(|py| { + // Acquiring the gil clears the pool + assert!(!POOL.pointer_ops.lock().0.contains(&ptr)); + assert!(!POOL.pointer_ops.lock().1.contains(&ptr)); - // Overall count is still unchanged - assert_eq!(count, obj.get_refcnt(gil.python())); + // Overall count is still unchanged + assert_eq!(count, obj.get_refcnt(py)); + }); } #[test]