Skip to content
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

Developers can use Compression in WebSocket #20004

Closed
2 tasks done
Tracked by #44314
Tratcher opened this issue Jan 24, 2017 · 36 comments
Closed
2 tasks done
Tracked by #44314

Developers can use Compression in WebSocket #20004

Tratcher opened this issue Jan 24, 2017 · 36 comments
Assignees
Labels
area-System.Net Bottom Up Work Not part of a theme, epic, or user story Cost:M Work that requires one engineer up to 2 weeks Priority:1 Work that is critical for the release, but we could probably ship without Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@Tratcher
Copy link
Member

Tratcher commented Jan 24, 2017

AB#1118550
https://tools.ietf.org/html/rfc7692
aspnet/WebSockets#19

Implementation notes:
The System.Net.WebSockets.WebSocket.SendAsync API is poorly suited to compression/extension. Compression is a per-message flag set on the first frame using the RSV1 bit. SendAsync sends frames and only takes the buffer, message type, an EndOfMessage bool, and a cancellationToken. There's no simple way to indicate if the data is or should be compressed / transformed. Possible solutions:

  • Enable compression for all messages, don't provide a per message option
  • Add a new overload of SendAsync with a compression / RSV1 flag. (Requires changing the core WebSocket abstraction)
  • Add a property to WebSocket bool CompressOutput that can be toggled between messages (Requires changing the core WebSocket abstraction)
  • Overload WebSocketMessageType with CompressedBinary and CompressedText. As an enum we may be able to temporarily pass data through here by casting non-standard values. The enum values already do not map directly to the WebSocket frame type values.

ReceiveAsync is easier, it could:

  • Decompress the content internally
  • Add a flag to WebSocketReceiveResult indicating if the content was compressed.
  • Use the same WebSocketMessageType overloads from above indicating if the content is compressed (but only if it does not decompress it internally, don't want to confuse consumers).

The compression extension could not be correctly implemented as wrapper over the current WebSocket API due to the framing requirement. Compression either needs to be built in or the API needs to expose additional frame fields (e.g. RSV1).

Dev work:

@karelz
Copy link
Member

karelz commented Mar 2, 2017

Next step: We need formal API proposal

@talarari
Copy link

Any update on this?

@TechnikEmpire
Copy link

Also interested in this. I know that "+1" type comments are generally unhelpful, but remarks on other threads by team members have suggested they are helpful in this cause because this issue is being ignored due to a lack of interest expressed by the community.

I would suggest that people are simply not saying anything because there are third-party libs that implement this. I would also suggest that waiting for community input is a bad way to measure interest because of that. If HttpClient didn't support compression and MS was dragging their feet on it, the community would just make their own compliant client.

The community has done this already, and this effort predates Microsoft's. Have a gander on Nuget. There's over a million downloads of community driven websocket libraries that all address missing features in each other. The community interest has already come and the ship sailed, Microsoft can't gauge interest based on this.

@karelz
Copy link
Member

karelz commented Sep 14, 2018

Thanks for additional comments. Upvotes on top post are best way to help us prioritize.
Given the amount of work we have across the board on Networking team (incl. HTTP/2 support and some high-impactful missing APIs in the platform), it is unlikely we will be able to get to this in .NET Core 3.0 timeframe (hence Future milestone).
If there are community alternartives (a nuget package), that is great and relieves a bit pressure - we can focus on things which cannot be easily worked around by 3rd party nuget packages and need to be done in the platform itself.

@TechnikEmpire
Copy link

@karelz Ok that's cool. I agree that if there's a good community alternative, then it can be relied on for now anyway instead. However I'm not sure how this issue applies across the board. Kestrel implements its own websocket, for example, so you're stuck with whatever that implementation has. Anyway thanks for feedback. Is it possible for the community to work on this, or do you think it's something the core team will want control of initially?

@Tratcher
Copy link
Member Author

All of the ASP.NET Core servers expose the opaque upgrade so it's possible to plug in alternate WebSocket implementations. WebSockets isn't even part of the servers, it's shipped as middleware.
https://github.com/aspnet/WebSockets/blob/64e736173c294ac22028995ee47d56b21aaa3831/src/Microsoft.AspNetCore.WebSockets/WebSocketMiddleware.cs#L57-L64

@TechnikEmpire
Copy link

Thanks bruv

@karelz
Copy link
Member

karelz commented Sep 14, 2018

Community can work on almost anything in our repos. It just depends on how deeply invested the contributors are and how much time they are willing to put into it.
For new APIs, an API review has to happen when there is solid proposal - that is something the "core" team has to review & approve. I can help expedite that if there is serious interest from community members to push it forward.

@davidfowl
Copy link
Member

Yes it’s absolutely possible. @anurse implemented an alternative web socket API a while back before we switched to the corefx version.

@talarari
Copy link

Any suggestions for good community alternatives?

@zlatanov
Copy link
Contributor

@karelz @Tratcher If the task is still up for grabs, I would like to express interest in contributing here. I have already implemented this in a custom library of mine - https://github.com/zlatanov/websockets but my implementation differs from the suggested API.

What I did is introducing the ability to create a send buffer that implements IBufferWriter - see https://github.com/zlatanov/websockets/blob/4d0c4e43e278d3bc6c0e185bb09b828160af2db8/src/Maverick.WebSockets/WebSocketSendBuffer.cs. Based on the websocket capabilities (established during handshake) the send buffer automatically enables compression. It uses array pooling and the last segment being written to is always the uncompressed one. When the segment is full then a new compressed segment is taken from the array pool and the current one is deflated onto it.

Since the IBufferWriter interface is synchronous the disadvantage of this technique is that the message must be fully compressed before we start sending it. In my experience though this is not always a problem because messages sent through websockets are never too big. In a real world example I've had uncompressed text message of 1MB become 40KB.

Currently using my implementation the API usage would look like this:

// We would also have socket.TryCreateBinaryBuffer( out var buffer )
if ( socket.TryCreateTextBuffer( out var buffer ) ) // This would fail if socket isn't connected.
{
       // Buffer implements IBufferWriter<Byte> so any API that supports writing 
       // to it will work. (e.g. the new Json implementation )
       // ....
       // When we're done writing
       await socket.SendAsync( buffer ); 
       // Sending the buffer will consume it and release any resources held 
       // so it's illegal to use after send
}

What do you think?

@Tratcher
Copy link
Member Author

When messages can be of an arbitrary size needing to compress them all in memory isn't great. We're working through a lot of similar issues in AspNetCore where too much needs to happen before we can flush data and it impacts scalability.

@zlatanov
Copy link
Contributor

I understand. So what if the API looked like this:

Task SendAsync(ArraySegment<byte> buffer, 
               WebSocketMessageType messageType, 
               bool endOfMessage, 
               bool compress, 
               CancellationToken cancellationToken);

In this case though we need to be careful when we really do send. For example the compressed version of the payload (when endOfMessage is false) is too small, there will be negative impact if we flush the data to the underlying connection for every send.

Is it acceptable for this API to return Task.CompletedTask when the underlying compressed buffer isn't full enough (configurable) and endOfMessage isn't true? Thus we really flush only when we reach the underlying buffer capacity or end of message is true.

@Tratcher
Copy link
Member Author

Tratcher commented Mar 7, 2019

Avoiding a signature change for SendAsync would be ideal. Adding new WebSocketMessageType enum values looks like the least invasive option.

Is it acceptable for this API to return Task.CompletedTask when the underlying compressed buffer isn't full enough (configurable) and endOfMessage isn't true? Thus we really flush only when we reach the underlying buffer capacity or end of message is true.

Yes that should be fine as long as we flush for message boundaries.

@zlatanov
Copy link
Contributor

zlatanov commented Mar 7, 2019

@Tratcher That makes sense.

Can I submit a pull request for this or should we go through a more formal discussion about the changes required?

@Tratcher
Copy link
Member Author

Tratcher commented Mar 7, 2019

They're going to want a formal write-up of the the proposed API changes before a PR. E.g. a diff of the API surface including doc comments describing the new items.

@lukzog
Copy link

lukzog commented Sep 16, 2019

@Tratcher do you have a plan to fix in in .net core 5.0 version?
Do you know if any third party implementations like the one from @zlatanov or @tpeczek supports hosting under IIS with kestrel? Or this is not really possible because the work also involves changes to AspNetCoreModule?

@Tratcher
Copy link
Member Author

No changes have been planned yet.

You're right that hosting with IIS out-of-proc would be problematic because the proxy wouldn't support compression. Hosting with IIS in-proc should allow it.

@lukzog
Copy link

lukzog commented Sep 16, 2019

Thanks for clarification. However, is it possible to host ASP .Net Core application with IIS in-proc?

Is there any other workaround to enable compression with SignalR? Could one use e.g. long-polling?

@analogrelay
Copy link
Contributor

analogrelay commented Sep 16, 2019

Thanks for clarification. However, is it possible to host ASP .Net Core application with IIS in-proc?

It's possible to host ASP.NET Core apps in-proc in IIS since 2.2 and it's the default when using IIS in ASP.NET Core 3.0 now. More details can be found in our documentation: https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/iis/?view=aspnetcore-2.2#hosting-models

Is there any other workaround to enable compression with SignalR? Could one use e.g. long-polling?

SignalR actually tries to bypass and disable response compression as much as possible since compression generally requires some level of buffering, which disrupts the real-time nature of SignalR communication.

Can you provide more context on why you want WebSocket compression? You can always compress payloads you are sending yourself and decompress them on the client side.

@lukzog
Copy link

lukzog commented Sep 16, 2019

We have a fairly large and complex application in ASP .NET Core. We use regular http Ajax calls to retrieve page metadata/data and SignalR to push updates to the browser.
We would like to simplify our codebase by moving all communication to SignalR/Websockets. However, some responses may be large and their size could be greatly reduced with compression (it is plain Json).

We could compress and decompress the payloads ourselves, The only disadvantage is that we need to use an external javascript based decompression library. It is also a bit slower then letting a browser do the decompression.

@TheAngryByrd
Copy link

TheAngryByrd commented Sep 16, 2019

Same reason web-request clients can ask for content to be gzipped, so the applications on both ends don't need to have compression in their app layers. It's transparent.

@Tratcher
Copy link
Member Author

Understood that it would be more convenient at the websocket layer. In the meantime though you'll need to do the app level compression.

@analogrelay
Copy link
Contributor

We would like to simplify our codebase by moving all communication to SignalR/Websockets. However, some responses may be large and their size could be greatly reduced with compression (it is plain Json).

Another option may be to switch to using SignalR's binary format (based on MessagePack). That may reduce the overhead enough to help in your situation. If you still need to compress text, then yes, WebSocket compression is the only transparent way to do that.

@karelz
Copy link
Member

karelz commented Oct 2, 2019

Triage: We need solid API proposal with use case samples, etc. Any takers?

@zlatanov
Copy link
Contributor

zlatanov commented Oct 2, 2019

@karelz I would like to volunteer.

I've put this off long enough, and now that 3.0 has been released I would like to find time to do it. I know the internals of the WebSocket well, and what needs to be done to efficiently support compression.

@analogrelay
Copy link
Contributor

We'd certainly like to be able to have an end-to-end scenario around WebSocket compression. ASP.NET Core does share some code (via the WebSocket.CreateFromStream API) so we'll be happy to help review APIs and code to ensure we'll be able to adopt this on the server side. SignalR customers have asked us about supporting compressed payloads (for sending large content).

@karelz
Copy link
Member

karelz commented Oct 2, 2019

@zlatanov cool, as next step, can you please propose the API shape? See API process for details and examples. Thanks!

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@karelz karelz added Bottom Up Work Not part of a theme, epic, or user story and removed Bottom Up Work Not part of a theme, epic, or user story labels Dec 7, 2020
@StephenBonikowsky StephenBonikowsky moved this from Triaged - Future to .NET 6 Committed in .NET Core impacting internal partners Dec 8, 2020
@karelz karelz modified the milestones: Future, 6.0.0 Jan 5, 2021
@karelz karelz changed the title Implement WebSocket Compression Developers can use Compression in WebSocket Jan 12, 2021
@karelz karelz added Bottom Up Work Not part of a theme, epic, or user story Cost:M Work that requires one engineer up to 2 weeks Priority:1 Work that is critical for the release, but we could probably ship without Team:Libraries User Story A single user-facing feature. Can be grouped under an epic. and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 12, 2021
@karelz karelz self-assigned this Jan 12, 2021
@BrennanConroy
Copy link
Member

Close via #49304?

@davidfowl
Copy link
Member

We have to enable it.

@karelz
Copy link
Member

karelz commented Apr 30, 2021

@davidfowl what has to be enabled where? Is there a tracking work item?

@Tratcher
Copy link
Member Author

dotnet/aspnetcore#2715

@karelz
Copy link
Member

karelz commented Apr 30, 2021

Thanks @Tratcher, I added it to the list of dev work items in top post.

@karelz karelz assigned BrennanConroy and unassigned zlatanov May 4, 2021
@BrennanConroy
Copy link
Member

Is there a difference between this issue and the one in aspnetcore? Can we close this one if there isn't

@karelz
Copy link
Member

karelz commented May 14, 2021

@BrennanConroy This is User Story plugged into overall .NET 6 planning, the ASP.NET Core is a child.

@karelz
Copy link
Member

karelz commented May 27, 2021

The remaining work was addressed in dotnet/aspnetcore#2715 by PR dotnet/aspnetcore#32600

@karelz karelz closed this as completed May 27, 2021
.NET Core impacting internal partners automation moved this from .NET 6 Committed to Done May 27, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net Bottom Up Work Not part of a theme, epic, or user story Cost:M Work that requires one engineer up to 2 weeks Priority:1 Work that is critical for the release, but we could probably ship without Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
No open projects
Development

No branches or pull requests