Fix stores deadlock

[ealmloff]

We can't hold the stores subscription lock while calling mark_dirty because it runs arbitrary code which may try to add subscriptions. This PR fixes the issue by collecting a list of paths in the store that need to be marked as dirty first before marking each one as dirty without holding the lock over the mark_dirty call

Fixes #4935

[alextechcc]

So fast! Thank you for looking into this.

I still seem to be able to deadlock - working on a minimal reproduction at the moment and a trace. Happens in my bevy/dioxus sync library I am working on.

EDIT: This may have been a false positive. Going to let my app run for a while.

[alextechcc]

I've found a probably separate but somewhat related issue where this unwrap fails when iterating over the list:
https://github.com/ealmloff/dioxus/blob/fix-store-deadlock/packages/stores/src/impls/hashmap.rs#L86

Crash Log:
crash.txt

I can make another issue if necessary.

This one is hard to replicate. So, I've put my test here:
https://github.com/alextechcc/bevy_store_deadlock

It crashes faster if I type in the input fields, but crashes eventually naturally.

The gist is

1. A thread is randomly adding, mutating, and removing hashmap store entries.
2. A dioxus thread is rendering the hashmap by iterating over it.

This results in a crash where I believe what is happening is that there isn't a lock held preventing removal of elements when this block is running:

        keys.into_iter().map(move |key| {
            let value = self.clone().get(key.clone()).unwrap();
            (key, value)
        })

So I think while the map is running, key no longer points to a valid entry, and unwrap fails.

[ealmloff]

That specific issue should be fixed, but your reproduction still panics. You can't easily use fail-able mappings like Store<Hashmap>::get in multithreaded settings since the other thread could take the value out from under you after the item is removed

[alextechcc]

I guess kinda the point of the design of stores is being able to grab handles to substructures specifically without moving ownership or holding a lock - so it's a natural consequence that the value may disappear under you while iterating. And if you could grab a lock on the structure and pass it around (to prevent the value disappearing) - then any callbacks defined would just prevent mutation of whatever they're grabbing a handle to until they go away - so a lock wouldn't work.

Maybe I'm missing something here. I can't do this:

        for (_entity , mut name) in store.iter() {
            input {
                value: format!(
                    "{}",
                    name.try_read().map_or(String::new(), |name| name.as_str().to_string()),
                ),
            }
        }

I think it'd be more ergonomic to allow iteration in sync, so long as you handle the error when you read the value. Otherwise it feels a bit difficult for someone new to Rust to figure out that you basically have to iterate over keys and the values are not safe to touch.

Does it make sense to do something like this change for GetWrite in HashMap/btreemap?

@@ -316,7 +316,7 @@ where
             Self::Storage::map(value, |value: &Write::Target| {
                 value
                     .get(&self.index)
-                    .expect("Tried to access a key that does not exist")
+                    .unwrap_or(BorrowError::Dropped(self.index))
             })
         })
     }

Also, not to expand on the scope much, but I believe the unwrap is also here:
https://github.com/ealmloff/dioxus/blob/fix-store-deadlock/packages/stores/src/impls/btreemap.rs#L80

[ealmloff]

I think it'd be more ergonomic to allow iteration in sync, so long as you handle the error when you read the value. Otherwise it feels a bit difficult for someone new to Rust to figure out that you basically have to iterate over keys and the values are not safe to touch.

I think this is reasonable behavior to expect when reading an index that no longer exists and both provides a better error message and makes some iteration more ergonomic.

However, in general iteration with sync stores is still fraught as items could also be added after the iterator is created.

[alextechcc]

I think it'd be more ergonomic to allow iteration in sync, so long as you handle the error when you read the value. Otherwise it feels a bit difficult for someone new to Rust to figure out that you basically have to iterate over keys and the values are not safe to touch.

I think this is reasonable behavior to expect when reading an index that no longer exists and both provides a better error message and makes some iteration more ergonomic.

However, in general iteration with sync stores is still fraught as items could also be added after the iterator is created.

So I guess in this scenario - you may get in a state where the component is not rerendered despite a new hashmap entry being added? I guess Dioxus isn't really set up for this - where dirtyness can change while being rendered. I feel like this is a trap for Dioxus users and maybe sync stores should not be available until it's addressed.

_The error message is good - but what I was suggesting above was to make it so you're iterating over a Result type where you have to explicitly handle the fact that the key could've disappeared - instead of it panicking without you being able to handle it. If the error happens rarely - it could lead to a rare crash in a production application - rather than a user just handling the issue explicitly.

As it stands - sync hashmaps are a bit odd of an API surface - if you want to iterate, you have to use .iter()- throw out the value (because it can panic if you use it), then use .get() with the key - and there is no .keys() iterator, just a `.values()._
EDIT: I see the new changes now