Add support for ServerSentEventsResult and extension methods #60616

captainsafia · 2025-02-25T20:19:31Z

Implements #56172.

Copilot

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

src/Http/Http.Results/src/ServerSentEventsResult.cs

src/Http/Http.Results/src/Results.cs

BrennanConroy · 2025-02-25T20:38:59Z

src/Http/Http.Results/src/ServerSentEventsResult.cs

+    {
+        ArgumentNullException.ThrowIfNull(httpContext);
+
+        httpContext.Response.ContentType = "text/event-stream";


Consider following what we do in SignalR
Lines 33-41
https://source.dot.net/#Microsoft.AspNetCore.Http.Connections/Internal/Transports/ServerSentEventsServerTransport.cs,33

Oh, and line 47 if that's something we care about.

Bleh -- I am torn about adding the Firefox workaround here. That seems like something the SseFormatter should be doing? Although admittedly it's a little gross to hardcode workarounds for buggy clients.

Maybe we leave it out for now and see what kind of feedback we get?

Yeah, until we see feedback I think it's less interesting for an API. For SignalR it was important because we want to signal that the connection is active and let the user start sending messages.

Any reason not to disable buffering?

That seems like something the SseFormatter should be doing?

What is that?

We should do what SignalR is doing, we should not wait for feedback. Disable buffering and do the crazy IIS workaround 😄 (did you test this in IIS?)

That seems like something the SseFormatter should be doing?

SignalR applies a special workaround to send some filler data through the SSE stream before the actual data to force Firefox's EventSource to emit an open event. It's a reaction to this bug in Firefox. The discussion is around whether or not to apply this workaround here in the ServerSentEventResult or have it be part of the default writing beahvior in the SseFormatter.

src/Http/Http.Results/src/ServerSentEventsResult.cs

src/Http/Http.Results/test/ServerSentEventsResultTests.cs

src/Http/Http.Results/src/ServerSentEventsResult.cs

BrennanConroy · 2025-02-25T21:12:56Z

src/Http/Http.Results/src/ServerSentEventsResult.cs

+            ? jsonTypeInfo
+            : jsonOptions.SerializerOptions.GetTypeInfo(typeof(object));
+
+        var json = JsonSerializer.Serialize(item.Data, typeInfo);


There must be a more performant way to do this.
@eiriktsarpalis

JsonSerializer.SerializeToUtf8Bytes?

Slightly better since it avoids the string, but it still allocates a byte[] that we have to copy into IBufferWriter.

looks like SerializeToUtf8Bytes boils down to roughly

using var jsonWriter = new Utf8JsonWriter(writer, jsonOptions.SerializerOptions); typeInfo.Write(jsonWriter, item.Data);

but I'm not familiar enough to be sure

It seems like SerializeToUtf8Bytes/initializing a Utf8JsonWriter around the IBufferWriter<byte> might be as good as it gets here but I'd be curious to here what @eiriktsarpalis thinks.

Would it make sense to utilize ReusableUtf8JsonWriter?

Would exposing JsonSerializer.Serialize accepting IBufferWriter<byte> overloads help in this case?

dotnet/runtime#112943

Would exposing JsonSerializer.Serialize accepting IBufferWriter<byte> overloads help in this case?

Yep! I think that would be a nice solution.

eng/SharedFramework.External.props

BrennanConroy · 2025-02-26T05:33:29Z

src/Http/Http.Results/src/ServerSentEventsResult.cs

+    {
+        ArgumentNullException.ThrowIfNull(httpContext);
+
+        httpContext.Response.ContentType = "text/event-stream";


Any reason not to disable buffering?

eiriktsarpalis · 2025-02-26T08:24:46Z

src/Http/Http.Results/src/Results.cs

+    /// Strings serialized by this result type are serialized as raw strings without any additional formatting.
+    /// </remarks>
+#pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters
+    public static IResult ServerSentEvents(IAsyncEnumerable<string> values, string? eventType = null)


Event types are values scoped to individual events, and not the entire stream. Given that the SseItem<T> overload hardcodes to serialization, how can this API produce raw SSE events that includes other per-event fields such as event types?

I would guess that this was discussed during API review, but have you considered factoring into a separate method group (e.g. ServerSentEventsRaw) that also accepts IAsyncEnumerable<SseItem<string>>?

Nevermind me, it seems the comment in line 1004 is addressing my concern.

eiriktsarpalis · 2025-02-26T08:31:08Z

src/Http/Http.Results/src/Results.cs

+    /// <param name="eventType">The event type to be included in the HTTP response body.</param>
+    /// <returns>The created <see cref="ServerSentEventsResult{T}"/> for the response.</returns>
+    /// <remarks>
+    /// Strings serialized by this result type are serialized as raw strings without any additional formatting.


Do we need to expose an option overriding this behavior?

We opted to expose fewer APIs for modifying the behavior of the formatter to start but I think we can pursue the proposal of adding an Action<Sse<T>, IBufferWriter> argument to these overloads in the event we hear feedback on it.

davidfowl · 2025-02-26T08:44:44Z

src/Http/Http.Results/src/ServerSentEventsResult.cs

+
+        // If the event type is string, we can skip JSON serialization
+        // and directly use the SseFormatter's WriteAsync overload for strings.
+        if (_events is IAsyncEnumerable<SseItem<string>> stringEvents)


What about byte[]?

It's useful in scenaria where you need to write raw UTF-8 data, but it might make more sense if we exposed that using the IBufferWriter callback as with the underlying library.

but it might make more sense if we exposed that using the IBufferWriter callback as with the underlying library.

This seems like a good proposal. In the meantime, we can add special handling for byte arrays in our FormatSseItem callback.

ANahr · 2025-02-27T22:47:17Z

The implementation doesn't seem to have any keep-alive mechanism.
In real live web applications it is likely that users will run into a lot of problems with proxies, firewalls and all kind of other systems that cut the connection between server and client after a certain time without any data sent.
If you take the "Basic usage with simple values" example and set the delay to 100000ms you will likely never get a second event when running the code outside a private network.

For a general purpose usable implementation it should send a kind of keep-alive message if no user-provided message is sent within a specific timeframe (e.g. 5/10/20/30/tbd seconds).

…inimal-sse-result

captainsafia · 2025-03-03T02:18:12Z

@ANahr Thanks for providing this feedback! I err on the side of keeping the API simpler here. It's possible for users to compose their own keep-alive message on a period timer on top of this API. The one gotcha is that there's no way to transmit comment messages (not complying with the SSE event format) with the current use of the SseFormatter.

Add support for ServerSentEventsResult and extension methods

5d844ff

Copilot AI review requested due to automatic review settings February 25, 2025 20:19

captainsafia requested review from BrennanConroy, halter73, wtgodbe and a team as code owners February 25, 2025 20:19

dotnet-issue-labeler bot added the area-web-frameworks label Feb 25, 2025

captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks labels Feb 25, 2025

Copilot AI reviewed Feb 25, 2025

View reviewed changes

src/Http/Http.Results/src/ServerSentEventsResult.cs Outdated Show resolved Hide resolved

src/Http/Http.Results/src/ServerSentEventsResult.cs Show resolved Hide resolved

BrennanConroy reviewed Feb 25, 2025

View reviewed changes

wtgodbe reviewed Feb 25, 2025

View reviewed changes

eng/SharedFramework.External.props Outdated Show resolved Hide resolved

captainsafia added 2 commits February 25, 2025 16:11

Address feedback

de86920

Use TaskCompletionSource to observe cancellation in tests

6f1f6be

BrennanConroy approved these changes Feb 26, 2025

View reviewed changes

eiriktsarpalis reviewed Feb 26, 2025

View reviewed changes

eiriktsarpalis mentioned this pull request Feb 26, 2025

Consider adding IBufferWriter<byte> overloads to JsonSerializer dotnet/runtime#112943

Open

eiriktsarpalis approved these changes Feb 26, 2025

View reviewed changes

davidfowl reviewed Feb 26, 2025

View reviewed changes

Disable buffering and add byte[] handling

664aa6d

captainsafia force-pushed the cs/minimal-sse-result branch from 9c83ed2 to 664aa6d Compare February 26, 2025 18:00

captainsafia added 2 commits March 2, 2025 17:49

Merge branch 'main' of https://github.com/dotnet/aspnetcore into cs/m…

ff67f8d

…inimal-sse-result

Rely on SSE in shared framework

b0c76b8

captainsafia merged commit bd1ba76 into main Mar 3, 2025
27 checks passed

captainsafia deleted the cs/minimal-sse-result branch March 3, 2025 17:20

dotnet-policy-service bot added this to the 10.0-preview3 milestone Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ServerSentEventsResult and extension methods #60616

Add support for ServerSentEventsResult and extension methods #60616

captainsafia commented Feb 25, 2025

Copilot AI left a comment

BrennanConroy Feb 25, 2025

BrennanConroy Feb 25, 2025

captainsafia Feb 26, 2025

BrennanConroy Feb 26, 2025

BrennanConroy Feb 26, 2025

eiriktsarpalis Feb 26, 2025

davidfowl Feb 26, 2025 •

edited

Loading

captainsafia Feb 26, 2025

BrennanConroy Feb 25, 2025

lewing Feb 25, 2025

BrennanConroy Feb 25, 2025

lewing Feb 25, 2025 •

edited

Loading

captainsafia Feb 26, 2025 •

edited

Loading

halter73 Feb 26, 2025

eiriktsarpalis Feb 26, 2025

eiriktsarpalis Feb 26, 2025

captainsafia Feb 26, 2025

BrennanConroy Feb 26, 2025

eiriktsarpalis Feb 26, 2025

eiriktsarpalis Feb 26, 2025 •

edited

Loading

eiriktsarpalis Feb 26, 2025

captainsafia Feb 26, 2025

davidfowl Feb 26, 2025

eiriktsarpalis Feb 26, 2025

captainsafia Feb 26, 2025

ANahr commented Feb 27, 2025 •

edited

Loading

captainsafia commented Mar 3, 2025

Add support for ServerSentEventsResult and extension methods #60616

Add support for ServerSentEventsResult and extension methods #60616

Conversation

captainsafia commented Feb 25, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewing Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

captainsafia Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiriktsarpalis Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ANahr commented Feb 27, 2025 • edited Loading

captainsafia commented Mar 3, 2025

davidfowl Feb 26, 2025 •

edited

Loading

lewing Feb 25, 2025 •

edited

Loading

captainsafia Feb 26, 2025 •

edited

Loading

eiriktsarpalis Feb 26, 2025 •

edited

Loading

ANahr commented Feb 27, 2025 •

edited

Loading