Repository navigation
Fix TempData and SupplyParameterFromSession persistence for streaming SSR case #66832
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
Changes from 10 commits
1acf121
376d91c
ee4ce7e
3fffa0c
27c8524
b8da91a
0bc4de3
16a6e52
e43b92a
cc816f9
18d36f6
29d04cf
42664d8
295808b
fb4827f
fef336d
346eaa4
7fe1b48
c194ae4
cda1185
315c172
1c27423
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ namespace Microsoft.AspNetCore.Components.Endpoints; | |
|
|
||
| internal partial class RazorComponentEndpointInvoker : IRazorComponentEndpointInvoker | ||
| { | ||
| private const string SessionEstablishmentKey = "__AspNetCore.Components.Endpoints.SessionEstablishment"; | ||
| private readonly EndpointHtmlRenderer _renderer; | ||
| private readonly ILogger<RazorComponentEndpointInvoker> _logger; | ||
| private readonly ComponentsActivityLinkStore _activityLinkStore; | ||
|
|
@@ -33,6 +34,34 @@ public RazorComponentEndpointInvoker(EndpointHtmlRenderer renderer, ILogger<Razo | |
| _logger = logger; | ||
| } | ||
|
|
||
| // Issues the session cookie before the first response chunk is flushed, so that | ||
| // session-backed values written during or after streaming SSR (TempData, | ||
| // SupplyParameterFromSession) can still be committed once headers are frozen. | ||
| private static void TryRegisterSessionEstablishment(HttpContext context) | ||
| { | ||
| var session = context.Features.Get<ISessionFeature>()?.Session; | ||
| if (session is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| context.Response.OnStarting(static state => | ||
| { | ||
| var session = (ISession)state!; | ||
| if (session.TryGetValue(SessionEstablishmentKey, out _)) | ||
| { | ||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| // Flip the session middleware's establishment gate with a no-op Set+Remove. | ||
| // Set is the only operation that triggers SessionEstablisher.TryEstablishSession; | ||
| // Remove leaves no entry behind in the in-memory store and doesn't reset the gate. | ||
| session.Set(SessionEstablishmentKey, []); | ||
| session.Remove(SessionEstablishmentKey); | ||
| return Task.CompletedTask; | ||
| }, session); | ||
| } | ||
|
|
||
| public Task Render(HttpContext context) | ||
| { | ||
| return _renderer.Dispatcher.InvokeAsync(() => RenderComponentCore(context)); | ||
|
|
@@ -89,6 +118,12 @@ await _renderer.InitializeStandardComponentServicesAsync( | |
| handler: result.HandlerName, | ||
| form: result.HandlerName != null && context.Request.HasFormContentType ? await context.Request.ReadFormAsync() : null); | ||
|
|
||
| // Issue the session cookie before any streaming chunks flush the response headers, | ||
| // so that TempData / [SupplyParameterFromSession] values written during async | ||
| // rendering can still be persisted after Response.HasStarted. No-op when session | ||
| // middleware is not registered. | ||
| TryRegisterSessionEstablishment(context); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now runs for every component endpoint even if the endpoint doesn't use the supply-param storage. We could avoid it by calling it in subscription methods. We can move I'm not sure though if it won't be too late, please check the case of streaming deferred children. Add a test: SSR page with streaming that renders a streaming child component. That child starts streaming in In case that scenario would fail, please try to think how to avoid over-releasing the cookie, we don't want behavioral change for endpoints not using this feature. I was considering if we should prevent this call in non-streaming scenarios but there's no need if we already move it to subscription. The cookie must be released in these cases anyway so it's fine to do it a bit earlier.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved cookie creation to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If we continue trying to have this feature working in streaming then we have to clearly define what scenarios are supported. Docs need clear information when it's safe to use the parameter or prevent them from using the parameter in scenarios that are silently no-op.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. Compare the behavior against MVC/RazorPages after
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @javiercn From my investigation MVC TempData just silently skips persisting elements after response has started. |
||
|
|
||
| // Matches MVC's MemoryPoolHttpResponseStreamWriterFactory.DefaultBufferSize | ||
| var defaultBufferSize = 16 * 1024; | ||
| await using var writer = new HttpResponseStreamWriter(context.Response.Body, Encoding.UTF8, defaultBufferSize, ArrayPool<byte>.Shared, ArrayPool<char>.Shared); | ||
|
|
@@ -173,6 +208,14 @@ await _renderer.InitializeStandardComponentServicesAsync( | |
| _renderer.EmitInitializersIfNecessary(context, bufferWriter); | ||
| } | ||
|
|
||
| // Persist TempData and Session values after all components (including streaming) | ||
| // have finished rendering, so that values modified during async rendering are captured. | ||
| if (context.RequestServices.GetService<SessionCascadingValueSupplier>() is { } sessionSupplier) | ||
| { | ||
| await sessionSupplier.PersistAllValues(); | ||
| } | ||
| TempDataProviderServiceCollectionExtensions.PersistTempData(context); | ||
|
|
||
|
dariatiurina marked this conversation as resolved.
|
||
| // Emit comment containing state. | ||
| if (!isErrorHandlerOrReExecuted) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Components.TestServer.RazorComponents; | ||
| using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; | ||
| using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures; | ||
| using Microsoft.AspNetCore.E2ETesting; | ||
| using OpenQA.Selenium; | ||
| using TestServer; | ||
| using Xunit.Abstractions; | ||
|
|
||
| namespace Microsoft.AspNetCore.Components.E2ETest.Tests; | ||
|
|
||
| public class StreamingSessionPersistenceTest : ServerTestBase<BasicTestAppServerSiteFixture<RazorComponentEndpointsNoInteractivityStartup<App>>> | ||
| { | ||
| private const string SessionCookieName = ".AspNetCore.Session"; | ||
|
|
||
| public StreamingSessionPersistenceTest( | ||
| BrowserFixture browserFixture, | ||
| BasicTestAppServerSiteFixture<RazorComponentEndpointsNoInteractivityStartup<App>> serverFixture, | ||
| ITestOutputHelper output) | ||
| : base(browserFixture, serverFixture, output) | ||
| { | ||
| } | ||
|
|
||
| protected override void InitializeAsyncCore() | ||
| { | ||
| _serverFixture.AdditionalArguments.Add("--UseSessionStorageTempDataProvider=true"); | ||
|
dariatiurina marked this conversation as resolved.
|
||
| _serverFixture.AdditionalArguments.Add("--UseSession=true"); | ||
| Browser.Manage().Cookies.DeleteCookieNamed(SessionCookieName); | ||
| base.InitializeAsyncCore(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void StreamingSSR_PersistsSupplyParameterFromSession_AfterAsyncRendering() | ||
| { | ||
| Navigate($"{ServerPathBase}/streaming-session-persistence"); | ||
|
|
||
| Browser.Exists(By.Id("streaming-complete")); | ||
| Navigate($"{ServerPathBase}/supply-parameter-from-session"); | ||
| Browser.Equal("set-during-streaming", () => Browser.FindElement(By.Id("text-email")).Text); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void StreamingSSR_PersistsSupplyParameterFromTempData_AfterAsyncRendering() | ||
| { | ||
| Navigate($"{ServerPathBase}/streaming-session-persistence"); | ||
|
|
||
| Browser.Exists(By.Id("streaming-complete")); | ||
| Navigate($"{ServerPathBase}/tempdata"); | ||
| Browser.Equal("tempdata-set-during-streaming", () => Browser.FindElement(By.Id("supply-parameter-from-tempdata")).Text); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void StreamingSSR_PersistsTempDataCascadingParameter_AfterAsyncRendering() | ||
| { | ||
| Navigate($"{ServerPathBase}/streaming-session-persistence"); | ||
|
|
||
| Browser.Exists(By.Id("streaming-complete")); | ||
| Navigate($"{ServerPathBase}/tempdata"); | ||
| Browser.Equal("streaming-tempdata-message", () => Browser.FindElement(By.Id("message")).Text); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| @page "/streaming-session-persistence" | ||
| @attribute [StreamRendering] | ||
|
|
||
| <h1>Streaming Session Persistence Test</h1> | ||
|
|
||
| @if (_done) | ||
| { | ||
| <p id="streaming-complete">Done</p> | ||
| } | ||
| else | ||
| { | ||
| <p id="streaming-status">Waiting for streaming...</p> | ||
| } | ||
|
|
||
| @code { | ||
| private bool _done; | ||
|
|
||
| [SupplyParameterFromSession] | ||
| public string? Email { get; set; } | ||
|
|
||
| [SupplyParameterFromTempData] | ||
| private string SupplyParameterFromTempDataValue { get; set; } = string.Empty; | ||
|
|
||
| [CascadingParameter] | ||
| public ITempData? TempData { get; set; } | ||
|
|
||
| protected override async Task OnInitializedAsync() | ||
| { | ||
| // The await is critical: it causes the component to enter the streaming phase. | ||
| // Values set AFTER the await are the ones that were silently lost with the old | ||
| // OnStarting-based persistence, because OnStarting fires before streaming begins. | ||
| await Task.Delay(100); | ||
|
|
||
| Email = "set-during-streaming"; | ||
| SupplyParameterFromTempDataValue = "tempdata-set-during-streaming"; | ||
| if (TempData is not null) | ||
| { | ||
| TempData["Message"] = "streaming-tempdata-message"; | ||
| } | ||
| _done = true; | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.