Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky tests caused by relaxed load in C++ 'shared_ptr::use_count()' #450

Merged
merged 2 commits into from
Sep 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/array_buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl Shared for Allocator {
)
}
}
fn get(ptr: &SharedPtrBase<Self>) -> *mut Self {
fn get(ptr: &SharedPtrBase<Self>) -> *const Self {
unsafe { std__shared_ptr__v8__ArrayBuffer__Allocator__get(ptr) }
}
fn reset(ptr: &mut SharedPtrBase<Self>) {
Expand Down Expand Up @@ -219,7 +219,7 @@ impl Shared for BackingStore {
std__shared_ptr__v8__BackingStore__CONVERT__std__unique_ptr(unique_ptr)
}
}
fn get(ptr: &SharedPtrBase<Self>) -> *mut Self {
fn get(ptr: &SharedPtrBase<Self>) -> *const Self {
unsafe { std__shared_ptr__v8__BackingStore__get(ptr) }
}
fn reset(ptr: &mut SharedPtrBase<Self>) {
Expand Down
185 changes: 177 additions & 8 deletions src/support.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -178,14 +183,15 @@ where
{
fn clone(shared_ptr: &SharedPtrBase<Self>) -> SharedPtrBase<Self>;
fn from_unique_ptr(unique_ptr: UniquePtr<Self>) -> SharedPtrBase<Self>;
fn get(shared_ptr: &SharedPtrBase<Self>) -> *mut Self;
fn get(shared_ptr: &SharedPtrBase<Self>) -> *const Self;
fn reset(shared_ptr: &mut SharedPtrBase<Self>);
fn use_count(shared_ptr: &SharedPtrBase<Self>) -> long;
}

/// Private base type which is shared by the `SharedPtr` and `SharedRef`
/// implementations.
#[repr(C)]
#[derive(Eq, PartialEq)]
pub struct SharedPtrBase<T: Shared>([usize; 2], PhantomData<T>);

unsafe impl<T: Shared + Sync> Send for SharedPtrBase<T> {}
Expand All @@ -205,16 +211,23 @@ impl<T: Shared> Drop for SharedPtrBase<T> {

/// Wrapper around a C++ shared_ptr. A shared_ptr may be be null.
#[repr(C)]
#[derive(Default)]
pub struct SharedPtr<T: Shared>(SharedPtrBase<T>);

impl<T: Shared> SharedPtr<T> {
pub fn is_null(&self) -> bool {
<T as Shared>::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]
piscisaureus marked this conversation as resolved.
Show resolved Hide resolved
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 {
<T as Shared>::use_count(&self.0)
pub fn is_null(&self) -> bool {
<T as Shared>::get(&self.0).is_null()
}

pub fn take(&mut self) -> Option<SharedRef<T>> {
Expand All @@ -238,6 +251,12 @@ impl<T: Shared> Clone for SharedPtr<T> {
}
}

impl<T: Shared> Default for SharedPtr<T> {
fn default() -> Self {
Self(Default::default())
}
}

impl<T, U> From<U> for SharedPtr<T>
where
T: Shared,
Expand All @@ -261,8 +280,16 @@ impl<T: Shared> From<SharedRef<T>> for SharedPtr<T> {
pub struct SharedRef<T: Shared>(SharedPtrBase<T>);

impl<T: Shared> SharedRef<T> {
pub fn use_count(&self) -> long {
<T as Shared>::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);
}
}

Expand Down Expand Up @@ -297,6 +324,42 @@ impl<T: Shared> Borrow<T> for SharedRef<T> {
}
}

#[track_caller]
fn assert_shared_ptr_use_count_eq<T: Shared>(
wrapper_type_name: &str,
shared_ptr: &SharedPtrBase<T>,
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;
}
}
ry marked this conversation as resolved.
Show resolved Hide resolved
}
};
assert!(
ok,
"assertion failed: `{}<{}>` reference count does not match expectation\
\n actual: {}\
\n expected: {}",
wrapper_type_name,
type_name::<T>(),
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`,
Expand Down Expand Up @@ -625,26 +688,132 @@ where
#[cfg(test)]
mod tests {
use super::*;
use std::ptr::null;
use std::sync::atomic::AtomicBool;
use std::sync::atomic::Ordering;

#[derive(Eq, PartialEq)]
struct MockSharedObj {
pub inner: u32,
}

impl MockSharedObj {
const INSTANCE_A: Self = Self { inner: 11111 };
const INSTANCE_B: Self = Self { inner: 22222 };

const SHARED_PTR_BASE_A: SharedPtrBase<Self> =
SharedPtrBase([1, 1], PhantomData);
const SHARED_PTR_BASE_B: SharedPtrBase<Self> =
SharedPtrBase([2, 2], PhantomData);
}

impl Shared for MockSharedObj {
fn clone(_: &SharedPtrBase<Self>) -> SharedPtrBase<Self> {
unimplemented!()
}

fn from_unique_ptr(_: UniquePtr<Self>) -> SharedPtrBase<Self> {
unimplemented!()
}

fn get(p: &SharedPtrBase<Self>) -> *const Self {
match p {
&Self::SHARED_PTR_BASE_A => &Self::INSTANCE_A,
&Self::SHARED_PTR_BASE_B => &Self::INSTANCE_B,
p if p == &Default::default() => null(),
_ => unreachable!(),
}
}

fn reset(p: &mut SharedPtrBase<Self>) {
forget(take(p));
}

fn use_count(p: &SharedPtrBase<Self>) -> long {
match p {
&Self::SHARED_PTR_BASE_A => 1,
&Self::SHARED_PTR_BASE_B => 2,
p if p == &Default::default() => 0,
_ => unreachable!(),
}
}
}

#[test]
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<rusty_v8::support::tests::MockSharedObj>` reference count \
does not match expectation")]
fn shared_ptr_use_count_assertion_failed() {
let shared_ptr: SharedPtr<MockSharedObj> = Default::default();
shared_ptr.assert_use_count_eq(3);
}

#[test]
#[should_panic(expected = "assertion failed: \
`SharedRef<rusty_v8::support::tests::MockSharedObj>` 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 {
pub id: u32,
}

impl Drop for TestObj {
fn drop(&mut self) {
assert!(!TEST_OBJ_DROPPED.swap(true, Ordering::SeqCst));
}
}

struct TestObjRef(TestObj);

impl Deref for TestObjRef {
type Target = TestObj;

fn deref(&self) -> &TestObj {
&self.0
}
}

impl Borrow<TestObj> for TestObjRef {
fn borrow(&self) -> &TestObj {
&**self
Expand Down
Loading