Stop processing original response streaming content if user has started navigating away #50814
Conversation
ghost
added
the
SteveSandersonMS
added
the
Servicing-consider
|
Hi @SteveSandersonMS. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
| const isFromEnhancedNav = node.getAttribute('enhanced-nav') === 'true'; | ||
| if (isFromEnhancedNav || hasNeverStartedAnyEnhancedPageLoad()) { | ||
| insertStreamingContentIntoDocument(componentId, node.content); | ||
| } |
There was a problem hiding this comment.
Can it happen that we cancel an enhanced nav mid-way (while some content has already been inserted) and we start processing a new enhanced nav response? Would we be in a situation where we've updated half the DOM and we start updating the rest with a newer version? Or will the new version replace the updated bits so far + the non-updated bits.
There was a problem hiding this comment.
Or will the new version replace the updated bits so far + the non-updated bits.
Yes, that. Each enhanced nav response is treated as a desired end state, and we diff as needed to reach that state, no matter what state we were in before.
Because of the single-threadedness of JS, and because we cancel any preceding enhanced nav response stream before we issue a new request, only the latest enhanced nav response is processed and it replaces whatever content was there before. If the previous fetch was streaming SSR and hadn't finished sending streaming updates, we don't care - we just cancel that previous fetch and hence won't see any more of its streaming updates.
There was a problem hiding this comment.
@SteveSandersonMS could that behavior cause weird outcomes/bugs? I imagine that worst case scenario it ends up with the whole document being replaced, I am just trying to think of situations where it could be problematic break things.
There was a problem hiding this comment.
Not that I know of. This is the way it's designed and expected to work. The guiding principle of enhanced nav is that it should produce the same outcome as a full-page navigation, with the exception of specific things we explicitly preserve (interactive components and data-preserve elements). On those grounds it's correct to disregard orphaned streaming updates from prior navigations.
…rsistedState cases See #50810
|
Note that I've been forced to cherry-pick two other test fixes into this PR in order to get the tests to pass. This may be because the version of Selenium/Chrome has changed in Helix. |
rbhanda
added
Servicing-approved
|
Hi @SteveSandersonMS. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed. |
|
The one test failure is completely unrelated (it's Kestrel Http2ConnectionTests), so merging anyway to begin unblocking the backlog of merges. |
SteveSandersonMS
deleted the
stevesa/fix-50733-streaming-continues-after-navigation
branch
* Handle obsolete InterceptorsPreview with .NET 8 RC2 SDK (#50797) * Quarantine (actually comment out) one of the CanRenderComponentWithPersistedState cases (#50811) See #50810 * Stop processing original response streaming content if user has started navigating away (#50814) * Reproduce #50733 as a failing E2E test * Don't process original request blazor-ssr content if the user has already navigated away * Quarantine (actually comment out) one of the CanRenderComponentWithPersistedState cases See #50810 * Update EventTest.cs * Disable another flaky test --------- Co-authored-by: Matt Thalman <mthalman@microsoft.com> Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
…0823) * Handle obsolete InterceptorsPreview with .NET 8 RC2 SDK (dotnet#50797) * Quarantine (actually comment out) one of the CanRenderComponentWithPersistedState cases (dotnet#50811) See dotnet#50810 * Stop processing original response streaming content if user has started navigating away (dotnet#50814) * Reproduce dotnet#50733 as a failing E2E test * Don't process original request blazor-ssr content if the user has already navigated away * Quarantine (actually comment out) one of the CanRenderComponentWithPersistedState cases See dotnet#50810 * Update EventTest.cs * Disable another flaky test --------- Co-authored-by: Matt Thalman <mthalman@microsoft.com> Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
Stop processing original response streaming content if user has started navigating away
Fix a correctness issue with streaming SSR and enhanced nav
Description
If enhanced nav occurred while streaming SSR was still in progress, subsequent streaming SSR updates could be inserted into the new page even though they are for the old page.
Fixes #50733
Customer Impact
Without this fix, the combination of streaming SSR and enhanced navigation (which is on by default) would be subject to edge-case issues where pages could display incorrect content.
Regression?
[If yes, specify the version the behavior has regressed from]
Risk
Only affects this one scenario. The change is very targeted.
Verification
Packaging changes reviewed?