Runtime: Add WebSocket.CreateFromConnectedStream

[stephentoub]

Several corefx components pull in the internal ManagedWebSocket implementation, both System.Net.WebSockets.Client on Unix and System.Net.HttpListener on Unix, and with https://github.com/dotnet/corefx/issues/9503 the plan is to use it in System.Net.WebSockets.Client on Windows 7 as well. Plus, ASP.NET uses the same implementation, via source cloning. We should stop building this multiple times and instead expose it out through an API that anyone can use to wrap an established stream for a websockets connection.
```C#
public abstract class WebSocket
{
public static WebSocket CreateFromConnectedStream(
Stream stream,
bool isServer,
string subprotocol,
TimeSpan keepAliveInterval,
int receiveBufferSize,
ArraySegment? receiveBuffer = null);
...
}

Related to https://github.com/dotnet/corefx/issues/13663#issuecomment-265879275
Related to https://github.com/dotnet/corefx/issues/21509#issuecomment-310862176

Once this is added, we can stop compiling it into the various projects and just use the exposed API.

Alternative:
WebSocket currently exposes the following API:
```C#
        [EditorBrowsable(EditorBrowsableState.Never)]
        public static WebSocket CreateClientWebSocket(Stream innerStream,
            string subProtocol, int receiveBufferSize, int sendBufferSize,
            TimeSpan keepAliveInterval, bool useZeroMaskingKey, ArraySegment<byte> internalBuffer)

We could remove that EditorBrowsable(Never) and add a corresponding CreateServerWebSocket API. Both would effectively use CreateFromConnectedStream, just with a different value for isServer. However, it's not clear we could overload the meaning of CreateClientWebSocket in corefx to be what we need, given its current semantics on desktop, in particular as on desktop it uses WPSC and is documented as such.

EDIT 9/26/2017:
C# // System.Net.WebSockets.Protocol.dll, with a netstandard2.0 build // (and subsequently specialized for netcoreapp2.1) namespace System.Net.WebSockets { public static class WebSocketProtocol { public static WebSocket CreateFromConnectedStream( Stream stream, bool isServer, string subprotocol, TimeSpan keepAliveInterval, int receiveBufferSize, int sendBufferSize, // hints as to desired size ArraySegment<byte>? buffer = null); // buffer space to use however the implementation wants } }

[weshaggard]

ApiReview:

• @stephentoub is going to look and see if we can reuse CreateClientWebSocket in a compatible way and just add a CreateServerWebSocket method. If not then we will go with the CreateFromConnectedStream method.

[stephentoub]

@stephentoub is going to look and see if we can reuse CreateClientWebSocket in a compatible way and just add a CreateServerWebSocket method.

In short, I think we could, albeit with some minor incompatibilities. I tried it out, and the main issue appears to be around argument validation (once you get past that, it appears to work fine, regardless of how the connection was established). The desktop implementation has tight restrictions around the values of the buffer and buffer size arguments, as it's just passing along the requirements of the underlying websockets.dll implementation it's built on top of. We have no such restrictions in the managed implementation, nor do we want them, which means on core this method will be more permissive in what it allows to be provided; if the same code then ran against desktop, any inputs that were outside of the allowed desktop range would throw. We could either live with this, and devs that want their code to be portable across the two would just need to use values from the more constrained set, and/or we could modify the desktop implementation in the future to be more adaptable (e.g. for values outside the range, "fix them" silently rather than throwing).

Or we could go with the new method. Any preferences? @anurse, thoughts from your end?

[anurse]

That sounds like a good plan. Would that support WebSocket serving from Win7 or would we be limited by the lack of websocket.dll there?

[stephentoub]

That sounds like a good plan

Which plan is the good plan? :)

Would that support WebSocket serving from Win7 or would we be limited by the lack of websocket.dll there?

On .NET Core, whichever of these APIs we expose, it'll work on Win7, as it'll be entirely managed code. The current implementation on desktop won't work on Win7 (nor is there currently a server API).

[stephentoub]

I guess the other option is we could just create another public class, like WebSocketProtocol or something, that exposes such a CreateFromConnectedStream method. That class could live in a separate library, which could be used on desktop as well, and ASP.NET could depend on that for the implementation everywhere. I don't love the idea of introducing a new library for a single method, but maybe it addresses folks' concerns?

[anurse]

Which plan is the good plan? :)

Yes. :P

Seriously though, having a CreateServerWebSocket is just fine with us. The only reason for CreateFromConnectedStream was that it seemed more general-purpose and there isn't really much different between client or server. But since CreateClientWebSocket already exists, CreateServerWebSocket is all we actually need.

On .NET Core, whichever of these APIs we expose, it'll work on Win7, as it'll be entirely managed code.

:+1:

[anurse]

That class could live in a separate library, which could be used on desktop as well

One thing this bring up (and maybe this was covered and I just didn't follow it): Since this type already exists, any new API on WebSocket wouldn't be available in Desktop, would it? If that's the case, perhaps the new type/new assembly is better, since it could run on both... Since we're netstandard2.0 and can't really take a dependency on a later version of the standard, a stand-alone library would be better.

[stephentoub]

@weshaggard, how do we feel about adding a new library that'd essentially be a single method (with a bunch of implementation behind it), e.g.
C# // System.Net.WebSockets.Protocol.dll, or something like that, for lack of a great name namespace System.Net.WebSockets { public static class WebSocketProtocol { public static WebSocket CreateFromConnectedStream( Stream stream, bool isServer, string subprotocol, TimeSpan keepAliveInterval, int receiveBufferSize, int sendBufferSize ArraySegment<byte>? buffer = null); } }

[davidfowl]

I like the new method and assembly but I'm a purist 😄

[weshaggard]

@weshaggard, how do we feel about adding a new library that'd essentially be a single method (with a bunch of implementation behind it), e.g.

It wouldn't be the first time we do it but is certainly isn't without costs either (i.e. new assembly which will need to always be deployed, and makes it harder to standardize assuming we want to standardize this API). Can you implement this API cleanly on top of netstandard 2.0 only?

[stephentoub]

Can you implement this API cleanly on top of netstandard 2.0 only?

Yes and no. Yes in that it can be done on top of netstandard 2.0. No in that a) our implementation of other libraries like System.Net.WebSockets.Client that are in .NET Core's standard implementation will need to depend on it, and b) .NET Core-only APIs help to make the implementation more efficient, in particular once dotnet/runtime#22387 is implemented and we have the Buffer/ValueTask-based APIs related to Stream.

[weshaggard]

OK if we implemented against the netstandard 2.0 API surface would it work correctly if run on desktop and on core? If so then this could is a reasonable option. Even if the NS2.0 version works on core we can still build a specialized version on core to make it more efficient I just wanted to make sure we aren't making any assumptions in the NS2.0 implementation that would cause it not to work on both.

[stephentoub]

OK if we implemented against the netstandard 2.0 API surface would it work correctly if run on desktop and on core?

Yes, functionally it would be correct.

[weshaggard]

OK then we could do it if we felt there was enough meat in the implementation to warrant it being separated.

[stephentoub]

if we felt there was enough meat in the implementation

The "meat" as it were is essentially this file:
https://github.com/dotnet/corefx/blob/master/src/Common/src/System/Net/WebSockets/ManagedWebSocket.cs

[stephentoub]

Then I suggest we do what I outlined above:
C# // System.Net.WebSockets.Protocol.dll, with a netstandard2.0 build // (and subsequently specialized for netcoreapp2.1) namespace System.Net.WebSockets { public static class WebSocketProtocol { public static WebSocket CreateFromConnectedStream( Stream stream, bool isServer, string subprotocol, TimeSpan keepAliveInterval, int receiveBufferSize, int sendBufferSize, // hints as to desired size ArraySegment<byte>? buffer = null); // buffer space to use however the implementation wants } }

[anurse]

Is this still on-track for 2.1.0? https://github.com/aspnet/WebSockets/issues/198 just reminded me just how awful it is to copy this code ;). Let me know if there's any way we can help, I think we're eager to get rid of the copied code ASAP.

/cc @jkotalik @muratg

[stephentoub]

Is this still on-track for 2.1.0?

I think it should be. I'm just waiting for an API review meeting to review putting it in its own assembly/package. @terrajobst, @weshaggard, can we get that on the books for whenever the next one is?

[davidfowl]

27 days have passed 😄

[stephentoub]

@karelz, this is the one I asked we ensure we get to in the review yesterday.

[anurse]

I was going to ping it but @davidfowl beat me to it ;). Hopefully it's a pretty straightforward review since it's a single API. Let me know if you need any help from us to get things moving.

[anurse]

One data point to add in: Our desire is to have a single netstandard API to do this. It can use native light-up on Windows 8+, but we'd want Windows 7 and non-Windows supported as well via the managed implementation. To be honest, I don't think we'd mind if it just uses the managed implementation everywhere (cc @davidfowl on that statement)

[stephentoub]

To be honest, I don't think we'd mind if it just uses the managed implementation everywhere

My plan is that it's purely a managed implementation. I'd want to see data / strong arguments to do otherwise.

[davidfowl]

MANAGED CODE EVERYWHERE!

[anurse]

[karelz]

this is the one I asked we ensure we get to in the review yesterday.

Sorry @stephentoub, I entirely missed that yesterday :( (at least I don't recollect it at all). The truth is we have accumulated API review backlog over the summer and we focused on larger API reviews lately to get through bigger things to support SignalR, so we didn't get through the "small ones" on GH.

Is there any pressure on this one beyond "just" getting it into 2.1? E.g. upstack dependencies, etc. 2.1 is still "far out" and we will be for sure able to catch up with he API review backlog in next 2 months.

Also, we can start reviewing issues marked as 2.1 (instead of based on age) -- I assume those are higher priority than the rest. cc @terrajobst

[stephentoub]

upstack dependencies, etc.

Yes, the reason @anurse and @davidfowl have been pushing on it is because ASP.NET web sockets support depends on it.

[stephentoub]

Thanks, @karelz.

[anurse]

Priority-wise, we would certainly appreciate taking care of this sooner rather than later as the hack we've had in place (copying source code) has been there for a while and is starting to break down a bit.

[karelz]

Can you please provide some insights into how it is breaking down a bit? (I get that source copy is not pretty and should be replaced, just curious what else might make it break ...)

[stephentoub]

I get that source copy is not pretty and should be replaced, just curious what else might make it break

e.g.

• We have different build systems and they handle resources differently, so any time for example we change a resource string (or worse, add one), that complicates the ASP.NET process.
• Any time we take a dependency on something in corefx that's not available on top of netstandard, it breaks ASP.NET.

[anurse]

so any time for example we change a resource string (or worse, add one), that complicates the ASP.NET process.

This is the one that hit us recently. We have to figure out a pattern for handling CoreFX resources that are in a different form to those we use. It's not by any means a hard problem, just another of the little cuts that are making this process difficult :). The source copying process was always designed to be temporary and started in 1.1 so it's been on the queue for a little while.

[karelz]

Ah, does it mean that you have a copy of entire WebSocket class and maybe even more?
I originally though about it as "just the method source code".

[jkotalik]

We currently copy these two classes from from the websockets source code. It is a bit more than "just the method source code".

[karelz]

Thanks for info! It's useful background for future.

[terrajobst]

We'll discuss the OOB separately. Let's remove the size parameters; if they are needed we can them back. But as it stands it looks weird to pass in sizes and a buffer.

C# // System.Net.WebSockets.Protocol.dll, with a netstandard2.0 build // (and subsequently specialized for netcoreapp2.1) namespace System.Net.WebSockets { // If we don't OOB we should use WebSocket public static class WebSocketProtocol { // If we don't OOB it, it should just be CreateFromStream public static WebSocket CreateWebSocketFromStream( Stream stream, bool isServer, string subProtocol, TimeSpan keepAliveInterval, Memory<byte> buffer = default); // buffer space to use however the implementation wants } }

[terrajobst]

We still need to decide on OOBing

[davidfowl]

OOB it

[anurse]

As a data point: If it isn't OOBed it's going to be a long time before we can actually use it and we'll have to keep our code copying in place for quite a while, risking divergence if we aren't diligent about syncing.

[karelz]

Let's not forget also the option of the middle ground: Use same source code in CoreFX, but don't OOB the same types by changing namespace (the EventSource route).
Overall there is cost, risk and value to each of these 3 solutions. There is no clear winner.

[davidfowl]

We'll keep copying source codez then.

[ghost]

This seems to be implemented by https://github.com/dotnet/corefx/pull/24846 months ago, but waiting for OOB decision which will still take some time? Please consider that many many people are relying on WebSockets in ASP.NET. If it isn't a package-update step away in ASP.NET WebSockets, then we will be in the situation where we are today that ASP.NET WebSockets copy is quite old.

We'll keep copying

Until 2.1 or forever? :)