From 21f6ecf483c755366d95f190c2907cfd9b517717 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Mon, 31 Aug 2020 08:12:59 +0200 Subject: [PATCH] Fix flaky tests caused by relaxed load in C++ 'shared_ptr::use_count()' (#450) Fixes: #284 --- src/support.rs | 104 ++++++++++++++++++++++++++++++++++++++++++---- tests/test_api.rs | 62 +++++++++++++-------------- 2 files changed, 127 insertions(+), 39 deletions(-) diff --git a/src/support.rs b/src/support.rs index 19415162..26822b0b 100644 --- a/src/support.rs +++ b/src/support.rs @@ -1,9 +1,11 @@ +use std::any::type_name; use std::any::Any; use std::borrow::Borrow; use std::borrow::BorrowMut; use std::convert::identity; use std::convert::AsMut; use std::convert::AsRef; +use std::convert::TryFrom; use std::marker::PhantomData; use std::mem::align_of; use std::mem::forget; @@ -18,6 +20,9 @@ use std::ptr::null_mut; use std::ptr::NonNull; use std::rc::Rc; use std::sync::Arc; +use std::thread::yield_now; +use std::time::Duration; +use std::time::Instant; // TODO use libc::intptr_t when stable. // https://doc.rust-lang.org/1.7.0/libc/type.intptr_t.html @@ -209,12 +214,20 @@ impl Drop for SharedPtrBase { pub struct SharedPtr(SharedPtrBase); impl SharedPtr { - pub fn is_null(&self) -> bool { - ::get(&self.0).is_null() + /// Asserts that the number of references to the shared inner value is equal + /// to the `expected` count. + /// + /// This function relies on the C++ method `std::shared_ptr::use_count()`, + /// which usually performs a relaxed load. This function will repeatedly call + /// `use_count()` until it returns the expected value, for up to one second. + /// Therefore it should probably not be used in performance critical code. + #[track_caller] + pub fn assert_use_count_eq(&self, expected: usize) { + assert_shared_ptr_use_count_eq("SharedPtr", &self.0, expected); } - pub fn use_count(&self) -> long { - ::use_count(&self.0) + pub fn is_null(&self) -> bool { + ::get(&self.0).is_null() } pub fn take(&mut self) -> Option> { @@ -267,8 +280,16 @@ impl From> for SharedPtr { pub struct SharedRef(SharedPtrBase); impl SharedRef { - pub fn use_count(&self) -> long { - ::use_count(&self.0) + /// Asserts that the number of references to the shared inner value is equal + /// to the `expected` count. + /// + /// This function relies on the C++ method `std::shared_ptr::use_count()`, + /// which usually performs a relaxed load. This function will repeatedly call + /// `use_count()` until it returns the expected value, for up to one second. + /// Therefore it should probably not be used in performance critical code. + #[track_caller] + pub fn assert_use_count_eq(&self, expected: usize) { + assert_shared_ptr_use_count_eq("SharedRef", &self.0, expected); } } @@ -303,6 +324,42 @@ impl Borrow for SharedRef { } } +#[track_caller] +fn assert_shared_ptr_use_count_eq( + wrapper_type_name: &str, + shared_ptr: &SharedPtrBase, + expected: usize, +) { + let mut actual = T::use_count(shared_ptr); + let ok = match long::try_from(expected) { + Err(_) => false, // Non-`long` value can never match actual use count. + Ok(expected) if actual == expected => true, // Fast path. + Ok(expected) => { + pub const RETRY_TIMEOUT: Duration = Duration::from_secs(1); + let start = Instant::now(); + loop { + yield_now(); + actual = T::use_count(shared_ptr); + if actual == expected { + break true; + } else if start.elapsed() > RETRY_TIMEOUT { + break false; + } + } + } + }; + assert!( + ok, + "assertion failed: `{}<{}>` reference count does not match expectation\ + \n actual: {}\ + \n expected: {}", + wrapper_type_name, + type_name::(), + actual, + expected + ); +} + /// A trait for values with static lifetimes that are allocated at a fixed /// address in memory. Practically speaking, that means they're either a /// `&'static` reference, or they're heap-allocated in a `Arc`, `Box`, `Rc`, @@ -672,8 +729,13 @@ mod tests { forget(take(p)); } - fn use_count(_: &SharedPtrBase) -> long { - unimplemented!() + fn use_count(p: &SharedPtrBase) -> long { + match p { + &Self::SHARED_PTR_BASE_A => 1, + &Self::SHARED_PTR_BASE_B => 2, + p if p == &Default::default() => 0, + _ => unreachable!(), + } } } @@ -681,29 +743,55 @@ mod tests { fn shared_ptr_and_shared_ref() { let mut shared_ptr_a1 = SharedPtr(MockSharedObj::SHARED_PTR_BASE_A); assert!(!shared_ptr_a1.is_null()); + shared_ptr_a1.assert_use_count_eq(1); let shared_ref_a: SharedRef<_> = shared_ptr_a1.take().unwrap(); assert_eq!(shared_ref_a.inner, 11111); + shared_ref_a.assert_use_count_eq(1); assert!(shared_ptr_a1.is_null()); + shared_ptr_a1.assert_use_count_eq(0); let shared_ptr_a2: SharedPtr<_> = shared_ref_a.into(); assert!(!shared_ptr_a2.is_null()); + shared_ptr_a2.assert_use_count_eq(1); assert_eq!(shared_ptr_a2.unwrap().inner, 11111); let mut shared_ptr_b1 = SharedPtr(MockSharedObj::SHARED_PTR_BASE_B); assert!(!shared_ptr_b1.is_null()); + shared_ptr_b1.assert_use_count_eq(2); let shared_ref_b: SharedRef<_> = shared_ptr_b1.take().unwrap(); assert_eq!(shared_ref_b.inner, 22222); + shared_ref_b.assert_use_count_eq(2); assert!(shared_ptr_b1.is_null()); + shared_ptr_b1.assert_use_count_eq(0); let shared_ptr_b2: SharedPtr<_> = shared_ref_b.into(); assert!(!shared_ptr_b2.is_null()); + shared_ptr_b2.assert_use_count_eq(2); assert_eq!(shared_ptr_b2.unwrap().inner, 22222); } + #[test] + #[should_panic(expected = "assertion failed: \ + `SharedPtr` reference count \ + does not match expectation")] + fn shared_ptr_use_count_assertion_failed() { + let shared_ptr: SharedPtr = Default::default(); + shared_ptr.assert_use_count_eq(3); + } + + #[test] + #[should_panic(expected = "assertion failed: \ + `SharedRef` reference count \ + does not match expectation")] + fn shared_ref_use_count_assertion_failed() { + let shared_ref = SharedRef(MockSharedObj::SHARED_PTR_BASE_B); + shared_ref.assert_use_count_eq(7); + } + static TEST_OBJ_DROPPED: AtomicBool = AtomicBool::new(false); struct TestObj { diff --git a/tests/test_api.rs b/tests/test_api.rs index d7c9c938..ea97d898 100644 --- a/tests/test_api.rs +++ b/tests/test_api.rs @@ -459,22 +459,22 @@ fn backing_store_segfault() { let _setup_guard = setup(); let array_buffer_allocator = v8::new_default_allocator().make_shared(); let shared_bs = { - assert_eq!(1, v8::SharedRef::use_count(&array_buffer_allocator)); + array_buffer_allocator.assert_use_count_eq(1); let params = v8::Isolate::create_params() .array_buffer_allocator(array_buffer_allocator.clone()); - assert_eq!(2, v8::SharedRef::use_count(&array_buffer_allocator)); + array_buffer_allocator.assert_use_count_eq(2); let isolate = &mut v8::Isolate::new(params); - assert_eq!(2, v8::SharedRef::use_count(&array_buffer_allocator)); + array_buffer_allocator.assert_use_count_eq(2); let scope = &mut v8::HandleScope::new(isolate); let context = v8::Context::new(scope); let scope = &mut v8::ContextScope::new(scope, context); let ab = v8::ArrayBuffer::new(scope, 10); let shared_bs = ab.get_backing_store(); - assert_eq!(3, v8::SharedRef::use_count(&array_buffer_allocator)); + array_buffer_allocator.assert_use_count_eq(3); shared_bs }; - assert_eq!(1, v8::SharedRef::use_count(&shared_bs)); - assert_eq!(2, v8::SharedRef::use_count(&array_buffer_allocator)); + shared_bs.assert_use_count_eq(1); + array_buffer_allocator.assert_use_count_eq(2); drop(array_buffer_allocator); drop(shared_bs); // Error occurred here. } @@ -482,21 +482,21 @@ fn backing_store_segfault() { #[test] fn shared_array_buffer_allocator() { let alloc1 = v8::new_default_allocator().make_shared(); - assert_eq!(1, v8::SharedRef::use_count(&alloc1)); + alloc1.assert_use_count_eq(1); let alloc2 = alloc1.clone(); - assert_eq!(2, v8::SharedRef::use_count(&alloc1)); - assert_eq!(2, v8::SharedRef::use_count(&alloc2)); + alloc1.assert_use_count_eq(2); + alloc2.assert_use_count_eq(2); let mut alloc2 = v8::SharedPtr::from(alloc2); - assert_eq!(2, v8::SharedRef::use_count(&alloc1)); - assert_eq!(2, v8::SharedPtr::use_count(&alloc2)); + alloc1.assert_use_count_eq(2); + alloc2.assert_use_count_eq(2); drop(alloc1); - assert_eq!(1, v8::SharedPtr::use_count(&alloc2)); + alloc2.assert_use_count_eq(1); alloc2.take(); - assert_eq!(0, v8::SharedPtr::use_count(&alloc2)); + alloc2.assert_use_count_eq(0); } #[test] @@ -514,46 +514,46 @@ fn array_buffer_with_shared_backing_store() { let bs1 = ab1.get_backing_store(); assert_eq!(ab1.byte_length(), bs1.byte_length()); - assert_eq!(2, v8::SharedRef::use_count(&bs1)); + bs1.assert_use_count_eq(2); let bs2 = ab1.get_backing_store(); assert_eq!(ab1.byte_length(), bs2.byte_length()); - assert_eq!(3, v8::SharedRef::use_count(&bs1)); - assert_eq!(3, v8::SharedRef::use_count(&bs2)); + bs1.assert_use_count_eq(3); + bs2.assert_use_count_eq(3); let bs3 = ab1.get_backing_store(); assert_eq!(ab1.byte_length(), bs3.byte_length()); - assert_eq!(4, v8::SharedRef::use_count(&bs1)); - assert_eq!(4, v8::SharedRef::use_count(&bs2)); - assert_eq!(4, v8::SharedRef::use_count(&bs3)); + bs1.assert_use_count_eq(4); + bs2.assert_use_count_eq(4); + bs3.assert_use_count_eq(4); drop(bs2); - assert_eq!(3, v8::SharedRef::use_count(&bs1)); - assert_eq!(3, v8::SharedRef::use_count(&bs3)); + bs1.assert_use_count_eq(3); + bs3.assert_use_count_eq(3); drop(bs1); - assert_eq!(2, v8::SharedRef::use_count(&bs3)); + bs3.assert_use_count_eq(2); let ab2 = v8::ArrayBuffer::with_backing_store(scope, &bs3); assert_eq!(ab1.byte_length(), ab2.byte_length()); - assert_eq!(3, v8::SharedRef::use_count(&bs3)); + bs3.assert_use_count_eq(3); let bs4 = ab2.get_backing_store(); assert_eq!(ab1.byte_length(), bs4.byte_length()); - assert_eq!(4, v8::SharedRef::use_count(&bs3)); - assert_eq!(4, v8::SharedRef::use_count(&bs4)); + bs3.assert_use_count_eq(4); + bs4.assert_use_count_eq(4); let bs5 = bs4.clone(); - assert_eq!(5, v8::SharedRef::use_count(&bs3)); - assert_eq!(5, v8::SharedRef::use_count(&bs4)); - assert_eq!(5, v8::SharedRef::use_count(&bs5)); + bs3.assert_use_count_eq(5); + bs4.assert_use_count_eq(5); + bs5.assert_use_count_eq(5); drop(bs3); - assert_eq!(4, v8::SharedRef::use_count(&bs4)); - assert_eq!(4, v8::SharedRef::use_count(&bs4)); + bs4.assert_use_count_eq(4); + bs5.assert_use_count_eq(4); drop(bs4); - assert_eq!(3, v8::SharedRef::use_count(&bs5)); + bs5.assert_use_count_eq(3); } }