dioxus icon indicating copy to clipboard operation
dioxus copied to clipboard

`use_reactive` causes a false positive for "write on signal" warning

Open joseph-gio opened this issue 1 year ago • 8 comments

Problem

When writing to a signal in the same scope that you read it from, a warning is printed (added in https://github.com/DioxusLabs/dioxus/pull/2277). There appears to be a false positive for this warning, as it seems to update any time a dependency from use_reactive changes

Steps To Reproduce

Steps to reproduce the behavior:

    let is_hoverd: bool = ...;
    let color = use_memo(use_reactive!(|is_hovered| if is_hovered {
        "red"
    } else {
        "blue"
    }));

Expected behavior

When the is_hovered value changes, the color memo should update properly.

Actual behavior

The memo updates, but it prints this warning despite not writing to any signals:

Write on signal at dioxus-1e619ce595d3799d/74352f2/packages/signals/src/memo.rs:98:18 finished in ReactiveContext(for scope: ScopeId(0, "app")) which is also subscribed to the signal. This will likely cause an infinite loop. When the write finishes, ReactiveContext(for scope: ScopeId(0, "app")) will rerun which may cause the write to be rerun again.
HINT:
This issue is caused by reading and writing to the same signal in a reactive scope. Components, effects, memos, and resources each have their own a reactive scopes. Reactive scopes rerun when any signal you read inside of them are changed. If you read and write to the same signal in the same scope, the write will cause the scope to rerun and trigger the write again. This can cause an infinite loop.

More details

I did a bit of digging through the code for use_memo and use_reactive. I could be wrong, but the code for use_callback looked a little suspicious. I think it might be running the code for use_reactive in the incorrect scope, leading to this warning (and potentially incorrect behavior). If my theory is correct, then this bug may be responsible for infinite loops that have been causing crashes for us.

Environment:

  • Dioxus version: https://github.com/DioxusLabs/dioxus/commit/74352f2f615c1be2081c9255860d9127feee92d4
  • Rust version: rustc 1.79.0
  • OS info: Ubuntu
  • App platform: desktop

Questionnaire

  • [x] I'm interested in fixing this myself but don't know where to start
  • [ ] I would like to fix and I have a solution
  • [ ] I don't have time to fix this right now, but maybe later

joseph-gio avatar Jun 21 '24 02:06 joseph-gio

See also https://github.com/DioxusLabs/dioxus/issues/2499, which appears to be related

joseph-gio avatar Jun 21 '24 02:06 joseph-gio

Hey @ealmloff, not sure if the 'tweak' label is right here, since this issue might be a symptom of a deeper correctness issue with scopes. Just wanna make this this gets prioritized correctly :)

joseph-gio avatar Jun 21 '24 16:06 joseph-gio

We break some of our own rules internally in a way that is fine, but can be easy to get wrong. use_reactive should both write to and subscribe to a signal in the component is is run in. Reading and writing in the same scope is usually a bug, but in this case we are careful to not write unconditionally to avoid infinite loops. I think the right fix here is to introduce something like allow_same_scope_read_write_signals(|| /*run fishy code here*/) to disable the warning for some of the built in hooks

ealmloff avatar Jun 21 '24 17:06 ealmloff

Unless I'm misunderstanding something, use_reactive actually shouldn't break that rule. The signal gets written to in the scope where the hook gets called, but it should only get read within the reactive scope of the memo it gets passed to.

Right here, the signal is written to during the call to use_reactive, which occurs in the top-level scope for the component using the hook.

if !first_run && non_reactive_data.changed(&*last_state.peek()) {
    last_state.set(non_reactive_data.out());
}
move || closure(last_state())

It only gets read from in the closure that gets returned, which is immediately passed to use_memo. So if I'm understanding correctly, it's only the memo's reactive scope that should be subscribed to the signal, indicating that something is going wrong with the scopes

joseph-gio avatar Jun 21 '24 17:06 joseph-gio

Oh, that is correct. use_reactive shouldn't be triggering that warning. Can you share the code that is triggering the issue? This code doesn't trigger the warning when you click the button:

use dioxus::prelude::*;

fn main() {
    tracing_subscriber::fmt::init();

    launch(|| {
        let mut state = use_signal(|| false);
        let current_state = state();
        let color = use_memo(use_reactive!(|current_state| if current_state {
            "red"
        } else {
            "blue"
        }));
        
        rsx! {
            div {
                "{color}"
            }
            button {
                onclick: move |_| {
                    state.toggle();
                },
                "Toggle"
            }
        }
    })
}

But this code (which is actually reading and writing from the memo in the same scope in a problematic way) does trigger the warning:

use dioxus::prelude::*;

fn main() {
    tracing_subscriber::fmt::init();

    launch(|| {
        let mut state = use_signal(|| false);
        let current_state = state();
        let color = use_memo(use_reactive!(|current_state| if current_state {
            "red"
        } else {
            "blue"
        }));

        if generation() == 0 {
            state.toggle();
        }

        rsx! {
            div {
                "{color}"
            }
            button {
                onclick: move |_| {
                    state.toggle();
                },
                "Toggle"
            }
        }
    })
}

ealmloff avatar Jun 21 '24 18:06 ealmloff

I didn't save the full example I used to reproduce it but it was basically identical to your first example, only it was in a #[component] instead of using launch. I can remake the example though

I actually don't follow why the memo usage is problematic in the second example. Calling state.toggle() at the top level should definitely trigger the write-on-signal warning for the state signal, but the signal owned by use_reactive shouldn't have any incorrect reads or writes (at least not directly within the same scope)

joseph-gio avatar Jun 21 '24 19:06 joseph-gio

Here's a full example

#![allow(non_snake_case)]

use bevy::prelude::*;
use bevy_dioxus_lib::prelude::*;

#[component]
fn app() -> Element {
    let mut button_down = use_signal(|| false);

    let is_button_down = button_down();
    let color = use_memo(use_reactive!(|is_button_down| if is_button_down {
        "red"
    } else {
        "blue"
    }));

    rsx! {
        flex {
            flex_grow: 1,
            align_items: "center",
            justify_content: "center",
            flex {
                width: 100,
                height: 100,
                color,
                border: 10,
                border_color: "rgba(100%, 100%, 100%, 0.5)",
                onmousedown: move |_| {
                    button_down.set(true);
                },
                onmouseup: move |_| {
                    button_down.set(false);
                }
            }
        }

    }
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
}

fn main() {
    App::new()
        .add_plugins((DefaultPlugins, BevyDioxusPlugin(app)))
        .add_systems(Startup, setup)
        .run();
}

This is using our currently-proprietary bevy integration of dioxus. If needed I can make an example targeting dioxus a built-in dioxus backend but I wouldn't expect this to affect the behavior of the virtual dom's scopes in this way. I had some build script issues when trying to run the built-in dioxus examples so I just used our integration since it's easier

joseph-gio avatar Jun 21 '24 19:06 joseph-gio

Is it possible that the usage of rt.on_scope within use_callback is overriding the reactive scope of the memo? https://github.com/DioxusLabs/dioxus/blob/5ae7fd65ee2c0c01e296d09e2583a9c7031b7e10/packages/hooks/src/use_callback.rs#L30

If this is true I'm not quite sure why memos or effects would work at all, but that's the only potential cause that stands out to me right now

joseph-gio avatar Jun 21 '24 20:06 joseph-gio