Skip to content

Commit

Permalink
Fix streaming SSR timing issue (#51333)
Browse files Browse the repository at this point in the history
# 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
- [x] No

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

## Risk

- [ ] High
- [ ] Medium
- [x] 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

- [x] Manual (required)
- [x] Automated

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [x] N/A
  • Loading branch information
SteveSandersonMS authored Oct 14, 2023
1 parent cbfc558 commit faf594a
Show file tree
Hide file tree
Showing 8 changed files with 100 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ await EndpointHtmlRenderer.InitializeStandardComponentServicesAsync(
}
else
{
await _renderer.EmitInitializersIfNecessary(context, bufferWriter);
_renderer.EmitInitializersIfNecessary(context, bufferWriter);
}

// Emit comment containing state.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ public void InitializeStreamingRenderingFraming(HttpContext httpContext, bool is

public async Task SendStreamingUpdatesAsync(HttpContext httpContext, Task untilTaskCompleted, TextWriter writer)
{
// Important: do not introduce any 'await' statements in this method above the point where we write
// the SSR framing markers, otherwise batches may be emitted before the framing makers, and then the
// response would be invalid. See the comment below indicating the point where we intentionally yield
// the sync context to allow SSR batches to begin being emitted.

SetHttpContext(httpContext);

if (_streamingUpdatesWriter is not null)
Expand All @@ -56,9 +61,11 @@ public async Task SendStreamingUpdatesAsync(HttpContext httpContext, Task untilT

try
{
await writer.WriteAsync(_ssrFramingCommentMarkup);
await EmitInitializersIfNecessary(httpContext, writer);
await writer.FlushAsync(); // Make sure the initial HTML was sent
writer.Write(_ssrFramingCommentMarkup);
EmitInitializersIfNecessary(httpContext, writer);

// At this point we yield the sync context. SSR batches may then be emitted at any time.
await writer.FlushAsync();
await untilTaskCompleted;
}
catch (NavigationException navigationException)
Expand All @@ -77,15 +84,15 @@ public async Task SendStreamingUpdatesAsync(HttpContext httpContext, Task untilT
}
}

internal async Task EmitInitializersIfNecessary(HttpContext httpContext, TextWriter writer)
internal void EmitInitializersIfNecessary(HttpContext httpContext, TextWriter writer)
{
if (_options.JavaScriptInitializers != null &&
!IsProgressivelyEnhancedNavigation(httpContext.Request))
{
var initializersBase64 = Convert.ToBase64String(Encoding.UTF8.GetBytes(_options.JavaScriptInitializers));
await writer.WriteAsync("<!--Blazor-Web-Initializers:");
await writer.WriteAsync(initializersBase64);
await writer.WriteAsync("-->");
writer.Write("<!--Blazor-Web-Initializers:");
writer.Write(initializersBase64);
writer.Write("-->");
}
}

Expand Down Expand Up @@ -286,7 +293,7 @@ private static bool IsProgressivelyEnhancedNavigation(HttpRequest request)
{
// For enhanced nav, the Blazor JS code controls the "accept" header precisely, so we can be very specific about the format
var accept = request.Headers.Accept;
return accept.Count == 1 && string.Equals(accept[0]!, "text/html;blazor-enhanced-nav=on", StringComparison.Ordinal);
return accept.Count == 1 && string.Equals(accept[0]!, "text/html; blazor-enhanced-nav=on", StringComparison.Ordinal);
}

private readonly struct ComponentIdAndDepth
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,12 @@ protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)

if (_streamingUpdatesWriter is { } writer)
{
// Important: SendBatchAsStreamingUpdate *must* be invoked synchronously
// before any 'await' in this method. That's enforced by the compiler
// (the method has an 'in' parameter) but even if it wasn't, it would still
// be important, because the RenderBatch buffers may be overwritten as soon
// as we yield the sync context. The only alternative would be to clone the
// batch deeply, or serialize it synchronously (e.g., via RenderBatchWriter).
SendBatchAsStreamingUpdate(renderBatch, writer);
return FlushThenComplete(writer, base.UpdateDisplayAsync(renderBatch));
}
Expand Down
2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.web.js

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export async function performEnhancedPageLoad(internalDestinationHref: string, i
headers: {
// Because of no-cors, we can only send CORS-safelisted headers, so communicate the info about
// enhanced nav as a MIME type parameter
'accept': 'text/html;blazor-enhanced-nav=on',
'accept': 'text/html; blazor-enhanced-nav=on',
},
}, fetchOptions));
let isNonRedirectedPostToADifferentUrlMessage: string | null = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public void EnhancedNavRequestsIncludeExpectedHeaders()
// Specifying text/html is to make the enhanced nav outcomes more similar to non-enhanced nav.
// For example, the default error middleware will only serve the error page if this content type is requested.
// The blazor-enhanced-nav parameter can be used to trigger arbitrary server-side behaviors.
Assert.Contains("accept: text/html;blazor-enhanced-nav=on", allHeaders);
Assert.Contains("accept: text/html; blazor-enhanced-nav=on", allHeaders);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Globalization;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Text;
using System.Text.RegularExpressions;
using Components.TestServer.RazorComponents;
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure;
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures;
Expand Down Expand Up @@ -275,4 +278,42 @@ public void CanStreamDirectlyIntoSectionContentConnectedToNonStreamingOutlet()
Navigate($"{ServerPathBase}/streaming-with-sections");
Browser.Equal("This is some streaming content", () => Browser.Exists(By.Id("streaming-message")).Text);
}

[Fact]
public async Task WorksWithVeryBriefStreamingDelays()
{
// First check it works in the browser
Navigate($"{ServerPathBase}/brief-streaming");
var header = Browser.Exists(By.Id("brief-streaming"));
for (var i = 1; i < 20; i++)
{
Browser.FindElement(By.LinkText("Load this page")).Click();

// Keep checking the same header to show this is always enhanced nav
Assert.Equal("Brief streaming", header.Text);

Browser.True(() =>
{
var loadCount = int.Parse(Browser.FindElement(By.Id("load-count")).Text, CultureInfo.InvariantCulture);
return loadCount >= i;
});
}

// That's not enough to be sure it was really correct, since it might
// work in the browser even if the SSR framing is emitted in the wrong
// place depending on exactly where it was emitted. To be sure, we'll
// also validate the HTML response directly.
var url = Browser.Url;
var httpClient = new HttpClient();
for (var i = 0; i < 100; i++)
{
// We expect to see the SSR framing marker right before the first <blazor-ssr>
var req = new HttpRequestMessage(HttpMethod.Get, url);
req.Headers.Accept.Clear();
req.Headers.Add("accept", "text/html; blazor-enhanced-nav=on");
var response = await httpClient.SendAsync(req);
var html = await response.Content.ReadAsStringAsync();
Assert.Matches(new Regex(@"</html><!--[0-9a-f\-]{36}--><blazor-ssr>"), html);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
@page "/brief-streaming"
@attribute [StreamRendering]

<h3 id="brief-streaming">Brief streaming</h3>

<p>
At one point there was a bug whereby, if streaming was enabled but only waited
for a very short period, it could insert the SSR framing markers in the wrong place,
making the output corrupt and causing the UI to be replace with a blank page.
</p>
<p>
The test loads this page via enhanced nav many times, validating it always loads.
</p>

<a href="brief-streaming">Load this page</a>

@if (isLoaded)
{
<p>
Load counter: <span id="load-count">@loadCount</span>
</p>
}

@code {
static int loadCount;
bool isLoaded;

protected override async Task OnInitializedAsync()
{
await Task.Yield();
loadCount++;
isLoaded = true;
}
}

0 comments on commit faf594a

Please sign in to comment.