From b9b73f21cfaf1005480f8f681ed8e63ebb0bdf25 Mon Sep 17 00:00:00 2001 From: mejrs Date: Sat, 26 Feb 2022 15:53:35 +0100 Subject: [PATCH 1/2] Make tests not racy --- src/gil.rs | 149 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 99 insertions(+), 50 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index 01a576ebf6d..6b97b921494 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -700,71 +700,120 @@ 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) = 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_eq!(&*POOL.pointer_ops.lock().0, &[ptr]); + assert_eq!(&*POOL.pointer_ops.lock().1, &[ptr]); - // Re-acquring GIL will clear these pending changes - drop(gil); - let gil = Python::acquire_gil(); + // Re-acquring GIL will clear these pending changes + (obj, count) + }); - 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.is_empty()); + assert!(POOL.pointer_ops.lock().1.is_empty()); - // 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] From 67b77ced8dbbfd8b8f5c5eddab0085859a90b025 Mon Sep 17 00:00:00 2001 From: mejrs Date: Thu, 3 Mar 2022 11:41:33 +0100 Subject: [PATCH 2/2] be a bit more resilient to other tests --- src/gil.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index 6b97b921494..cd9d4bb4eb1 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -778,7 +778,7 @@ mod tests { // Some spinlocks for synchronizing static OBJECT_CLONED: AtomicBool = AtomicBool::new(false); - let (obj, count) = Python::with_gil(|py| { + 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); @@ -799,17 +799,16 @@ mod tests { // 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, &[ptr]); - assert_eq!(&*POOL.pointer_ops.lock().1, &[ptr]); + assert!(POOL.pointer_ops.lock().0.contains(&ptr)); + assert!(POOL.pointer_ops.lock().1.contains(&ptr)); - // Re-acquring GIL will clear these pending changes - (obj, count) + (obj, count, ptr) }); Python::with_gil(|py| { // Acquiring the gil clears the pool - assert!(POOL.pointer_ops.lock().0.is_empty()); - assert!(POOL.pointer_ops.lock().1.is_empty()); + 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(py));