Intermediate memo does not recompute

[stefnotch]

Describe the bug
I have set up a reactive graph with "source => memo 1 => memo 2" and "memo1, memo2 => effect". When I change the source, then the first memo gets recomputed, the effect gets updated, but the second memo stays the same.

use any_spawner::Executor;
use reactive_graph::{
    computed::Memo,
    effect::Effect,
    owner::Owner,
    signal::RwSignal,
    traits::{Get, Read, Set},
};

fn main() {
    Executor::init_futures_executor().unwrap();
    let _owner = Owner::new();
    let source = RwSignal::new(0);

    let directly_derived = Memo::new_with_compare(move |_| source.get(), |_, _| true);
    let indirect = Memo::new_with_compare(move |_| directly_derived.get(), |_, _| true);

    Effect::new(move |_| {
        let direct_value = directly_derived.read(); // <!!!=== This seems to break it
        let indirect_value = indirect.get();
        println!(
            "After rerunning the effect effect {:?} ?= {:?} (memo should have been recomputed ^^^)",
            *direct_value, indirect_value
        );
    });

    Executor::poll_local();
    source.set(1);
    Executor::poll_local();
    source.set(2);
    Executor::poll_local();
}

Leptos Dependencies

[dependencies]
any_spawner = { version = "0.1.1", features = [
    "wasm-bindgen",
    "futures-executor",
] }
reactive_graph = { version = "0.1.0-rc1", features = ["effects"] }
wasm-bindgen = "0.2.95"
web-sys = { version = "0.3.72", features = ["console"] }

[patch.crates-io]
any_spawner = { git = "https://github.com/leptos-rs/leptos", branch = "main" }

To Reproduce
Steps to reproduce the behavior:

1. Run the script above
2. It prints

After rerunning the effect effect 0 ?= 0 (memo should have been recomputed ^^^)
After rerunning the effect effect 1 ?= 0 (memo should have been recomputed ^^^)
After rerunning the effect effect 2 ?= 0 (memo should have been recomputed ^^^)

Expected behavior
It should have printed

After rerunning the effect effect 0 ?= 0 (memo should have been recomputed ^^^)
After rerunning the effect effect 1 ?= 1 (memo should have been recomputed ^^^)
After rerunning the effect effect 2 ?= 2 (memo should have been recomputed ^^^)

Additional context

I also have a demo project that can run in a browser environment
leptos-testos.zip

The demo project can also be run in a browser environment by doing

npm install
npm run build

and then serving index.html with a HTTP server and opening it in a browser.
I originally thought that it was a wasm-bindgen-futures specific bug, but it turned out that I just didn't manage to properly reproduce it on desktop.

[gbj]

Thanks for the repro, I noticed this or a similar bug last week but had trouble tracking it down. This is a nice minimal example.

[gbj]

Ah you know what, this is actually just the opposite of #3158 which you opened a while ago. Now, rather than deadlocking, having a ReadLock on a memo while it needs to be updated is failing silently.

Using .get() instead of .read() fixes the issue.

I am not sure that it is possible to refactor the way memos are structured such that the value and the reactive machinery are separate from one another, because whether a memo needs to update or not depends on its value. So taking a read lock on a memo, and holding that during other reactive operations, is quite tricky.

Any suggestions? Perhaps the answer is just a better answer to errors here, like a console warning instead of either panicking or failing silently?

[stefnotch]

An error or a warning would be a very welcome improvement indeed. 👍

As for what I'm doing:
I'm using memos with objects that cannot be cloned. For example, GPU shader code is in a signal, and a wgpu::ShaderModule is in a memo that is derived from the signal.
With the ReadGuard API I can get a reference to such an object.

In my case, I can also rework my code to use .with(|value| { do something with the value }), it's just a lot of extra nested code. Alternatively, I could wrap everything in an Arc, so that the objects are clone-able.

So there's a part of me that very much hopes for a world where the .read() API almost always works.

[stefnotch]

Another tidbit worth mentioning: I have quite a lot of memos that always recompute their value when their inputs change. And they never bother comparing the new value with the old value, because they cannot be the same.
For example
Memo::new_with_compare(move |_| createShaderModule(sourceCode.get()), |_, _| true);

[gbj]

And they never bother comparing the new value with the old value, because they cannot be the same.
For example
Memo::new_with_compare(move |_| createShaderModule(sourceCode.get()), |_, _| true);

That's fine, this is just not a memo and should be implemented differently. In fact, because of the trait-based approach in reactive_graph I'm pretty sure it's possible to create your own primitive, in userland, that does not have the same limitations you're seeing with a memo.

On the other hand, I'm not sure what purpose a memo is serving here? If the memos are defined such that they are never the same value, it's a bunch of reactive-graph overhead for no real reason, as far as I can tell.

[stefnotch]

I used a memo, because it seemed like the most straightforward way of doing "[...] will only run once per change, no matter how many times you access its value.", while also having laziness. The only memo part that I don't need is the equality check.

I suppose I could write my own primitive, although I'm not entirely sure where to start.

[stefnotch]

I started implementing my own primitive, and here are my notes so far

• Figuring out all the details of the reactive algorithm is still something that I struggle with
• I tried to follow the implementation of Memo, however
  • AnySource() cannot be constructed in userland. Is this intentional? https://docs.rs/reactive_graph/0.1.0-rc3/src/reactive_graph/computed/arc_memo.rs.html#272
  • SourceSet and SubscriberSet are not public, so I had to reimplement those https://docs.rs/reactive_graph/0.1.0-rc3/src/reactive_graph/computed/inner.rs.html#23
  • Observer::is is not public https://docs.rs/reactive_graph/0.1.0-rc3/src/reactive_graph/computed/inner.rs.html#125
  • The orphan rule means that https://docs.rs/reactive_graph/0.1.0-rc3/src/reactive_graph/computed/inner.rs.html#58 is not possible. One has to introduce a wrapper type for RwLock<MemoInner<T, S>> like struct RwComputedInner<T, S>(RwLock<ComputedInner<T, S>>) where S: Storage<T>;
    • But then Mapped::try_new(Arc::clone(&self.inner) at https://docs.rs/reactive_graph/0.1.0-rc3/src/reactive_graph/computed/arc_memo.rs.html#336 breaks, because self.inner is no longer an Arc<RwLock<...>>. At that point, it looks like one might have to resort to unsafe Rust?

[stefnotch]

Any suggestions? Perhaps the answer is just a better answer to errors here, like a console warning instead of either panicking or failing silently?

I'm going to try to refactor the memo to have a more granular lock than inner: Arc<RwLock<MemoInner<T, S>>>,. One where the state and subscribers list can be mutated independently of the locks. Idea being that https://github.com/leptos-rs/leptos/pull/3160/files should no longer need the try methods.

I do suspect that that'll be a bigger change, and that it might not fix everything. I'll have to write a few unit tests to make sure it's at least less error-prone after that change.