Add WebSocket compression support

[BrennanConroy]

Fixes #2715

Main questions are around usability. Do we want to allow setting specific deflate options or just have a bool for enabling compression and let the client negotiate choose what it wants to use?

Added an AcceptWebSocketAsync overload to WebSocketManager to see how it looks to pass per connection options.

https://tools.ietf.org/html/rfc7692#section-5 for general negotiation details
https://tools.ietf.org/html/rfc7692#section-7 for permessage-deflate details

namespace Microsoft.Net.Http.Headers
{
    public static class HeaderNames
    {
+        public static readonly string SecWebSocketExtensions = "Sec-WebSocket-Extensions";
    }
}

namespace Microsoft.AspNetCore.Http
{
    public abstract class WebSocketManager
    {
+        public virtual Task<WebSocket> AcceptWebSocketAsync(WebSocketAcceptContext acceptContext) => throw new NotImplementedException();
    }
}

namespace Microsoft.AspNetCore.Http
{
    public partial interface IHeaderDictionary
    {
+        StringValues SecWebSocketExtensions { get; set; }
    }
}

namespace Microsoft.AspNetCore.Http
{
    internal sealed class DefaultWebSocketManager : WebSocketManager
    {
+        public override Task<WebSocket> AcceptWebSocketAsync(WebSocketAcceptContext acceptContext);
    }
}

namespace Microsoft.AspNetCore.WebSockets
{
    public class ExtendedWebSocketAcceptContext : WebSocketAcceptContext
    {
+        public bool DangerousEnableCompression { get; set; }
+        public bool DisableServerContextTakeover { get; set; } = false;
+        public int ServerMaxWindowBits { get; set; } = 15;
    }
}

namespace Microsoft.AspNetCore.Http
{
+    public static class WebSocketManagerExtensions
+    {
+        public static Task<WebSocket> AcceptWebSocketAsync(this WebSocketManager webSocketManager, ExtendedWebSocketAcceptContext acceptContext);
+    }
}

[BrennanConroy]

• Changed the options to only have server side settings
  • We could make the settings nullable and if either is set then assume compression is enabled (so we can remove the bool)
• Question: do we reject server_no_context if servercontext=true?

[Tratcher]

• We could make the settings nullable and if either is set then assume compression is enabled (so we can remove the bool)

No, better to have an explicit on-off switch.

[Tratcher]

• Question: do we reject server_no_context if servercontext=true?

If the peer server doesn't use context
takeover, the client doesn't need to reserve memory to retain the
LZ77 sliding window between messages.

Sounds like the client is supposed to be able to opt out, so it's fine for them to override servercontext=true.

[BrennanConroy]

Sounds like the client is supposed to be able to opt out, so it's fine for them to override servercontext=true.

At that point, why even expose an option for this? Just in the case where the client doesn't specify an option?

[Tratcher]

At that point, why even expose an option for this? Just in the case where the client doesn't specify an option?

So the server can also opt-out. It sounds like context takeover uses more memory on both client and server.

[Tratcher]

Have you enabled the autobahn tests yet?

[BrennanConroy]

I ran it earlier, but wanting to get everything cleaned up before trying again

[BrennanConroy]

All Autobahn tests passed

[BrennanConroy]

🆙 📅
added support for quoted values, duplicate parameters from a negotiate request is invalid so added that, renamed the server context takeover option to make it obvious that it's an opt-out, changed the base parsing tests to not use a server.

[Tratcher]

It's looking really good. What's left, autobahn?

[BrennanConroy]

What's left, autobahn?

Yeah, need to do another round of that, would like to figure out if we can make the AcceptWebSocketAsync api with the ExtendedWebSocketAcceptContext discoverable.

[BrennanConroy]

Would we be against moving the new properties to the base WebSocketAcceptContext?

Or maybe we can make an extension in the websocket project for AcceptWebSocketAsync(ExtendedWebSocketAcceptContext)

[Tratcher]

Would we be against moving the new properties to the base WebSocketAcceptContext?

We'd originally left them separate because the base WebSocketAcceptContext was supposed to be implementation agnostic. Now that we only use one implementation I'm not sure if that matters anymore.

Or maybe we can make an extension in the websocket project for AcceptWebSocketAsync(ExtendedWebSocketAcceptContext)

Let's start with that.

[BrennanConroy]

The extension method is a bit weird, it doesn't get called when passing an ExtendedWebSocketAcceptContext, but it at least shows in intellisense.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[BrennanConroy]

API review:
Remove AcceptWebSocketAsync extension
Move options from ExtendedWebSocketAcceptContext to WebSocketAcceptContext
Make KeepAliveInterval virtual on WebSocketAcceptContext
Obsolete ExtendedWebSocketAcceptContext

[davidfowl]

Make KeepAliveInterval virtual on WebSocketAcceptContext

Turns out this is a breaking change.

[Tratcher]

So how do we consolidate ExtendedWebSocketAcceptContext?

[BrennanConroy]

❌ DISALLOWED: Adding the virtual keyword to a member

We're adding a member to WebSocketAcceptContext, not making an existing member virtual so does it actually fall under this category?

We're still going to as cast to ExtendedWebSocketAcceptContext in the middleware, so old usage should still work, and new code will use the base type and we'll reference the new member.