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

Reuse ManagedWebSocket for websocket connections on Windows HttpListener. #19367

Closed
Priya91 opened this issue Nov 15, 2016 · 15 comments
Closed
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@Priya91
Copy link
Contributor

Priya91 commented Nov 15, 2016

Refer comment here

cc @stephentoub

@karelz
Copy link
Member

karelz commented Nov 15, 2016

@Priya91 is it netstandard2.0?

@Priya91
Copy link
Contributor Author

Priya91 commented Nov 15, 2016

@karelz Need to understand the change, @stephentoub can elaborate..

@stephentoub
Copy link
Member

is it netstandard2.0?

It's an implementation detail to avoid having multiple competing copies of a fairly large swath of code, and using the one that's already had performance work done on it, rather than the one that was just ported over as part of HttpListener as part of netstandard2.0 work. But it's not stricly necessary in order to ship.

@CIPop
Copy link
Member

CIPop commented Dec 8, 2016

Instead of compiling the code in HttpListener, we should instead move the protocol implementation into System.Net.WebSockets.

Here's the proposed plan:

  • Move ManagedWebSocket from S.N.WebSocket.Client to S.N.WebSocket.
  • For Win7 (ARM legs are failing in CI in release/2.0.0 #9503) and all non-Windows platforms use CreateClientWebSocket that will return a ManagedWebSocket for client-side operations. We may want to keep the client-side WinHTTP implementation as having a more complete implementation for HTTP operations (and potentially better perf). We should experiment and determine if this can be phased out using a more complete ManagedWebSocket implementation.
  • Expose the currently internal (System.Net.HttpListener) factory function API for server-side operations:
        internal static WebSocket CreateServerWebSocket(Stream innerStream,
            string subProtocol,
            int receiveBufferSize,
            TimeSpan keepAliveInterval,
            ArraySegment<byte> internalBuffer)
  • Implement the server-side WS protocol and tests based on ManagedWebSocket.
  • Remove the websocket.dll implementation from HttpListener and switch the code to use ClientServerWebSocket().

/cc: @stephentoub @karelz @anurse

@Priya91 Priya91 changed the title Reuse ManagedWebSocket in WebSocketBase in System.Net.HttpListener. Reuse ManagedWebSocket for websocket connections on HttpListener. Jan 23, 2017
@Priya91 Priya91 changed the title Reuse ManagedWebSocket for websocket connections on HttpListener. Reuse ManagedWebSocket for websocket connections on Windows HttpListener. Jan 23, 2017
@Tratcher
Copy link
Member

Note the managed implementation won't work on Win7 HttpListener, Http.Sys doesn't support the necessary upgrade. You're probably better off leaving the Windows HttpListener WebSocket code alone. It was extensively tested and tuned when it was written.

@karelz
Copy link
Member

karelz commented Jan 24, 2017

Maintaining multiple implementations brings non-trivial cost. Maybe we should keep it just for Win7 (which will eventually go out of support) and replace Win8+ implementation with the managed one?

@Tratcher
Copy link
Member

? There is no Win7 implementation.

@davidsh
Copy link
Contributor

davidsh commented Jan 24, 2017

Yes, there is a "Win 7" implementation. @stephentoub wrote a new managed websocket implementation that works right now on *Nix. But the plan is to just use that code for the Win7 binary in the NuGet package.

@Tratcher
Copy link
Member

There's no Win7 HttpListener implementation.

@davidsh
Copy link
Contributor

davidsh commented Jan 24, 2017

ok...you're right. This is discussing server-side websocket.

But the plan is to implement HttpListener using Mono code (which doesn't use http.sys) and re-use that for Win7. Basically, the plan is to just have a sockets server that understands enough HTTP to get to the point of becoming a WebSocket.

@Tratcher
Copy link
Member

Wait, .NET Core HttpListener isn't going to use Http.Sys on Windows?

@davidsh
Copy link
Contributor

davidsh commented Jan 24, 2017

No. That stays the same. But to implement WebSocket server-side on Win7, it will use an http stack other than http.sys.

@Priya91
Copy link
Contributor Author

Priya91 commented Feb 22, 2017

Related to #17644

@CIPop
Copy link
Member

CIPop commented Jul 7, 2017

/cc @davidsh @DavidGoll @Caesar1995
Talked with @Priya91 offline. The current status is:

  • Core HttpListener running on Win8+ uses HTTP.SYS + websocket.dll
  • Core HttpListener running on Win7 doesn't have WebSocket support
  • Core HttpListener running on Unix and UWP uses the managed implementation for HTTP and managed implementation for WebSocket

@karelz
Copy link
Member

karelz commented Oct 2, 2019

Triage: While nice to have, there is not clear customer value. There may be some maintenance value long-term, but given we don't touch the code base often, we can close it now.

@karelz karelz closed this as completed Oct 2, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

7 participants