Fix generation race condition with sync storage (#2638)

* Fix generation race condition with sync storage

* Document GenerationalPointer

* check generation before recycling generational box

* fix clippy

* make race_condition_regression test panic if it fails

packages/generational-box/tests/basic.rs

@@ -1,4 +1,4 @@
-use generational_box::{AnyStorage, GenerationalBox, UnsyncStorage};
+use generational_box::{GenerationalBox, Storage, SyncStorage, UnsyncStorage};
 
 /// # Example
 ///
@@ -14,88 +14,119 @@ fn compile_fail() {}
 
 #[test]
 fn reused() {
-    let first_ptr;
-    {
-        let owner = UnsyncStorage::owner();
-        first_ptr = owner.insert(1).raw_ptr();
-        drop(owner);
-    }
-    {
-        let owner = UnsyncStorage::owner();
-        let second_ptr = owner.insert(1234).raw_ptr();
-        assert_eq!(first_ptr, second_ptr);
-        drop(owner);
+    fn reused_test<S: Storage<i32>>() {
+        let first_ptr;
+        {
+            let owner = S::owner();
+            first_ptr = owner.insert(1).raw_ptr();
+            drop(owner);
+        }
+        {
+            let owner = S::owner();
+            let second_ptr = owner.insert(1234).raw_ptr();
+            assert_eq!(first_ptr, second_ptr);
+            drop(owner);
+        }
     }
+
+    reused_test::<UnsyncStorage>();
+    reused_test::<SyncStorage>();
 }
 
 #[test]
 fn leaking_is_ok() {
-    let data = String::from("hello world");
-    let key;
-    {
-        // create an owner
-        let owner = UnsyncStorage::owner();
-        // insert data into the store
-        key = owner.insert(data);
-        // don't drop the owner
-        std::mem::forget(owner);
+    fn leaking_is_ok_test<S: Storage<String>>() {
+        let data = String::from("hello world");
+        let key;
+        {
+            // create an owner
+            let owner = S::owner();
+            // insert data into the store
+            key = owner.insert(data);
+            // don't drop the owner
+            std::mem::forget(owner);
+        }
+        assert_eq!(
+            key.try_read().as_deref().unwrap(),
+            &"hello world".to_string()
+        );
     }
-    assert_eq!(
-        key.try_read().as_deref().unwrap(),
-        &"hello world".to_string()
-    );
+
+    leaking_is_ok_test::<UnsyncStorage>();
+    leaking_is_ok_test::<SyncStorage>();
 }
 
 #[test]
 fn drops() {
-    let data = String::from("hello world");
-    let key;
-    {
-        // create an owner
-        let owner = UnsyncStorage::owner();
-        // insert data into the store
-        key = owner.insert(data);
-        // drop the owner
+    fn drops_test<S: Storage<String>>() {
+        let data = String::from("hello world");
+        let key;
+        {
+            // create an owner
+            let owner = S::owner();
+            // insert data into the store
+            key = owner.insert(data);
+            // drop the owner
+        }
+        assert!(key.try_read().is_err());
     }
-    assert!(key.try_read().is_err());
+
+    drops_test::<UnsyncStorage>();
+    drops_test::<SyncStorage>();
 }
 
 #[test]
 fn works() {
-    let owner = UnsyncStorage::owner();
-    let key = owner.insert(1);
+    fn works_test<S: Storage<i32>>() {
+        let owner = S::owner();
+        let key = owner.insert(1);
 
-    assert_eq!(*key.read(), 1);
+        assert_eq!(*key.read(), 1);
+    }
+
+    works_test::<UnsyncStorage>();
+    works_test::<SyncStorage>();
 }
 
 #[test]
 fn insert_while_reading() {
-    let owner = UnsyncStorage::owner();
-    let key;
-    {
-        let data: String = "hello world".to_string();
-        key = owner.insert(data);
+    fn insert_while_reading_test<S: Storage<String> + Storage<&'static i32>>() {
+        let owner = S::owner();
+        let key;
+        {
+            let data: String = "hello world".to_string();
+            key = owner.insert(data);
+        }
+        let value = key.read();
+        owner.insert(&1);
+        assert_eq!(*value, "hello world");
     }
-    let value = key.read();
-    owner.insert(&1);
-    assert_eq!(*value, "hello world");
+
+    insert_while_reading_test::<UnsyncStorage>();
+    insert_while_reading_test::<SyncStorage>();
 }
 
 #[test]
 #[should_panic]
 fn panics() {
-    let owner = UnsyncStorage::owner();
-    let key = owner.insert(1);
-    drop(owner);
+    fn panics_test<S: Storage<i32>>() {
+        let owner = S::owner();
 
-    assert_eq!(*key.read(), 1);
+        let key = owner.insert(1);
+        drop(owner);
+
+        assert_eq!(*key.read(), 1);
+    }
+
+    panics_test::<UnsyncStorage>();
+    panics_test::<SyncStorage>();
 }
 
 #[test]
 fn fuzz() {
-    fn maybe_owner_scope(
-        valid_keys: &mut Vec<GenerationalBox<String>>,
-        invalid_keys: &mut Vec<GenerationalBox<String>>,
+    fn maybe_owner_scope<S: Storage<String>>(
+        valid_keys: &mut Vec<GenerationalBox<String, S>>,
+        invalid_keys: &mut Vec<GenerationalBox<String, S>>,
         path: &mut Vec<u8>,
     ) {
         let branch_cutoff = 5;
@@ -106,28 +137,36 @@ fn fuzz() {
         };
 
         for i in 0..children {
-            let owner = UnsyncStorage::owner();
+            let owner = S::owner();
             let key = owner.insert(format!("hello world {path:?}"));
+            println!("created new box {key:?}");
             valid_keys.push(key);
             path.push(i);
             // read all keys
             println!("{:?}", path);
             for key in valid_keys.iter() {
+                println!("reading {key:?}");
                 let value = key.read();
                 println!("{:?}", &*value);
                 assert!(value.starts_with("hello world"));
             }
-            #[cfg(any(debug_assertions, feature = "check_generation"))]
             for key in invalid_keys.iter() {
+                println!("reading invalid {key:?}");
                 assert!(key.try_read().is_err());
             }
             maybe_owner_scope(valid_keys, invalid_keys, path);
-            invalid_keys.push(valid_keys.pop().unwrap());
+            let invalid = valid_keys.pop().unwrap();
+            println!("popping {invalid:?}");
+            invalid_keys.push(invalid);
             path.pop();
         }
     }
 
     for _ in 0..10 {
-        maybe_owner_scope(&mut Vec::new(), &mut Vec::new(), &mut Vec::new());
+        maybe_owner_scope::<UnsyncStorage>(&mut Vec::new(), &mut Vec::new(), &mut Vec::new());
+    }
+
+    for _ in 0..10 {
+        maybe_owner_scope::<SyncStorage>(&mut Vec::new(), &mut Vec::new(), &mut Vec::new());
     }
 }

packages/generational-box/tests/errors.rs

@@ -1,38 +1,33 @@
-use std::cell::{Ref, RefMut};
-
 use generational_box::{
-    AlreadyBorrowedError, AlreadyBorrowedMutError, AnyStorage, BorrowError, BorrowMutError,
-    GenerationalBox, GenerationalRef, GenerationalRefMut, Owner, UnsyncStorage, ValueDroppedError,
+    AlreadyBorrowedError, AlreadyBorrowedMutError, BorrowError, BorrowMutError, GenerationalBox,
+    Owner, Storage, SyncStorage, UnsyncStorage, ValueDroppedError,
 };
 
 #[track_caller]
-fn read_at_location(
-    value: GenerationalBox<i32>,
-) -> (
-    GenerationalRef<Ref<'static, i32>>,
-    &'static std::panic::Location<'static>,
-) {
+fn read_at_location<S: Storage<i32>>(
+    value: GenerationalBox<i32, S>,
+) -> (S::Ref<'static, i32>, &'static std::panic::Location<'static>) {
     let location = std::panic::Location::caller();
     let read = value.read();
     (read, location)
 }
 
 #[track_caller]
-fn write_at_location(
-    value: GenerationalBox<i32>,
-) -> (
-    GenerationalRefMut<RefMut<'static, i32>>,
-    &'static std::panic::Location<'static>,
-) {
+fn write_at_location<S: Storage<i32>>(
+    value: GenerationalBox<i32, S>,
+) -> (S::Mut<'static, i32>, &'static std::panic::Location<'static>) {
     let location = std::panic::Location::caller();
     let write = value.write();
     (write, location)
 }
 
 #[track_caller]
-fn create_at_location(
-    owner: &Owner,
-) -> (GenerationalBox<i32>, &'static std::panic::Location<'static>) {
+fn create_at_location<S: Storage<i32>>(
+    owner: &Owner<S>,
+) -> (
+    GenerationalBox<i32, S>,
+    &'static std::panic::Location<'static>,
+) {
     let location = std::panic::Location::caller();
     let value = owner.insert(1);
     (value, location)
@@ -40,65 +35,85 @@ fn create_at_location(
 
 #[test]
 fn read_while_writing_error() {
-    let owner = UnsyncStorage::owner();
-    let value = owner.insert(1);
+    fn read_while_writing_error_test<S: Storage<i32>>() {
+        let owner = S::owner();
+        let value = owner.insert(1);
 
-    let (write, location) = write_at_location(value);
+        let (write, location) = write_at_location(value);
 
-    assert_eq!(
-        value.try_read().err(),
-        Some(BorrowError::AlreadyBorrowedMut(
-            AlreadyBorrowedMutError::new(location)
-        ))
-    );
-    drop(write);
+        assert_eq!(
+            value.try_read().err(),
+            Some(BorrowError::AlreadyBorrowedMut(
+                AlreadyBorrowedMutError::new(location)
+            ))
+        );
+        drop(write);
+    }
+
+    // For sync storage this will deadlock
+    read_while_writing_error_test::<UnsyncStorage>();
 }
 
 #[test]
 fn read_after_dropped_error() {
-    let owner = UnsyncStorage::owner();
-    let (value, location) = create_at_location(&owner);
-    drop(owner);
-    assert_eq!(
-        value.try_read().err(),
-        Some(BorrowError::Dropped(ValueDroppedError::new(location)))
-    );
+    fn read_after_dropped_error_test<S: Storage<i32>>() {
+        let owner = S::owner();
+        let (value, location) = create_at_location(&owner);
+        drop(owner);
+        assert_eq!(
+            value.try_read().err(),
+            Some(BorrowError::Dropped(ValueDroppedError::new(location)))
+        );
+    }
+
+    read_after_dropped_error_test::<UnsyncStorage>();
+    read_after_dropped_error_test::<SyncStorage>();
 }
 
 #[test]
 fn write_while_writing_error() {
-    let owner = UnsyncStorage::owner();
-    let value = owner.insert(1);
+    fn write_while_writing_error_test<S: Storage<i32>>() {
+        let owner = S::owner();
+        let value = owner.insert(1);
 
-    #[allow(unused)]
-    let (write, location) = write_at_location(value);
+        #[allow(unused)]
+        let (write, location) = write_at_location(value);
 
-    let write_result = value.try_write();
-    assert!(write_result.is_err());
-    #[cfg(debug_assertions)]
-    assert_eq!(
-        write_result.err(),
-        Some(BorrowMutError::AlreadyBorrowedMut(
-            AlreadyBorrowedMutError::new(location)
-        ))
-    );
+        let write_result = value.try_write();
+        assert!(write_result.is_err());
+        #[cfg(any(debug_assertions, feature = "debug_borrows"))]
+        assert_eq!(
+            write_result.err(),
+            Some(BorrowMutError::AlreadyBorrowedMut(
+                AlreadyBorrowedMutError::new(location)
+            ))
+        );
 
-    drop(write);
+        drop(write);
+    }
+
+    // For sync storage this will deadlock
+    write_while_writing_error_test::<UnsyncStorage>();
 }
 
 #[test]
 fn write_while_reading_error() {
-    let owner = UnsyncStorage::owner();
-    let value = owner.insert(1);
+    fn write_while_reading_error_test<S: Storage<i32>>() {
+        let owner = S::owner();
+        let value = owner.insert(1);
 
-    let (read, location) = read_at_location(value);
+        let (read, location) = read_at_location(value);
 
-    assert_eq!(
-        value.try_write().err(),
-        Some(BorrowMutError::AlreadyBorrowed(AlreadyBorrowedError::new(
-            vec![location]
-        )))
-    );
+        assert_eq!(
+            value.try_write().err(),
+            Some(BorrowMutError::AlreadyBorrowed(AlreadyBorrowedError::new(
+                vec![location]
+            )))
+        );
 
-    drop(read);
+        drop(read);
+    }
+
+    // For sync storage this will deadlock
+    write_while_reading_error_test::<UnsyncStorage>();
 }

packages/generational-box/tests/sync.rs

@@ -0,0 +1,42 @@
+// Regression test for https://github.com/DioxusLabs/dioxus/issues/2636
+
+use std::time::Duration;
+
+use generational_box::{AnyStorage, GenerationalBox, SyncStorage};
+
+#[test]
+fn race_condition_regression() {
+    for _ in 0..100 {
+        let handle = {
+            let owner = SyncStorage::owner();
+            let key = owner.insert(1u64);
+            let handle = std::thread::spawn(move || reader(key));
+
+            std::thread::sleep(Duration::from_millis(10));
+            handle
+            // owner is dropped now
+        };
+        // owner is *recycled*
+        let owner = SyncStorage::owner();
+        let _key = owner.insert(2u64);
+        let _ = handle.join();
+    }
+}
+
+fn reader(key: GenerationalBox<u64, SyncStorage>) {
+    for _ in 0..1000000 {
+        match key.try_read() {
+            Ok(value) => {
+                if *value == 2 {
+                    panic!("Read a new value with the old generation");
+                } else {
+                    // fine
+                }
+            }
+            Err(err) => {
+                eprintln!("bailing out - {err:?}");
+                break;
+            }
+        }
+    }
+}