diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 1563b6ba6..9bc9d2bd1 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -176,6 +176,20 @@ jobs: - name: "Test tree borrows" run: MIRIFLAGS="-Zmiri-tree-borrows" cargo miri test -p godot-cell + proptest: + name: proptest + runs-on: ubuntu-20.04 + steps: + - uses: actions/checkout@v4 + + - name: "Install Rust" + uses: ./.github/composite/rust + + - name: "Compile tests" + run: cargo test -p godot-cell --features="proptest" --no-run + + - name: "Test" + run: cargo test -p godot-cell --features="proptest" # For complex matrix workflow, see https://stackoverflow.com/a/65434401 godot-itest: diff --git a/godot-cell/Cargo.toml b/godot-cell/Cargo.toml index 2cb8aebb5..d94592469 100644 --- a/godot-cell/Cargo.toml +++ b/godot-cell/Cargo.toml @@ -3,5 +3,8 @@ name = "godot-cell" version = "0.1.0" edition = "2021" -[dev-dependencies] -proptest = "1.4.0" +[features] +proptest = ["dep:proptest"] + +[dependencies] +proptest = { version = "1.4.0", optional = true } diff --git a/godot-cell/src/borrow_state.rs b/godot-cell/src/borrow_state.rs index 5fed93fe7..a4545eeb9 100644 --- a/godot-cell/src/borrow_state.rs +++ b/godot-cell/src/borrow_state.rs @@ -73,7 +73,7 @@ impl BorrowState { /// Set self as having reached an erroneous or unreliable state. /// /// Always returns [`BorrowStateErr::Poisoned`]. - fn poison(&mut self, err: impl Into) -> Result<(), BorrowStateErr> { + pub(crate) fn poison(&mut self, err: impl Into) -> Result<(), BorrowStateErr> { self.poisoned = true; Err(BorrowStateErr::Poisoned(err.into())) @@ -230,6 +230,10 @@ impl BorrowState { Ok(self.inaccessible_count) } + pub(crate) fn may_unset_inaccessible(&self) -> bool { + !self.has_accessible() && self.shared_count() == 0 && self.inaccessible_count > 0 + } + pub fn unset_inaccessible(&mut self) -> Result { if self.has_accessible() { return Err("cannot set current reference as accessible when an accessible mutable reference already exists".into()); @@ -294,7 +298,7 @@ impl From for BorrowStateErr { } } -#[cfg(all(test, not(miri)))] +#[cfg(all(test, feature = "proptest"))] mod proptests { use super::*; use proptest::{arbitrary::Arbitrary, collection::vec, prelude::*}; diff --git a/godot-cell/src/guards.rs b/godot-cell/src/guards.rs index c947ecc07..815c136bd 100644 --- a/godot-cell/src/guards.rs +++ b/godot-cell/src/guards.rs @@ -7,7 +7,7 @@ use std::ops::{Deref, DerefMut}; use std::ptr::NonNull; -use std::sync::Mutex; +use std::sync::{Mutex, MutexGuard}; use crate::CellState; @@ -69,8 +69,9 @@ impl<'a, T> Drop for RefGuard<'a, T> { /// Wraps a mutably borrowed value of type `T`. /// -/// This prevents all other borrows of `value`, unless the `&mut` reference handed out from this guard is -/// made inaccessible by a call to [`GdCell::make_inaccessible()`](crate::GdCell::make_inaccessible). +/// This prevents all other borrows of `value` while this guard is accessible. To make this guard +/// inaccessible, use [`GdCell::make_inaccessible()`](crate::GdCell::make_inaccessible) on a mutable +/// reference handed out by this guard. #[derive(Debug)] pub struct MutGuard<'a, T> { state: &'a Mutex>, @@ -101,11 +102,11 @@ impl<'a, T> MutGuard<'a, T> { /// This is the case because: /// - [`GdCell`](super::GdCell) will not create any new references while this guard exists and is /// accessible. - /// - When it is marked as inaccessible it is impossible to have any `&self` or `&mut self` references to - /// this guard that can be used. Because we take in a `&mut self` reference with a lifetime `'a` and - /// return an [`InaccessibleGuard`] with a lifetime `'b` where `'a: 'b` which ensure that the - /// `&mut self` outlives that guard and cannot be used until the guard is dropped. And the rust - /// borrow-checker will prevent any new references from being made. + /// - When it is made inaccessible it is impossible to have any `&self` or `&mut self` references to this + /// guard that can be used. Because we take in a `&mut self` reference with a lifetime `'a` and return + /// an [`InaccessibleGuard`] with a lifetime `'b` where `'a: 'b` which ensure that the `&mut self` + /// outlives that guard and cannot be used until the guard is dropped. And the rust borrow-checker will + /// prevent any new references from being made. /// - When it is made inaccessible, [`GdCell`](super::GdCell) will also ensure that any new references /// are derived from this guard's `value` pointer, thus preventing `value` from being invalidated. pub(crate) unsafe fn new( @@ -128,13 +129,23 @@ impl<'a, T> Deref for MutGuard<'a, T> { let count = self.state.lock().unwrap().borrow_state.mut_count(); // This is just a best-effort error check. It should never be triggered. assert_eq!( - self.count, count, - "attempted to access the non-current mutable borrow. **this is a bug, please report it**" + self.count, + count, + "\ + attempted to access a non-current mutable borrow of type: `{}`. \n\ + current count: {}\n\ + value pointer: {:p}\n\ + attempted access count: {}\n\ + **this is a bug, please report it**\ + ", + std::any::type_name::(), + self.count, + self.value, + count ); - // SAFETY: - // It is safe to call `as_ref()` on value when we have a `&self` reference because of the safety - // invariants of `new`. + // SAFETY: It is safe to call `as_ref()` on value when we have a `&self` reference because of the + // safety invariants of `new`. unsafe { self.value.as_ref() } } } @@ -144,8 +155,19 @@ impl<'a, T> DerefMut for MutGuard<'a, T> { let count = self.state.lock().unwrap().borrow_state.mut_count(); // This is just a best-effort error check. It should never be triggered. assert_eq!( - self.count, count, - "attempted to access the non-current mutable borrow. **this is a bug, please report it**" + self.count, + count, + "\ + attempted to access a non-current mutable borrow of type: `{}`. \n\ + current count: {}\n\ + value pointer: {:p}\n\ + attempted access count: {}\n\ + **this is a bug, please report it**\ + ", + std::any::type_name::(), + self.count, + self.value, + count ); // SAFETY: @@ -170,14 +192,15 @@ impl<'a, T> Drop for MutGuard<'a, T> { /// A guard that ensures a mutable reference is kept inaccessible until it is dropped. /// -/// The current reference is stored in the guard and we push a new reference to `state` on creation. We then -/// undo this upon dropping the guard. +/// We store the current reference in the guard upon creation, and push a new reference to `state` on +/// creation. When the guard is dropped, `state`'s pointer is reset to the original pointer. /// -/// This ensure that any new references are derived from the new reference we pass in, and when this guard is -/// dropped we reset it to the previous reference. +/// This ensures that any new references are derived from the new reference we pass in, and when this guard +/// is dropped the state is reset to what it was before, as if this guard never existed. #[derive(Debug)] pub struct InaccessibleGuard<'a, T> { state: &'a Mutex>, + stack_depth: usize, prev_ptr: NonNull, } @@ -185,7 +208,13 @@ impl<'a, T> InaccessibleGuard<'a, T> { /// Create a new inaccessible guard for `state`. /// /// Since `'b` must outlive `'a`, we cannot have any other references aliasing `new_ref` while this - /// guard exists. + /// guard exists. So this guard ensures that the guard that handed out `new_ref` is inaccessible while + /// this guard exists. + /// + /// Will error if: + /// - There is currently no accessible mutable borrow. + /// - There are any shared references. + /// - `new_ref` is not equal to the pointer in `state`. pub(crate) fn new<'b>( state: &'a Mutex>, new_ref: &'b mut T, @@ -205,16 +234,53 @@ impl<'a, T> InaccessibleGuard<'a, T> { guard.borrow_state.set_inaccessible()?; let prev_ptr = guard.get_ptr(); - guard.set_ptr(new_ptr); + let stack_depth = guard.push_ptr(new_ptr); + + Ok(Self { + state, + stack_depth, + prev_ptr, + }) + } - Ok(Self { state, prev_ptr }) + /// Single implementation of drop-logic for use in both drop implementations. + fn perform_drop( + mut state: MutexGuard<'_, CellState>, + prev_ptr: NonNull, + stack_depth: usize, + ) { + if state.stack_depth != stack_depth { + state + .borrow_state + .poison("cannot drop inaccessible guards in the wrong order") + .unwrap(); + } + state.borrow_state.unset_inaccessible().unwrap(); + state.pop_ptr(prev_ptr); + } + + /// Drop self if possible, otherwise returns self again. + /// + /// Used currently in the mock-tests, as we need a thread safe way to drop self. Using the normal drop + /// logic may poison state, however it should not cause any UB either way. + #[doc(hidden)] + pub fn try_drop(self) -> Result<(), std::mem::ManuallyDrop> { + let manual = std::mem::ManuallyDrop::new(self); + let state = manual.state.lock().unwrap(); + if !state.borrow_state.may_unset_inaccessible() || state.stack_depth != manual.stack_depth { + return Err(manual); + } + Self::perform_drop(state, manual.prev_ptr, manual.stack_depth); + + Ok(()) } } impl<'a, T> Drop for InaccessibleGuard<'a, T> { fn drop(&mut self) { - let mut state = self.state.lock().unwrap(); - state.borrow_state.unset_inaccessible().unwrap(); - state.set_ptr(self.prev_ptr); + // Default behavior of drop-logic simply panics and poisons the cell on failure. This is appropriate + // for single-threaded code where no errors should happen here. + let state = self.state.lock().unwrap(); + Self::perform_drop(state, self.prev_ptr, self.stack_depth); } } diff --git a/godot-cell/src/lib.rs b/godot-cell/src/lib.rs index 0d6f264a2..1677454e0 100644 --- a/godot-cell/src/lib.rs +++ b/godot-cell/src/lib.rs @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -//! A re-entrant cell implementation which allows for `&mut` references to be re-borrowed even while `&mut` +//! A re-entrant cell implementation which allows for `&mut` references to be reborrowed even while `&mut` //! references still exist. //! //! This is done by ensuring any existing `&mut` references cannot alias the new reference, and that the new @@ -17,9 +17,11 @@ //! Instead of directly using the concept of `aliasing` pointers, we use the term `accessible` instead. A //! reference (or other pointer) to some value is considered accessible when it is possible to either read //! from or write to the value it points to without using `unsafe`. Importantly, if we know that a reference -//! `a` is inaccessible, and then we create a new reference `b` to the same value, then we know for sure that -//! `b` wont alias `a`. This is because aliasing in rust is based on accesses, and if we never access `a` -//! then we cannot ever violate aliasing for `a` and `b`. +//! `a` is inaccessible, and then we create a new reference `b` derived from `a` to the same value, then we +//! know for sure that `b` wont alias `a`. This is because aliasing in rust is based on accesses, and if we +//! never access `a` then we cannot ever violate aliasing for `a` and `b`. And since `b` is derived from `a` +//! (that is, `b` was created from `a` somehow such as by casting `a` to a raw pointer then to a reference +//! `b`), then `a` wont get invalidated by accesses to `b`. mod borrow_state; mod guards; @@ -34,7 +36,7 @@ use std::sync::Mutex; use borrow_state::BorrowState; pub use guards::{InaccessibleGuard, MutGuard, RefGuard}; -/// A cell which can hand out new `&mut` references to its value even when one already exists, as long as +/// A cell which can hand out new `&mut` references to its value even when one already exists. As long as /// any such pre-existing references have been handed back to the cell first, and no shared references exist. /// /// This cell must be pinned to be usable, as it stores self-referential pointers. @@ -71,8 +73,8 @@ impl GdCell { let mut state = self.state.lock().unwrap(); state.borrow_state.increment_shared()?; - // SAFETY: - // `increment_shared` succeeded, therefore there cannot currently be any accessible mutable references. + // SAFETY: `increment_shared` succeeded, therefore there cannot currently be any accessible mutable + // references. unsafe { Ok(RefGuard::new(&self.get_ref().state, state.get_ptr())) } } @@ -85,32 +87,30 @@ impl GdCell { let count = state.borrow_state.mut_count(); let value = state.get_ptr(); - // SAFETY: - // `increment_mut` succeeded, therefore any existing mutable references are inaccessible. + // SAFETY: `increment_mut` succeeded, therefore any existing mutable references are inaccessible. // Additionally no new references can be created, unless the returned guard is made inaccessible. // - // This is the case because the only way for a new `GdMut` or `GdRef` to be made after this, is for - // either this guard to be dropped or `make_inaccessible` to be called. + // This is the case because the only way for a new `GdMut` or `GdRef` to be made after this is for + // either this guard to be dropped or `make_inaccessible` to be called and succeed. // // If this guard is dropped, then we dont need to worry. // - // If `make_inaccessible` is called, then either a mutable reference from this guard is passed in. - // In which case, we cannot use this guard again until the resulting inaccessible guard is dropped. - // - // We cannot pass in a different mutable reference, since `make_inaccessible` ensures any references - // matches the ones this one would return. And only one mutable reference to the same value can exist - // since we cannot have any other aliasing mutable references around to pass in. + // If `make_inaccessible` is called and succeeds, then a mutable reference from this guard is passed + // in. In which case, we cannot use this guard again until the resulting inaccessible guard is + // dropped. unsafe { Ok(MutGuard::new(&self.get_ref().state, count, value)) } } /// Make the current mutable borrow inaccessible, thus freeing the value up to be reborrowed again. /// - /// Will error if there is no current accessible mutable borrow, or if there are any shared - /// references. - pub fn make_inaccessible<'a: 'b, 'b>( - self: Pin<&'a Self>, - current_ref: &'b mut T, - ) -> Result, Box> { + /// Will error if: + /// - There is currently no accessible mutable borrow. + /// - There are any shared references. + /// - `current_ref` is not equal to the pointer in `self.state`. + pub fn make_inaccessible<'cell: 'val, 'val>( + self: Pin<&'cell Self>, + current_ref: &'val mut T, + ) -> Result, Box> { InaccessibleGuard::new(&self.get_ref().state, current_ref) } @@ -130,8 +130,7 @@ impl GdCell { } } -// SAFETY: -// `T` is sync so we can return references to it on different threads. +// SAFETY: `T` is sync so we can return references to it on different threads. // Additionally all internal state is synchronized via a mutex, so we wont have race conditions when trying // to use it from multiple threads. unsafe impl Sync for GdCell {} @@ -145,14 +144,20 @@ struct CellState { /// Current pointer to the value. /// - /// This is an `Option` as we must initialize the value after pinning it. + /// This will always be non-null after initialization. /// - /// When a reference is handed to a cell to enable re-entrancy, then this pointer is set to that + /// When a reference is handed to a cell to enable reborrowing, then this pointer is set to that /// reference. /// /// We always generate new pointer based off of the pointer currently in this field, to ensure any new /// references are derived from the most recent `&mut` reference. - ptr: Option>, + // TODO: Consider using `NonNull` instead. + ptr: *mut T, + + /// How many pointers have been handed out. + /// + /// This is used to ensure that the pointers are not replaced in the wrong order. + stack_depth: usize, } impl CellState { @@ -161,14 +166,16 @@ impl CellState { fn new() -> Self { Self { borrow_state: BorrowState::new(), - ptr: None, + ptr: std::ptr::null_mut(), + stack_depth: 0, } } /// Initialize the pointer if it is `None`. fn initialize_ptr(&mut self, value: &UnsafeCell) { - if self.ptr.is_none() { - self.set_ptr(NonNull::new(value.get()).unwrap()); + if self.ptr.is_null() { + self.ptr = value.get(); + assert!(!self.ptr.is_null()); } else { panic!("Cannot initialize pointer as it is already initialized.") } @@ -176,12 +183,21 @@ impl CellState { /// Returns the current pointer. Panics if uninitialized. fn get_ptr(&self) -> NonNull { - self.ptr.unwrap() + NonNull::new(self.ptr).unwrap() + } + + /// Push a pointer to this state.. + fn push_ptr(&mut self, new_ptr: NonNull) -> usize { + self.ptr = new_ptr.as_ptr(); + self.stack_depth += 1; + self.stack_depth } - /// Set the current pointer to the new pointer. - fn set_ptr(&mut self, new_ptr: NonNull) { - self.ptr = Some(new_ptr); + /// Pop a pointer to this state, resetting it to the given old pointer. + fn pop_ptr(&mut self, old_ptr: NonNull) -> usize { + self.ptr = old_ptr.as_ptr(); + self.stack_depth -= 1; + self.stack_depth } } diff --git a/godot-cell/tests/mock.rs b/godot-cell/tests/mock.rs index c991054a7..05f8105e5 100644 --- a/godot-cell/tests/mock.rs +++ b/godot-cell/tests/mock.rs @@ -35,12 +35,12 @@ fn binding() -> &'static Mutex> { fn register_instance(instance: T) -> usize { static COUNTER: AtomicUsize = AtomicUsize::new(0); + let key = COUNTER.fetch_add(1, std::sync::atomic::Ordering::AcqRel); + let binding = binding(); let mut guard = binding.lock().unwrap(); - let key = COUNTER.fetch_add(1, std::sync::atomic::Ordering::AcqRel); - assert!(!guard.contains_key(&key)); let cell = GdCell::new(instance); @@ -105,23 +105,42 @@ impl Base { struct BaseGuard<'a, T> { instance_id: usize, - _inaccessible_guard: InaccessibleGuard<'a, T>, + inaccessible_guard: Option>, } impl<'a, T> BaseGuard<'a, T> { fn new(instance_id: usize, inaccessible_guard: InaccessibleGuard<'a, T>) -> Self { Self { instance_id, - _inaccessible_guard: inaccessible_guard, + inaccessible_guard: Some(inaccessible_guard), } } - fn call_immut(&self, f: fn(&T)) { - unsafe { call_immut_method(self.instance_id, f).unwrap() } + fn call_immut(&self, f: fn(&T)) -> Result<(), Box> { + unsafe { call_immut_method(self.instance_id, f) } } - fn call_mut(&self, f: fn(&mut T)) { - unsafe { call_mut_method(self.instance_id, f).unwrap() } + fn call_mut(&self, f: fn(&mut T)) -> Result<(), Box> { + unsafe { call_mut_method(self.instance_id, f) } + } +} + +impl<'a, T> Drop for BaseGuard<'a, T> { + fn drop(&mut self) { + // Block while waiting for the guard to be droppable. + // This is only needed to make multi-threaded code work nicely, the default drop-impl for + // `InaccessibleGuard` may panic and get poisoned in multi-threaded code, which is non-ideal but + // still safe behavior. + let mut guard_opt = Some(std::mem::ManuallyDrop::new( + self.inaccessible_guard.take().unwrap(), + )); + + while let Some(guard) = guard_opt.take() { + if let Err(new_guard) = std::mem::ManuallyDrop::into_inner(guard).try_drop() { + guard_opt = Some(new_guard); + std::hint::spin_loop() + } + } } } @@ -166,7 +185,7 @@ impl MyClass { println!("mut_calls_immut #1: int is {}", self.int); self.int += 1; println!("mut_calls_immut #2: int is now {}", self.int); - self.base().call_immut(Self::immut_method); + _ = self.base().call_immut(Self::immut_method); println!("mut_calls_immut #3: int is now {}", self.int); } @@ -174,7 +193,7 @@ impl MyClass { println!("mut_calls_mut #1: int is {}", self.int); self.int += 1; println!("mut_calls_mut #2: int is now {}", self.int); - self.base().call_mut(Self::mut_method); + _ = self.base().call_mut(Self::mut_method); println!("mut_calls_mut #3: int is now {}", self.int); } @@ -182,7 +201,7 @@ impl MyClass { println!("mut_calls_twice #1: int is {}", self.int); self.int += 1; println!("mut_calls_twice #2: int is now {}", self.int); - self.base().call_mut(Self::mut_method_calls_immut); + _ = self.base().call_mut(Self::mut_method_calls_immut); println!("mut_calls_twice #3: int is now {}", self.int); } @@ -190,7 +209,7 @@ impl MyClass { println!("mut_calls_twice_mut #1: int is {}", self.int); self.int += 1; println!("mut_calls_twice_mut #2: int is now {}", self.int); - self.base().call_mut(Self::mut_method_calls_mut); + _ = self.base().call_mut(Self::mut_method_calls_mut); println!("mut_calls_twice_mut #3: int is now {}", self.int); } @@ -212,104 +231,205 @@ fn call_works() { unsafe { call_immut_method(instance_id, MyClass::immut_method).unwrap() }; } +/// `instance_id` must be the key of a `MyClass`. +unsafe fn get_int(instance_id: usize) -> i64 { + let storage = unsafe { get_instance::(instance_id) }; + let bind = storage.cell.as_ref().borrow().unwrap(); + bind.int +} + +/// `instance_id` must be the key of a `MyClass`. +unsafe fn assert_id_is(instance_id: usize, target: i64) { + let storage = unsafe { get_instance::(instance_id) }; + let bind = storage.cell.as_ref().borrow().unwrap(); + assert_eq!(bind.int, target); +} + +/// A list of each calls to each method of `MyClass`. The numbers are the minimum and maximum increment +/// of the method call. +static CALLS: &[(unsafe fn(usize) -> Result<(), Box>, i64, i64)] = &[ + ( + |id| unsafe { call_immut_method(id, MyClass::immut_method) }, + 0, + 0, + ), + ( + |id| unsafe { call_mut_method(id, MyClass::mut_method) }, + 1, + 1, + ), + ( + |id| unsafe { call_mut_method(id, MyClass::mut_method_calls_immut) }, + 1, + 1, + ), + ( + |id| unsafe { call_mut_method(id, MyClass::mut_method_calls_mut) }, + 1, + 2, + ), + ( + |id| unsafe { call_mut_method(id, MyClass::mut_method_calls_twice) }, + 1, + 2, + ), + ( + |id| unsafe { call_mut_method(id, MyClass::mut_method_calls_twice_mut) }, + 1, + 3, + ), + ( + |id| unsafe { call_immut_method(id, MyClass::immut_calls_immut_directly) }, + 0, + 0, + ), +]; + +/// Run each test once ensuring the integer changes as expected. #[test] fn all_calls_work() { let instance_id = MyClass::init(); - fn assert_int_is(instance_id: usize, target: i64) { - let storage = unsafe { get_instance::(instance_id) }; - let bind = storage.cell.as_ref().borrow().unwrap(); - assert_eq!(bind.int, target); + unsafe { + assert_id_is(instance_id, 0); } - assert_int_is(instance_id, 0); - unsafe { call_immut_method(instance_id, MyClass::immut_method).unwrap() }; - assert_int_is(instance_id, 0); - unsafe { call_mut_method(instance_id, MyClass::mut_method).unwrap() }; - assert_int_is(instance_id, 1); - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_immut).unwrap() }; - assert_int_is(instance_id, 2); - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_mut).unwrap() }; - assert_int_is(instance_id, 4); - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_twice).unwrap() }; - assert_int_is(instance_id, 6); - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_twice_mut).unwrap() }; - assert_int_is(instance_id, 9); - unsafe { call_immut_method(instance_id, MyClass::immut_calls_immut_directly).unwrap() }; - assert_int_is(instance_id, 9); + // We're not running in parallel, so it will never fail to increment completely. + for (f, _, expected_increment) in CALLS { + let start = unsafe { get_int(instance_id) }; + unsafe { + f(instance_id).unwrap(); + } + unsafe { + assert_id_is(instance_id, start + *expected_increment); + } + } } +/// Run each method both from the main thread and a newly created thread. #[test] fn calls_different_thread() { use std::thread; let instance_id = MyClass::init(); - fn assert_int_is(instance_id: usize, target: i64) { - let storage = unsafe { get_instance::(instance_id) }; - let bind = storage.cell.as_ref().borrow().unwrap(); - assert_eq!(bind.int, target); + + // We're not running in parallel, so it will never fail to increment completely. + for (f, _, expected_increment) in CALLS { + let start = unsafe { get_int(instance_id) }; + unsafe { + f(instance_id).unwrap(); + + assert_id_is(instance_id, start + expected_increment); + } + let start = start + expected_increment; + thread::spawn(move || unsafe { f(instance_id).unwrap() }) + .join() + .unwrap(); + unsafe { + assert_id_is(instance_id, start + expected_increment); + } } +} - assert_int_is(instance_id, 0); +/// Call each method from different threads, allowing them to run in parallel. +/// +/// This may cause borrow failures, we do a best-effort attempt at estimating the value then. We can detect +/// if the first call failed, so then we know the integer was incremented by 0. Otherwise we at least know +/// the range of values that it can be incremented by. +#[test] +fn calls_parallel() { + use std::thread; - unsafe { call_immut_method(instance_id, MyClass::immut_method).unwrap() }; - assert_int_is(instance_id, 0); - thread::spawn(move || unsafe { - call_immut_method(instance_id, MyClass::immut_method).unwrap() - }) - .join() - .unwrap(); - assert_int_is(instance_id, 0); - - unsafe { call_mut_method(instance_id, MyClass::mut_method).unwrap() }; - assert_int_is(instance_id, 1); - thread::spawn(move || unsafe { call_mut_method(instance_id, MyClass::mut_method).unwrap() }) - .join() + let instance_id = MyClass::init(); + let mut handles = Vec::new(); + + for (f, min_increment, max_increment) in CALLS { + let handle = thread::spawn(move || unsafe { + f(instance_id).map_or((0, 0), |_| (*min_increment, *max_increment)) + }); + handles.push(handle); + } + + let (min_expected, max_expected) = handles + .into_iter() + .map(|handle| handle.join().unwrap()) + .reduce(|(curr_min, curr_max), (min, max)| (curr_min + min, curr_max + max)) + .unwrap(); + + unsafe { + assert!(get_int(instance_id) >= min_expected); + assert!(get_int(instance_id) <= max_expected); + } +} + +/// Call each method from different threads, allowing them to run in parallel. +/// +/// This may cause borrow failures, we do a best-effort attempt at estimating the value then. We can detect +/// if the first call failed, so then we know the integer was incremented by 0. Otherwise we at least know +/// the range of values that it can be incremented by. +/// +/// Runs each method several times in a row. This should reduce the non-determinism that comes from +/// scheduling of threads. +#[test] +fn calls_parallel_many_serial() { + use std::thread; + + let instance_id = MyClass::init(); + let mut handles = Vec::new(); + + for (f, min_increment, max_increment) in CALLS { + for _ in 0..10 { + let handle = thread::spawn(move || unsafe { + f(instance_id).map_or((0, 0), |_| (*min_increment, *max_increment)) + }); + handles.push(handle); + } + } + + let (min_expected, max_expected) = handles + .into_iter() + .map(|handle| handle.join().unwrap()) + .reduce(|(curr_min, curr_max), (min, max)| (curr_min + min, curr_max + max)) .unwrap(); - assert_int_is(instance_id, 2); - - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_immut).unwrap() }; - assert_int_is(instance_id, 3); - thread::spawn(move || unsafe { - call_mut_method(instance_id, MyClass::mut_method_calls_immut).unwrap() - }) - .join() - .unwrap(); - assert_int_is(instance_id, 4); - - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_mut).unwrap() }; - assert_int_is(instance_id, 6); - thread::spawn(move || unsafe { - call_mut_method(instance_id, MyClass::mut_method_calls_mut).unwrap() - }) - .join() - .unwrap(); - assert_int_is(instance_id, 8); - - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_twice).unwrap() }; - assert_int_is(instance_id, 10); - thread::spawn(move || unsafe { - call_mut_method(instance_id, MyClass::mut_method_calls_twice).unwrap() - }) - .join() - .unwrap(); - assert_int_is(instance_id, 12); - - unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_twice_mut).unwrap() }; - assert_int_is(instance_id, 15); - thread::spawn(move || unsafe { - call_mut_method(instance_id, MyClass::mut_method_calls_twice_mut).unwrap() - }) - .join() - .unwrap(); - assert_int_is(instance_id, 18); - - unsafe { call_immut_method(instance_id, MyClass::immut_calls_immut_directly).unwrap() }; - assert_int_is(instance_id, 18); - thread::spawn(move || unsafe { - call_immut_method(instance_id, MyClass::immut_calls_immut_directly).unwrap() - }) - .join() - .unwrap(); - assert_int_is(instance_id, 18); + + unsafe { + assert!(get_int(instance_id) >= min_expected); + assert!(get_int(instance_id) <= max_expected); + } +} + +/// Call each method from different threads, allowing them to run in parallel. +/// +/// This may cause borrow failures, we do a best-effort attempt at estimating the value then. We can detect +/// if the first call failed, so then we know the integer was incremented by 0. Otherwise we at least know +/// the range of values that it can be incremented by. +/// +/// Runs all the tests several times. This is different from [`calls_parallel_many_serial`] as that calls the +/// methods like AAA...BBB...CCC..., whereas this interleaves the methods like ABC...ABC...ABC... +#[test] +fn calls_parallel_many_parallel() { + use std::thread; + + let instance_id = MyClass::init(); + let mut handles = Vec::new(); + + for _ in 0..10 { + for (f, min_increment, max_increment) in CALLS { + let handle = thread::spawn(move || unsafe { + f(instance_id).map_or((0, 0), |_| (*min_increment, *max_increment)) + }); + handles.push(handle); + } + } + + let (min_expected, max_expected) = handles + .into_iter() + .map(|handle| handle.join().unwrap()) + .reduce(|(curr_min, curr_max), (min, max)| (curr_min + min, curr_max + max)) + .unwrap(); + + unsafe { + assert!(get_int(instance_id) >= min_expected); + assert!(get_int(instance_id) <= max_expected); + } } diff --git a/godot-core/src/obj/guards.rs b/godot-core/src/obj/guards.rs index 98935e8a7..194790260 100644 --- a/godot-core/src/obj/guards.rs +++ b/godot-core/src/obj/guards.rs @@ -5,13 +5,12 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +use godot_cell::{InaccessibleGuard, MutGuard, RefGuard}; use godot_ffi::out; use std::fmt::Debug; use std::ops::{Deref, DerefMut}; -use crate::storage::{BaseMutGuard, MutGuard, RefGuard}; - use super::{Gd, GodotClass}; /// Immutably/shared bound reference guard for a [`Gd`][crate::obj::Gd] smart pointer. @@ -119,14 +118,14 @@ impl Deref for BaseRef<'_, T> { /// See [`WithBaseField::base_mut()`](super::WithBaseField::base_mut()) for usage. pub struct BaseMut<'a, T: GodotClass> { gd: Gd, - _base_mut_guard: BaseMutGuard<'a, T>, + _inaccessible_guard: InaccessibleGuard<'a, T>, } impl<'a, T: GodotClass> BaseMut<'a, T> { - pub(crate) fn new(gd: Gd, base_mut_guard: BaseMutGuard<'a, T>) -> Self { + pub(crate) fn new(gd: Gd, inaccessible_guard: InaccessibleGuard<'a, T>) -> Self { Self { gd, - _base_mut_guard: base_mut_guard, + _inaccessible_guard: inaccessible_guard, } } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index d880a2f3e..9977d7e2b 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -381,7 +381,7 @@ pub trait WithBaseField: GodotClass { .expect("we have a `Gd` so the raw should not be null") }; - let guard = storage.get_base_mut(self); + let guard = storage.get_inaccessible(self); BaseMut::new(base_gd, guard) } diff --git a/godot-core/src/storage.rs b/godot-core/src/storage.rs index 314470416..1b0a8802f 100644 --- a/godot-core/src/storage.rs +++ b/godot-core/src/storage.rs @@ -5,8 +5,6 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::ops::{Deref, DerefMut}; - use crate::obj::{Base, Gd, GodotClass, Inherits}; use crate::{godot_error, out}; use godot_ffi as sys; @@ -69,22 +67,6 @@ pub unsafe trait Storage { /// The type of instances stored by this storage. type Instance: GodotClass; - /// The type of guards returned when a shared reference is taken. - type RefGuard<'a>: Deref - where - Self: 'a; - - /// The type of guards returned when a mutable reference is taken. - type MutGuard<'a>: Deref + DerefMut - where - Self: 'a; - - /// The type of guards returned when we know that a mutable reference can be safely ignored for purposes - /// of taking mutable references. - type BaseMutGuard<'a> - where - Self: 'a; - /// Constructs a new storage for an instance binding referencing `user_instance`. fn construct( user_instance: Self::Instance, @@ -101,22 +83,22 @@ pub unsafe trait Storage { /// /// This will ensure Rust's rules surrounding references are upheld. Possibly panicking at runtime if /// they are violated. - fn get(&self) -> Self::RefGuard<'_>; + fn get(&self) -> godot_cell::RefGuard<'_, Self::Instance>; /// Returns a mutable/exclusive reference to this storage's instance. /// /// This will ensure Rust's rules surrounding references are upheld. Possibly panicking at runtime if /// they are violated. - fn get_mut(&self) -> Self::MutGuard<'_>; + fn get_mut(&self) -> godot_cell::MutGuard<'_, Self::Instance>; /// Returns a guard that allows calling methods on `Gd` that take `&mut self`. /// /// This can use the provided `instance` to provide extra safety guarantees such as allowing reentrant /// code to create new mutable references. - fn get_base_mut<'a: 'b, 'b>( + fn get_inaccessible<'a: 'b, 'b>( &'a self, instance: &'b mut Self::Instance, - ) -> Self::BaseMutGuard<'b>; + ) -> godot_cell::InaccessibleGuard<'b, Self::Instance>; /// Returns whether this storage is currently alive or being destroyed. /// @@ -189,10 +171,6 @@ pub type InstanceStorage = single_threaded::InstanceStorage; #[cfg(feature = "experimental-threads")] pub type InstanceStorage = multi_threaded::InstanceStorage; -pub type RefGuard<'a, T> = as Storage>::RefGuard<'a>; -pub type MutGuard<'a, T> = as Storage>::MutGuard<'a>; -pub type BaseMutGuard<'a, T> = as Storage>::BaseMutGuard<'a>; - const fn _assert_implements_storage() {} const _INSTANCE_STORAGE_IMPLEMENTS_STORAGE: () = diff --git a/godot-core/src/storage/multi_threaded.rs b/godot-core/src/storage/multi_threaded.rs index 480788e86..7ede90a84 100644 --- a/godot-core/src/storage/multi_threaded.rs +++ b/godot-core/src/storage/multi_threaded.rs @@ -34,12 +34,6 @@ pub struct InstanceStorage { unsafe impl Storage for InstanceStorage { type Instance = T; - type RefGuard<'a> = godot_cell::RefGuard<'a, T>; - - type MutGuard<'a> = godot_cell::MutGuard<'a, T>; - - type BaseMutGuard<'a> = godot_cell::InaccessibleGuard<'a, T>; - fn construct( user_instance: Self::Instance, base: Base<::Base>, @@ -61,7 +55,7 @@ unsafe impl Storage for InstanceStorage { &self.base } - fn get(&self) -> Self::RefGuard<'_> { + fn get(&self) -> godot_cell::RefGuard<'_, T> { self.user_instance.as_ref().borrow().unwrap_or_else(|err| { panic!( "\ @@ -74,7 +68,7 @@ unsafe impl Storage for InstanceStorage { }) } - fn get_mut(&self) -> Self::MutGuard<'_> { + fn get_mut(&self) -> godot_cell::MutGuard<'_, T> { self.user_instance .as_ref() .borrow_mut() @@ -90,7 +84,10 @@ unsafe impl Storage for InstanceStorage { }) } - fn get_base_mut<'a: 'b, 'b>(&'a self, value: &'b mut Self::Instance) -> Self::BaseMutGuard<'b> { + fn get_inaccessible<'a: 'b, 'b>( + &'a self, + value: &'b mut Self::Instance, + ) -> godot_cell::InaccessibleGuard<'b, T> { self.user_instance .as_ref() .make_inaccessible(value) diff --git a/godot-core/src/storage/single_threaded.rs b/godot-core/src/storage/single_threaded.rs index 329578def..3e2a391cc 100644 --- a/godot-core/src/storage/single_threaded.rs +++ b/godot-core/src/storage/single_threaded.rs @@ -34,12 +34,6 @@ pub struct InstanceStorage { unsafe impl Storage for InstanceStorage { type Instance = T; - type RefGuard<'a> = godot_cell::RefGuard<'a, T>; - - type MutGuard<'a> = godot_cell::MutGuard<'a, T>; - - type BaseMutGuard<'a> = godot_cell::InaccessibleGuard<'a, T>; - fn construct( user_instance: Self::Instance, base: Base<::Base>, @@ -61,7 +55,7 @@ unsafe impl Storage for InstanceStorage { &self.base } - fn get(&self) -> Self::RefGuard<'_> { + fn get(&self) -> godot_cell::RefGuard<'_, T> { self.user_instance.as_ref().borrow().unwrap_or_else(|err| { panic!( "\ @@ -74,7 +68,7 @@ unsafe impl Storage for InstanceStorage { }) } - fn get_mut(&self) -> Self::MutGuard<'_> { + fn get_mut(&self) -> godot_cell::MutGuard<'_, T> { self.user_instance .as_ref() .borrow_mut() @@ -90,7 +84,10 @@ unsafe impl Storage for InstanceStorage { }) } - fn get_base_mut<'a: 'b, 'b>(&'a self, value: &'b mut Self::Instance) -> Self::BaseMutGuard<'b> { + fn get_inaccessible<'a: 'b, 'b>( + &'a self, + value: &'b mut Self::Instance, + ) -> godot_cell::InaccessibleGuard<'b, T> { self.user_instance .as_ref() .make_inaccessible(value)