[0.7] Suspend inner runs twice, causing double-render of returned View about leptos HOT 13 CLOSED

AFAICT this is working as designed.

In order to register both synchronous and async Resource reads, Suspense will walk over the tree of its children once. It will poll any Suspend, but not wait for it: i.e., if there is an .await point that is not ready yet, it will not progress beyond that point.

When rendering, the Future in a Suspend is polled once to check whether it is immediately, synchronously available. If it is, then its contents are simply rendered, and if not, then the Future is spawned.

In the example you provide,

1. the "side efffect" (updating the signal) is done in the synchronous part of the Suspend, before the .await point, and
2. the server function/resource resolves synchronously on the server

As a result, the Suspend is always ready the first time it is polled on the server, so the effectful synchronous code ends up executing twice.

If the server function is actually async on the server (i.e., it is not immediately resolved when called) and the side effect is after the .await, you get the result you're anticipating

#[server]
pub async fn get_thing(thing: ()) -> Result<(), ServerFnError> {
    tokio::time::sleep(std::time::Duration::from_millis(25)).await;
    Ok(thing)
}

// ...

                Suspend::new(async move {
                    let _ = resource.await;
                    count.update_untracked(|x| *x += 1);

Another workaround here is not using Suspense, but just the Suspend (this is allowed now and doesn't have the same double-polling effect, because it doesn't need to check for synchronous resource reads).

I'm happy to try to help debug something closer to your real example with the websockets if needed. I don't think that the behavior will change here, as it's pretty fundamental to the async/sync Suspense system, but if there's an example that isn't working as intended I'm happy to fix it.

from leptos.

If the server function is actually async on the server (i.e., it is not immediately resolved when called) and the side effect is after the .await, you get the result you're anticipating

Since this was something I've observed while building the test case for #2937 I took the liberty to test this again with the additional modification to the same gist I've used as the test bed, the code after await will still be called twice under CSR as the first request will be held up by the unresolved future. I've used logging instead to more clearly illustrate the flow that the futures have taken. The relevant logging/action parts of closure that returns the Suspend::new for the component looks like this when inlined with irrelevant bits removed:

    view! {         
        <Suspense>
            {move || {
                leptos::logging::log!("inspect_view closure invoked");
                Suspend::new(async move {
                    leptos::logging::log!("inspect_view Suspend::new() called");
                    let result = res_inspect.await.map(|item| {
                        leptos::logging::log!("inspect_view res_inspect awaited");
                        view! { ... }
                    });
                    count.update_untracked(|x| *x += 1);
                    leptos::logging::log!(
                        "returning result, result.is_ok() = {}, count = {}",
                        result.is_ok(),
                        count.get(),
                    );
                    result
                })
            }}
        </Suspense>
    }                       

(Edit: The resource itself actually has two .await points, and the appropriate logging line has been added - the logs have been edited to reflect this too, just view that function at the gist for the full details.)

Where res_inspect is the resource wrapping the server function that has a delay (I actually set it to 2.5 seconds) and the following log is what gets generated on the server when navigating directly to Demo target from the Home page in CSR:

res_inspect: res_overview.await
inspect_view closure invoked
inspect_view Suspend::new() called
res_inspect: resolved res_overview.await
res_inspect: inspect_item().await  # a 2.5 second pause here
res_inspect: resolved inspect_item().await
inspect_view closure invoked
inspect_view res_inspect awaited
returning result, result.is_ok() = true, count = 1
inspect_view Suspend::new() called
inspect_view res_inspect awaited
returning result, result.is_ok() = true, count = 2

While reloading that to get the SSR rendering, we get this instead (logged onto the server's console):

res_inspect: res_overview.await
inspect_view closure invoked
inspect_view Suspend::new() called
inspect_view closure invoked
inspect_view Suspend::new() called
res_inspect: resolved res_overview.await
res_inspect: inspect_item().await
res_inspect: resolved inspect_item().await   # the same 2.5 second pause
inspect_view res_inspect awaited
returning result, result.is_ok() = true, count = 1

Plus the log related to hydration (logged to the browser's console):

inspect_view closure invoked
inspect_view Suspend::new() called
inspect_view res_inspect awaited
returning result, result.is_ok() = true, count = 1

So maybe there is something funny going on here as the SSR rendering is behaving as what OP expected and does the "if there is an .await point that is not ready yet, it will not progress beyond that point" as expected, while CSR isn't doing as per that requirement. Turns out I hadn't fully grok'd this as I was working through it, See conclusion at the end.

Edit - Moreover, I figured I should also visit this point:

Another workaround here is not using Suspense, but just the Suspend (this is allowed now and doesn't have the same double-polling effect, because it doesn't need to check for synchronous resource reads).

I did something like the following:

    let just_the_suspend = Suspend::new(async move {
        let result = res_overview.await;
        count.update_untracked(|x| *x += 1);
        leptos::logging::log!("res_overview.is_ok() = {}; count = {}", result.is_ok(), count.get());
    });

    view! {
        <h5>"<ItemInspect/>"</h5>
        {just_the_suspend}
        <Suspense>
            {inspect_view}
        </Suspense>
    }

Which does work as expected:

res_inspect: res_overview.await
res_inspect: resolved res_overview.await
res_inspect: inspect_item().await
res_overview.is_ok() = true; count = 1
res_inspect: resolved inspect_item().await

Note that wrapping the Suspend in a closure (i.e. move || Suspend::new(...)) will result in it being executed twice once more.

Now that I had all the time to work through this example again and spent a lot more time thinking about how this all works, I think I am starting to get that why the difference of behavior - the final rendering of SSR + hydration is effectively has the identical number of passes as CSR. Note that the final two lines of each of the SSR and hydrate logs are identical, and just simply done twice one after the other for CSR, which just shows how this is functioning as designed. Would be useful to document this exact behavior if this is really the case (specifically, the instances where an asynchronous function is needed but no reactivity is required that the bare Suspend::new be used, to avoid the "double running" something that should only run once, and the reactive version be structured as normal).

from leptos.

@metatoaster This behavior (running 2x on the client) is the opposite of the example in the original issue (running 2x on the server), but sounds more like the bug behavior described (creating websockets 2x).

I hadn't fully thought about this one, so thanks to you both.

What's happening here is:

• move || Suspend /* ... */ creates a RenderEffect like any other move ||
• this effect then tracks the resources that are read when you .await them, so that if the resources values changes, it will know to render again
• however, resources also notify their dependencies when they resolve, which means that this resource will actually notify the render effect when it resolves, causing the effect to re-run, which is why you get the second Suspend::new(). If you had additional components in the view, they would run, creating their effects, resources, websockets? etc.

I'll have to think a bit about how to handle this: We definitely do want that move || to subscribe to the resources it reads from, so that it re-runs when they change, but we don't want the fact that they're resolving to run it again, obviously.

from leptos.

This behavior (running 2x on the client) is the opposite of the example in the original issue

The original issue included showing the server side double-effect just because I noticed it after the fact. In the video, from my issue, you can see the count always going up twice when client-side routing.

The real issue for me was always 2x on the client (websockets only created in component on the client).

If the server function is actually async on the server (i.e., it is not immediately resolved when called) and the side effect is after the .await, you get the result you're anticipating

On the server side, this is true - the result is what I would expect. But on the client side, everything after the .await is still run twice - and I'd argue this is a bigger deal because:

1. the client side is where components are more likely to have side effects
2. all memory allocated from the first render is going to become garbage collected by WASM GC - so if the component waiting on the resource allocates a lot (mine does) this can cause some extra slowdown.

from leptos.

Yes, I just misunderstood the original issue and this is obviously bad.

from leptos.

FYI - I updated the example branch to include use_websocket from leptos-use in case you wanted to use that branch as a testing ground.

from leptos.

I'm starting to feel pretty good about this whole Suspend thing! (Famous last words)

#2959 should actually fix this issue: It creates a new reactive observer, which catches all the reactive async reads in the Suspend, and then forwards those subscriptions to the outer render effect (if any) after the Suspend's async render finishes. I did test this against metatoaster's gist and against the example in this issue, and I believe it should fix the problem without breaking reactivity.

But let me know!

I also want to take the opportunity to introduce a cancellation mechanism here, which was an existing // TODO but is fully necessary now, so that if a resource re-triggers the Suspend before it's finished loading (i.e., you have two resources and the second one you .await changes while the first is still loading) it will cancel the two instead of racing.

from leptos.

But let me know!

I will test this tonight and report back. I'm impressed by your turnaround on these issues! 🙌

from leptos.

I'll second the rapid turnaround, and I can tell that the CSR is significantly snappier!

However, I will note that in one of the use case in the application I am developing is definitely broken as the reactivity isn't being propagated like it did before. In short, I implemented portlet components that is rendered on the root component but depends on data from components at the leaf routes; it expect a context through a series of signals and resources to determine what gets rendered (e.g. components can set the context to render links for local navigational purposes). I will investigate this further to see if my implementation can be improved to restore reactivity and I will provide a MVRE should my attempt fail to resolve the issue.

from leptos.

Actually, there may be a different problem to what I described as I think it may be hydration related and the gist I've provided does demonstrate this issue. The delay of 2500 should be changed to a more reasonable 25, and I've moved the count RwSignal to the App component to better illustrate the point - I've also included the portlet demonstration with the updated version, as that also includes less ad-hoc path handling and link generation, and matches closer to the particular use case I am trying to implement.

From the Home Page, go into Demo target, then just poke around the Inspect link, the Inspecting line should change as it should. Now, refresh the page, the reactivity is broken when navigating back or using the Inspect links; this lasts until you leave that component completely by backing out enough or go Home straight away and back to visit that component which will restore reactivity. Note the browser's console logs - when the view isn't being reactive, the res_inspect: is still invoked, just that the Suspend isn't returning the result. Should also note that count does not actually increase while the Suspend isn't called after hydration.

Also, I've observed the multiple retrieval under certain conditions, which can be triggered by refreshing and navigating to certain routes. Not sure if this one is actually push-state related, but I will try to figure out the exact instructions to reproduce this. I've found the most straightforward reproduction for all the issues using the most up-to-date example.

Use the top nav bar (use the accompanied main.scss for styling to help make it easier the spot) use the Target 4, _, _ link, then Target 4, 1, _ link (or Inspect path1), then field3 (or Inspect path1/field3), then with the browser's navigational system, refresh, back, back, forward, forward. You should see get_item + inspect_item load normally upon the first back with no reactivity on the main content (but aria-current is updated for the nav portlet), nothing on next back but all the reactivity is back, a single inspect_item upon forward, with normal reactivity, and finally on forward once more the double resource access is triggered.

from leptos.

But let me know!

Branch works on my end to resolve the issues I've had.

from leptos.

@metatoaster The PR broke reactivity during the hydration of Suspend (so, refreshing on the page), which CI and you both caught. I think this covers the first issue and half of the second issue.

I would suggest opening the second half of the second issue (with the complex series of navigations) as a second issue, so that this one can be closed. I am not sure where in the complex chain of events this second server function invocation is coming from, given the routing structure, wildcard segment, it only happens when using the "forward" button but not navigating with the link, etc.

from leptos.

On the off chance that the latter half of the second problem might be related to the WIP incomplete fix, I will hold off on opening the new issue until the existing CI issues are fixed and this issue is properly closed. I've noticed something similar as you were completing the fix for #2937, where a similar issue was happening due to hydration having issues causing similar kind of misbehavior (in that case, it was doing triple fetch, IIRC).

An even more simple reproduction using links can be done by opening the browser directly at /item/4/path1/, then navigate up one level (Target 4, _, _) to "fix" the non-reactive rendering issue, then navigating to path1 (i.e. back to where we started) and then select field3 which would trigger that, or instead of selecting path1, select either path2 or path3 which would trigger that double fetch sooner. I was using the navigation links mostly because to show that reactivity was working normally but when refreshing everything is broken, not because the browser navigation was somehow special, I should have taken more care in noting that.

Edit: I can confirm the reactivity issue appears to be fixed and that CI also indicates that, but the double fetch is still present. I will be opening a new issue regarding #2959.

from leptos.