Skip to content

Use new System.Net.ServerSentEvents package in SignalR #56206

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 1 commit into from
Jun 13, 2024

Conversation

BrennanConroy
Copy link
Member

Main differences:

  1. \r is handled now, before we didn't add support for just \r
  2. We used to use CancelPendingRead() to trigger cancellation, now we get an OperationCanceledException
  3. FormatException is no longer thrown when a partial message is received and the HTTP request is done. I believe that is more spec compliant.
  4. When a message is split across multiple data frames, a \n character is inserted into the final message between each data frame. Our old implementation used Environment.NewLine but the spec mentions \n

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Jun 12, 2024
@BrennanConroy BrennanConroy requested review from halter73, wtgodbe and a team as code owners June 12, 2024 18:03
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Nice. Thanks.

@BrennanConroy
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

try
{
while (true)
var parser = SseParser.Create(stream, (eventType, bytes) => bytes.ToArray());
Copy link
Member

Choose a reason for hiding this comment

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

What's this array allocation?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Code deletion is so nice!

@BrennanConroy BrennanConroy merged commit fc8f7a6 into main Jun 13, 2024
26 checks passed
@BrennanConroy BrennanConroy deleted the brecon/sseparser branch June 13, 2024 15:21
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview6 milestone Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants