Skip to content

Fix streaming SSR timing issue #51333

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 14, 2023

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Oct 12, 2023

Fix streaming SSR timing issue

On certain rare occasions, streaming SSR could produce a blank page due to a timing issue.

Description

If a page component:

  • Has @attribute [StreamRendering]
  • Is being visited through enhanced navigtion
  • Has an OnInitializedAsync whose returned task is not pre-completed but does complete very quickly (e.g., await Task.Yield())

... then there was a race condition. On rare occasions (seems to be about 1 in 20 page loads), the result would be a blank page. This didn't show up in our E2E tests because they always wait for fairly long async tasks so that the browser automation had time to observe the loading states. This PR adds a new E2E test that does recreate the issue and reliably fails - it does many, many page loads and basically always hits the problem at least once without the fix included here.

Fixes #51345

Customer Impact

Without this fix, a particular combination of features would be unreliable.

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

The only change is to fix what is definitely a bug. It's hard to see how this would have a negative effect, as no other code is built on top of this. It's not a public API - it's just part of how the Razor Components endpoint works.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added area-blazor Includes: Blazor, Razor Components labels Oct 12, 2023
@SteveSandersonMS SteveSandersonMS added the Servicing-consider Shiproom approval is required for the issue label Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

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.

@SteveSandersonMS SteveSandersonMS changed the title Fix wrong streaming output Fix streaming SSR timing issue Oct 13, 2023
@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review October 13, 2023 09:01
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner October 13, 2023 09:01
Copy link
Member

@surayya-MS surayya-MS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Contributor

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Steve.

Good idea to also include the comment in the SendStreamingUpdateAsync method.

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

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.

@mkArtakMSFT
Copy link
Contributor

mkArtakMSFT commented Oct 14, 2023

@SteveSandersonMS I think I've messed up my local git repo, so here how I ended up updating the blazor.web.js file.

  1. created a new branch off of release/8.0.
  2. Cherry-picked your first three commits onto it.
  3. Generated the blazor.web.js using npm run build in the Components\Web.JS folder
  4. Used the GitHub conflict resolution editor and pasted the full content of locally generated blazor.web.js into the editor (replacing all content).

I believe it's good, but wanted to get confirmation from you before merging.

By the way, do we have tests that would fail if this file is generated incorrectly?

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/fix-wrong-streaming-output branch from 76038c1 to 3b36db8 Compare October 14, 2023 13:45
@SteveSandersonMS
Copy link
Member Author

Used the GitHub conflict resolution editor and pasted the full content of locally generated blazor.web.js into the editor (replacing all content).

That sounds a bit suspicious as it should not have a merge conflict at all if it's been rebased. So just in case, I've regenerated it and force-pushed an update. This should be OK to merge now.

By the way, do we have tests that would fail if this file is generated incorrectly?

We used to but they got disabled. I filed an issue last September asking for those tests to be re-enabled but it doesn't look like that's yet been done: #43715

@mkArtakMSFT mkArtakMSFT merged commit faf594a into release/8.0 Oct 14, 2023
@mkArtakMSFT mkArtakMSFT deleted the stevesa/fix-wrong-streaming-output branch October 14, 2023 15:33
@ghost ghost added this to the 8.0.0 milestone Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants