Stop shipping and building WebSocketsProtocol assembly

[buybackoff]

ManagedWebSocket in System.Net.WebSockets.WebSocketProtocol is different from the implementation in System.Net.WebSockets for .NET 2.1. The former NuGet package is updated on May 5 as 4.5.0-rc1 (current), on the same day when Microsoft.NETCore.App package that includes the newest System.Net.WebSockets for .NET Core 2.1 was released as 2.1.0-rc1.

I had to dig into the source to find out that Microsoft.AspNetCore.WebSockets, also released on that day as 2.1.0-rc1-final, depends on System.Net.WebSockets.WebSocketProtocol and not on System.Net.WebSockets on this line. Unfortunately, that doesn't allow to enjoy the performance benefits from https://github.com/dotnet/corefx/issues/27445 when running ASPNETCore 2.1 apps.

Is this for a reason? How to use the latest low-allocations implementation currently in ASPNETCore 2.1 apps?

[benaadams]

cc: @davidfowl, @anurse, @stephentoub, @geoffkizer

[buybackoff]

I've just copied the package Microsoft.AspNetCore.WebSockets and added to the linked line:

#if NETCOREAPP2_1
                return WebSocket.CreateFromStream(opaqueTransport, isServer: true, subProtocol: subProtocol, keepAliveInterval: keepAliveInterval);
#else
                return WebSocketProtocol.CreateFromStream(opaqueTransport, isServer: true, subProtocol: subProtocol, keepAliveInterval: keepAliveInterval);
#endif

WebSocket now works as expected in dotnet/corefx#27445 but the number of allocations is still very big.

It looks like underlying socket allocates a lot and doesn't behave like in dotnet/corefx#27445.

Playing with my old fork of SignalR with the goal to eliminate amortized allocations "completely" on the server. Did SignalR achieve that BTW? Cannot build the source master locally - too complicated without docs and with 2.2 dependencies and tooling. Ideally I want to understand if current SignalR adds enough overheads for small binary messages (I use my own header and dispatcher logic) so that manual WebSocket-only implementation could be better.

[davidfowl]

Yes, I think we need to cross somewhere here to get the correct overrides. Free performance win! Great catch!

/cc @BrennanConroy

Playing with my old fork of SignalR with the goal to eliminate amortized allocations "completely" on the server. Did SignalR achive that BTW? Cannot build the source master locally - too complicated without docs and with 2.2 dependencies and tooling. Ideally I want to understand if current SignalR adds enough overheads for small binary messages (I use my own header and dispatcher logic) so that manual WebSocket-only implementation could be better.

PS: Yes the current version of SignalR is much more efficient than the previous SignalW implementation. The version you forked was extremely early.

[buybackoff]

PS: Yes the current version of SignalR is much more efficient than the previous SignalW implementation. The version you forked was extremely early.

For sure, but I cannot easily comprehend the current version and many abstractions between bytes in --> process them --> bytes out 😄

Looking into HttpUpgradeStream and do not see any methods with ValueTask. Probably optimized ManagedWebSocket is useless if the underlying stream can't help it?

[davidfowl]

I should say a bit more about SignalR though that's unrelated to this issue (which could be moved to https://github.com/aspnet/WebSockets/).

Playing with my old fork of SignalR with the goal to eliminate amortized allocations "completely" on the server.

Which allocations? Per connection or per message?

Cannot build the source master locally - too complicated without docs and with 2.2 dependencies and tooling.

You can use the bits, they're on nuget and measure for yourself.

I'd continue the discussion about SignalR specifics over here https://github.com/aspnet/SignalR.

[buybackoff]

Which allocations? Per connection or per message?

The goal is to avoid GC stall/jitter while processing messages so that 99.99% percentile for message processing time is bounded and stable.

You can use the bits, they're on nuget and measure for yourself.

I need to compare it with some alternative. Maybe I end up with raw NetworkStream

I'd continue the discussion about SignalR specifics over here https://github.com/aspnet/SignalR.

Sure when I touch & profile it. Issue for this repo is about ManagedWebSockets mismatch in sibling packages.

[davidfowl]

Sounds great! I'm pretty sure it'll be sufficient for your needs! The version you forked was a prototype. Lots of performance work went into the 1.0.0 release and there's more to go but most of the low hanging fruit was fixed.

I need to compare it with some alternative. Maybe I end up with raw NetworkStream

That's pretty much writing your own web server which might not be crazy depending on what you're trying to achieve. If you're going down that path, you can look at this https://github.com/davidfowl/PlatformLevelTechempower/blob/944597f9ad196d3a333b837fb8c62a7304d5f9d2/ServerWithKestrel2.1/WebSocketConnection.cs. It's a websocket server built on top of kestrel directly without ASP.NET. It's still using the ManagedWebSocket implementation but it's up to you to decide how far you're willing to go.

Sure when I touch & profile it. Issue for this repo is about ManagedWebSockets mismatch in sibling packages.

To put the issue back on topic, we either need to ifdef in ASP.NET or in the WebSocketProtocol package. I don't have a strong opinion here.

[davidfowl]

This is being fixed in ASP.NET Core 3.0. It's possible that a future version of WebSocketProtocol.CreateFromStream could be cross compiled to light up WebSocket.CreateFromStream.

@stephentoub Any thoughts on that? If no, then we can close this bug?

[stephentoub]

Any thoughts on that? If no, then we can close this bug?

I agree. WebSocketProtocol was created purely as a temporary solution to enable ASP.NET when built targeting netstandard20 to reuse the same WebSocket.CreateFromStream implementation without needing to copy and paste and tweak source. When built targeting netstandard20, WebSocketProtocol couldn't use those new overloads introduced in .NET Core 2.1, and anything built for newer surface area can just target WebSocket.CreateFromStream. So I agree this can be closed, as at this point I think WebSocketProtocol has served its purpose and is unlikely to evolve in any meaningful way.

[karelz]

Can we stop building it and shipping it then?

[karelz]

Triage: Stop building and shipping the assembly altogether.

Whoever picks it up, check with @ericstj for guidance.

[stephentoub]

@karelz, this got triaged to future. Wouldn't it make sense to fix it for .NET 5 so that we don't need to produce new packages for it? Or would we still need to anyway for some reason? @ericstj?

[zlatanov]

What is the current status of this item? I've started working on implementing websocket compression (issue here) and I've noticed that the actual websocket implementation exists as a shared source and cross compiled in both in System.Net.WebSockets and System.Net.WebSockets.WebSocketProtocol.

How should I proceed with the implementation:

1. Continue to support WebSocketProtocol and add the implementation here - https://github.com/dotnet/runtime/tree/master/src/libraries/Common/src/System/Net/WebSockets.
2. Ignore WebSocketProtocol and add the implementation inside System.Net.WebSockets project only.

/cc @stephentoub @karelz @davidfowl

[stephentoub]

What is the current status of this item?

I believe we've all agreed it should be done but no one's done it. @karelz, can this be prioritized?

[karelz]

@CarnaViire will take care of this one. Thanks @CarnaViire! (expected start next week)

[stephentoub]

Cool, thanks.