WIP: WebSocket Compression #48470
WIP: WebSocket Compression #48470
Conversation
…supported platforms because we now depend on native API.
…t deflate encoders more naturally.
…late was used and a message were to be sent with multiple frames the implementation would have used seprate deflates for each of the frames.
…hich is now supported.
…s seperate builds for each platform.
… websocket handles incoming data where we could easily just use a memory stream.
…mStream method in order to keep the name for the ArgumentException that would happen the same as it was.
…d a few bugs along the way.
…press / decompress messages as expected.
…nger a valid window bits value, because the underlying gzip library doesn't really support it.
… multiple frames and reading with smaller output buffers.
…xpected to happen but doesn't and we don't have timeouts.
…te flag in the header if decoder is not configured.
…nt sizes. Fixed a bug when receiving 0 byte message. Fixed another bug where the payload length wasn't reduced after receiving non compressed message.
… during client initiated close, the server would not respond with close message and the client's close task would hang until timeout or server closes the connection.
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketDeflateOptions.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketCreationOptions.cs
Show resolved
Hide resolved
| @@ -137,29 +137,34 @@ public static ArraySegment<byte> CreateServerBuffer(int receiveBufferSize) | |||
| [UnsupportedOSPlatform("browser")] | |||
| public static WebSocket CreateFromStream(Stream stream, bool isServer, string? subProtocol, TimeSpan keepAliveInterval) | |||
| { | |||
| if (stream == null) | |||
| if (!WebSocketCreationOptions.IsKeepAliveValid(keepAliveInterval)) | |||
There was a problem hiding this comment.
Why is KeelAliveInterval validated twice, here and on WebSocketCreationOptions?
There was a problem hiding this comment.
I did this for backward compatibility. The issue is that when the KeepAliveInterval is validated in WebSocketCreationOptions, when invalid an ArgumentOutOfRangeException for paramName "KeepAliveInterval" (PascalCase), but the existing code was throwing ArgumentOutOfRangeException with paramName "keepAliveInterval" (camelCase). There was even a test for that. It felt wrong for me to change the test to check for PascalCase for the name or even more wrong to throw camelCase when validating the property.
If you consider that the CreateFromStream would throw ArgumentOutOfRangeException with different casing than the actual name of the argument OK and not a breaking change, I will remove it and validate it only once in the property setter.
src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/ref/System.Net.WebSockets.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Provides a wrapper around the ZLib compression API. | ||
| /// </summary> | ||
| internal sealed class WebSocketDeflater : IDisposable |
There was a problem hiding this comment.
How is it guaranteed that it will not be used concurrently?
There was a problem hiding this comment.
The guarantee comes from the ManagedWebSocket class itself. It disallows concurrent sends or receives.
…added explanation why we don't support 8 bits althouh the RFC allows it.
… connection close.
…terminism from tests.
|
Closing this PR in favor of a new implementation that requires no (or least very little) refactoring of the existing ManagedWebSocket code. |
ghost
locked as resolved and limited conversation to collaborators
Creating this WIP Pull Request so others can give feedback. The WebSocket per message compression is fully implemented. I've written several tests and will add more to make sure everything is covered.
I would like to write a few performance tests to make sure there are no regressions and to find out if/where we could optimize if needed.
For the ClientWebSocket I need a working HTTP Server that supports WebSocket compression in order to write integration tests (outer loop). I see that there are some external servers and endpoints that are being used, but I don't know who is responsible for them and how can we add such support before AspNetCore team implements it?
A note to the owners of System.IO.Compression - as discussed with @CarnaViire, I've moved two of the files (ZLibNative.ZStream.cs and ZLibNative.cs) to Common so I can use the Interop.zlib.cs.
Any feedback and/or suggestions are welcomed.
Closes #31088
/cc @CarnaViire, @karelz, @scalablecory, @davidfowl