BorrowMut error when using signal immutably in `<Show/>` and mutably in `on:click`.

[adrianncovaci]

Describe the bug A clear and concise description of what the bug is. I'm not really sure if leptos should handle this or not, but Im trying to use a signal to both <Show/> and handle an on:click event and get a borrowmut error. I think it's because the effect in show gets an immutable reference while there's still a mutable one for the on:click handler. Leptos Dependencies

Please copy and paste the Leptos dependencies and features from your Cargo.toml.

[workspace.dependencies]
leptos = { version = "0.5", features = ["nightly"] }
leptos_meta = { version = "0.5", features = ["nightly"] }
leptos_router = { version = "0.5", features = ["nightly"] }
leptos_axum = { version = "0.5" }

To Reproduce Steps to reproduce the behavior: Run (or inspect) https://github.com/adrianncovaci/leptos_reproduce and click on one of the divs.

Screenshots If applicable, add screenshots to help explain your problem.

[gbj]

I can't fully follow the logic here but here's a version of your code that doesn't panic

    pub fn toggle_topic(&mut self, data_info: &'static str) {
        self.chart_panels.with(|chart| {
            if let Some(chart_panel) = chart.last() {
                let new_topic_buffer = TopicBuffer {
                    identifier: data_info,
                };
                chart_panel
                    .topic_buffers
                    .update(|topic_buffers| topic_buffers.push(new_topic_buffer));
            }
        });
    }

rather than the nested .update() given you mutable access to self.chart_panels that you aren't using, this uses .with() to give you immutable access to self.chart_panels.

[svieujot]

Thank you for your suggestion.

Since it is difficult to guarantee that in a large application to guarantee that such cases do no repeat (and actually we already spent a number of days on such errors, we are trying to find ways to have the above test code either does not compile or does not panic in any circumstance.

With my limited understanding of Leptos, I believe the issue might come from the fact that in NodeId::update, we have:

            // notify subscribers
            if updated.is_some() {
                // mark descendants dirty
                runtime.mark_dirty(*self);

                runtime.run_effects();
            }

See here

So I understand that the effects are run while still in the update function, and if the effect trigger a borrow, we have an error.

Could the runtime.run_effects(); instead be scheduled to run once the update is completed, or after the unwrap_or_default() on line 2174 ?

[svieujot]

For some context, in our case, we have a lot of signals that are updated by a websocket (and not by the user).

So the structure of our application has a lot of structs that contains other structs that contain signals.

In this specific case, we have a page that displays a list of configurable graphs. So we have a top struct that contains a Vec of ChartPanels and each Chart contains a Vec of Topics. Each Topic has a name, a signal to get the last value and a Vec of the previous values to plot the values over time.

So to add a topic to a ChartPanel, we have a method ChartPanel::add_topic(&mut self, ...). The code fix you suggest means that we can not have such method using a &mut self.

Should we ban struct holding Signals ?

[gbj]

So I understand that the effects are run while still in the update function, and if the effect trigger a borrow, we have an error.

Effects are run after the update function that triggers them, but because you have two nested .update() functions they run after the inner function, which is during the outer function.

You'll notice that run_effects in the code you quote above first checks for batching https://github.com/leptos-rs/leptos/blob/8e374efe8d7a6739a8dcdcd30850246ee24e0283/leptos_reactive/src/runtime.rs#L483-L490

So, here's another version of your nested .update() code that also compiles and runs with no error, by using batch()

pub fn toggle_topic(&mut self, data_info: &'static str) {
        batch(|| {
            self.chart_panels.update(|chart| {
                if let Some(chart_panel) = chart.last_mut() {
                    let new_topic_buffer = TopicBuffer {
                        identifier: data_info,
                    };
                    chart_panel
                        .topic_buffers
                        .update(|topic_buffers| topic_buffers.push(new_topic_buffer));
                }
            })
        });
    }

Should we ban struct holding Signals ?

I don't think you should ban structs holding signals. I do think that if you're going to nest .update() calls on different signals like this, and triggering one of them might trigger an immutable read on the other, you should batch them.

[svieujot]

Thank you very much for the solution.

Would it be possible to detect such nested update() and switch to batching automatically ?

[gbj]

Sure, I could add a debug-mode-only flag that is set whenever you enter an .update() and then warns if it's already set -- that should catch nested updates and tell you to batch. But I won't add a flag that automatically upgrades to batching, because that would create an unnecessary runtime drag on every .update() call.

[svieujot]

Yes such a flag would be really useful.

I think a feature would be better than a debug-mode-only flag as in our case we have to run the application in release mode as otherwise we get WASM overflow error due to large struct sizes and too many variables.

[gbj]

Effects batch automatically in 0.7, so I'm closing this one for now.

[svieujot]

Thank you !