Added support for WebSockets #41

mkosieradzki · 2017-02-22T16:08:35Z

Added support for WebSockets

Bumped .NET Framework version to 4.6 (required by System.Net.WebSockets.Client)
Updated sample project to be HttpUpgradeFeature-compatible

Addresses #38

Known issues:

Connection closing handshake crashes when using IIS Express (It works correctly with standalone Kestrel and WebListener)
Lacking tests - it would be good idea to test it using Autobahn Testsuite

Upgraded .NET Framework to 4.6 Updated example to use web sockets on Kestrel

dnfclas · 2017-02-22T16:08:41Z

Hi @mkosieradzki, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

Tratcher · 2017-02-22T16:57:51Z

Note some implementations of the ClientWebSocket don't support Win7.

Tratcher · 2017-02-22T16:59:27Z

Too bad Ping and Pong messages can't be forwarded with this abstraction. You'll want some configuration for the ping/pong intervals.

Tratcher · 2017-02-22T16:48:17Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+            {
+                foreach (var kv in context.Request.Headers)
+                {
+                    if (!NotForwardedWebSocketHeaders.Contains(kv.Key))


OrdinalIgnoreCase

Tratcher · 2017-02-22T16:50:06Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+                var wsScheme = string.Equals(_options.Scheme, "https", StringComparison.OrdinalIgnoreCase) ? "wss" : "ws";
+                var uriString = $"{wsScheme}://{_options.Host}:{_options.Port}{context.Request.PathBase}{context.Request.Path}{context.Request.QueryString}";
+
+                await client.ConnectAsync(new Uri(uriString), CancellationToken.None);


This will need some error handling. Not every WebSocket request is accepted. E.g. some get rejected with 401 for auth.

Use the HttpContext.RequestAborted CancellationToken.

I agree about error handling. Fixed it. However full implementation requires more than netstandard13 as WebException is not available there. So I created a generic implementation with 400 for start. After we figure out what to do with this WebException I can uncomment and add full error handling with entire HttpResponse forwarding.

Tratcher · 2017-02-22T16:52:54Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+            var buffer = new byte[4096];
+            while (true)
+            {
+                var res = await source.ReceiveAsync(new ArraySegment<byte>(buffer), CancellationToken.None);


HttpContext.RequestAborted

Tratcher · 2017-02-22T16:55:13Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+                var res = await source.ReceiveAsync(new ArraySegment<byte>(buffer), CancellationToken.None);
+                if (res.MessageType == WebSocketMessageType.Close)
+                {
+                    await dest.CloseAsync(source.CloseStatus.Value, source.CloseStatusDescription, CancellationToken.None);


CloseOutputAsync.

I was considering this one before. I am not 100% sure which one is correct here... Assuming that CloseOutputAsync is similar to socket shutdown this is the proper choice. But both options are not optimally documented.

CloseOutputAsync only sends a close. CloseAsync sends a close and then starts a read loop waiting for a close response. Since you have a separate thread handling the communication the other direction, you only want CloseOutputAsync in each thread.

That explains a lot (and complies with what I found in ManagedWebSocket source code) I got mislead by a .... stack overflow answer again. Shame on me :-(. Thank you Chris!

Added RequestAborted cancellation token support Added basic error handling for connect Replaced CloseAsync with CloseOutputAsync

mkosieradzki · 2017-02-22T22:58:25Z

@Tratcher Thank you very much for code review! I have addressed your concerns.

mikaelm12

Couple of things. Thanks for the contribution!

mikaelm12 · 2017-02-23T05:58:50Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

@@ -8,6 +8,9 @@
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Options;
+using System.Threading;


nit:Can you sort these using statements. Keeping the Microsoft namespaces after System though

mikaelm12 · 2017-02-23T06:00:41Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+            {
+                foreach (var kv in context.Request.Headers)
+                {
+                    if (!NotForwardedWebSocketHeaders.Contains(kv.Key, StringComparer.OrdinalIgnoreCase))


Can we use a better iterator variable name. Maybe headerEntry or something else

mikaelm12 · 2017-02-23T06:10:33Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+
+        private static async Task PumpWebSocket(WebSocket source, WebSocket dest, CancellationToken ct)
+        {
+            var buffer = new byte[4096];


Hmm. Maybe we should think about making the buffer size configurable.

I agree. Done.

Renamed iterator

mkosieradzki · 2017-02-23T11:13:59Z

@mikaelm12 Thank you for your code review. I have addressed your concerns.

davidfowl · 2017-02-23T11:16:56Z

src/Microsoft.AspNetCore.Proxy/ProxyOptions.cs

+        public int? WebSocketBufferSize
+        {
+            get => _webSocketBufferSize;
+            set => _webSocketBufferSize = value.HasValue && value.Value <= 0 ? throw new ArgumentOutOfRangeException(nameof(value)) : value;


This looks pretty unreadable. I'd split it across lines.

Done - your project your rules :).

mkosieradzki · 2017-02-23T11:40:55Z

@davidfowl Thanks. I have rewritten this line.

mikaelm12

One last thing regarding the AppVeyor failure.

mikaelm12 · 2017-02-23T17:41:58Z

samples/Microsoft.AspNetCore.Proxy.Samples/Microsoft.AspNetCore.Proxy.Samples.csproj

@@ -1,7 +1,7 @@
-<Project Sdk="Microsoft.NET.Sdk.Web">
+<Project Sdk="Microsoft.NET.Sdk.Web">



So currently AppVeyor fails with a Missing Method Exception.
You can add

<PropertyGroup>  <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects> <GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType> </PropertyGroup>`

That's our current work around and that should fix it.

Done. btw. Should we remove:

 <RuntimeIdentifier Condition=" '$(TargetFramework)' != 'netcoreapp1.1' ">win7-x64</RuntimeIdentifier>

as it seems to be already resolved? (Just asking not sure about this)

mkosieradzki · 2017-02-23T20:54:48Z

Done

mikaelm12 · 2017-02-23T22:54:42Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

@@ -1,22 +1,28 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using Microsoft.AspNetCore.Builder;


Sorry if this wasn't clear earlier but we put the all System namespaces before the Microsoft namespaces. After this, it should be good to merge.

No. It's my bad. My apologies. I've brainlessly used VS 2017 autosort refactoring.

The default settings are bad... Not your fault. You can change them here:

@davidfowl Thanks for that tip :). I've changed it.

mikaelm12

Sorry, I kind of jumped the gun on the approval. I was about to merge until I realized that there are no tests. We're gonna need some test coverage before this goes in.

mkosieradzki · 2017-02-23T23:32:02Z

https://github.com/aspnet/WebSockets/ this is similar case. They use Autobahn Testsuite.

mkosieradzki · 2017-02-23T23:41:08Z

@anurse Is there any chance you could help with porting automated tests from WebSockets repo?
Or maybe somehow integrate testing between those two repos.
@mikaelm12 Would you agree that porting entire testing infrastructure from WebSockets repo would deserve a separate PR?

analogrelay · 2017-02-23T23:52:20Z

I can certainly help review it. The code in the WebSockets repo should migrate reasonably well. We use it in aspnet/SignalR as well and it migrated well. We may want to consider abstracting it into a separate assembly so that we don't have to keep duplicating it, but I don't really have the cycles to work on that right now :)

analogrelay · 2017-02-23T23:53:01Z

I'd definitely suggest setting up some kind of simple smoke-test before merging this PR, but a full conformance test could go in a separate PR.

mkosieradzki · 2017-02-24T00:01:04Z

@anurse Thanks a lot for your response. I understand your lack of "cycles" and appreciate your effort in SignalR and different projects :). Smoke-test here sounds like a good idea. I will try to implement such.

Regarding full conformance tests I am VSTS-guy and have no hands-on experience with AppVeyor and I cannot commit to porting full test suite now or later, as it would take me way too much time.

mikaelm12 · 2017-02-24T06:44:54Z

No worries @mkosieradzki 😄 . I'll merge this as soon as I get one more approval

davidfowl · 2017-02-24T06:45:39Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+            var buffer = new byte[_options.WebSocketBufferSize ?? DefaultWebSocketBufferSize];
+            while (true)
+            {
+                var res = await source.ReceiveAsync(new ArraySegment<byte>(buffer), ct);


nit: result instead of res

davidfowl · 2017-02-24T06:45:53Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+            context.Response.StatusCode = 400;
+        }
+
+        private async Task PumpWebSocket(WebSocket source, WebSocket dest, CancellationToken ct)


nit: cancellationToken instead of ct

davidfowl · 2017-02-24T06:47:55Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+            var buffer = new byte[_options.WebSocketBufferSize ?? DefaultWebSocketBufferSize];
+            while (true)
+            {
+                var res = await source.ReceiveAsync(new ArraySegment<byte>(buffer), ct);


Should we be handling operation cancelled exceptions here? If the client should that really 500?

I partially agree: For receive error there should be websocket closure with InternalServerError instead of 500.
I have pushed fixes (I also have renamed some more variables to match your conventions).

Improved variable naming

…(1001)

mkosieradzki · 2017-02-24T10:06:32Z

@davidfowl I have ended up handling OperationCanceledException in the Receive. In that case I close the other socket with EndpointUnavailable (1001 https://tools.ietf.org/html/rfc6455#page-45) to ensure the closure sequence is initiated property with proper error.

Alternatively we can send 1001 from proxy to server and 1011 from proxy to client if it makes more sense.

I've decided not to handle it with during ConnectAsync as it will be (should be) handled by the infrastructure which induced AbortRequest.

mkosieradzki · 2017-02-27T23:01:19Z

@mikaelm12 Do you need anything else from me here?

mikaelm12 · 2017-02-28T00:06:59Z

I need to do some verification for this on my side. We had some other high pri tasks last week so this had to wait. But don't worry, this hasn't been forgotten 😄
And thanks for the contributions!

mikaelm12 · 2017-02-28T00:30:27Z

Seems like the sample is no longer working... I'm looking into it

mkosieradzki · 2017-02-28T00:31:51Z

@mikaelm12 You need to run it from Kestrel. There are some problems with IIS Express.

Tratcher · 2017-02-28T19:44:02Z

src/Microsoft.AspNetCore.Proxy/ProxyMiddleware.cs

+                if (result.MessageType == WebSocketMessageType.Close)
+                {
+                    await destination.CloseOutputAsync(source.CloseStatus.Value, source.CloseStatusDescription, cancellationToken);
+                    return;


nit: this return and the following else are redundant, either way your at the end of the method.

I have fixed this in #46

mikaelm12 · 2017-03-01T01:09:10Z

test/Microsoft.AspNetCore.Proxy.Test/WebSocketsTest.cs

+        }
+
+        [Fact]
+        public async Task ProxyWebSocketsSmokeTest()


@Tratcher @davidfowl Shouldn't this have a skip condition for Win7 and Windows Server 2008?

Just for reference, we also do this in the EndToEnd tests here

Thank you for the reference, very helpful!

mikaelm12 · 2017-03-01T04:33:20Z

As Kestrel is not an edge server, we can't in good faith add a feature that only works with Kestrel. It needs to work with IIS before being merged.

mkosieradzki · 2017-03-01T12:53:04Z

@mikaelm12 I have fixed broken example. Sample project works as before.

Basic HTTP works properly with Kestrel, HttpSys (former WebListener) and IIS Express.

BTW. HttpSys is a proper edge server.

Known issue is #47 . Due to breaking changes in System.Net.HttpClient.
Another known issue is that IIS Express does not seem to correctly perform WebSockets closing.

When it receives Close message it kills the entire connection instead of completing proper close handshake. It seems to be an issue with AspNetCoreModule, it requires further investigation and after this PR is merged I will start some investigation and probably open an issue with AspNetCoreModule.

However IIS-Express support might get broken later by the workaround for #47.

mkosieradzki · 2017-03-01T13:31:19Z

OK. I did some investigation and have opened: aspnet/AspNetCoreModule#77 .

Removed workarounds for aspnet#47

mikaelm12

Tested with a small app that sends websockets requests to the proxy which forwards to a websockets echo end point and it seems to work.

Added support for proxying websockets.

cbb7e17

Upgraded .NET Framework to 4.6 Updated example to use web sockets on Kestrel

dnfclas added the cla-already-signed label Feb 22, 2017

Tratcher reviewed Feb 22, 2017

View reviewed changes

mkosieradzki added 2 commits February 22, 2017 23:50

Added ping/pong configuration

fb8e43b

Added RequestAborted cancellation token support Added basic error handling for connect Replaced CloseAsync with CloseOutputAsync

Added comparison specification.

13ed159

Tratcher requested a review from mikaelm12 February 22, 2017 22:57

Tratcher assigned mikaelm12 Feb 22, 2017

mikaelm12 reviewed Feb 23, 2017

View reviewed changes

Added configuration option for WebSocketBufferSize

316a1ce

Renamed iterator

davidfowl reviewed Feb 23, 2017

View reviewed changes

Replaced throw expression with imperative check.

9ebb2e3

mikaelm12 suggested changes Feb 23, 2017

View reviewed changes

Attempt to fix AppVeyor build

07e4d8d

mikaelm12 reviewed Feb 23, 2017

View reviewed changes

Fixed header ordering

f618602

mikaelm12 approved these changes Feb 23, 2017

View reviewed changes

mikaelm12 suggested changes Feb 23, 2017

View reviewed changes

mkosieradzki mentioned this pull request Feb 24, 2017

Refactoring ProxyMiddleware #44

Closed

Removed dead code

490e38b

mkosieradzki mentioned this pull request Feb 24, 2017

WebSocket connect error is not forwarded #45

Closed

davidfowl reviewed Feb 24, 2017

View reviewed changes

mkosieradzki added 3 commits February 24, 2017 10:35

Improved error handling

dade44f

Improved variable naming

Removed the unnecessary handling of OperationCancelledException

780e228

Changed the internal server error (1011) to the endpoint unavailable …

c24f095

…(1001)

Merge remote-tracking branch 'upstream/dev' into dev

b5a86cd

mkosieradzki mentioned this pull request Feb 24, 2017

Added dynamic routing and redesigned API #46

Merged

Tratcher reviewed Feb 28, 2017

View reviewed changes

Tratcher approved these changes Feb 28, 2017

View reviewed changes

mikaelm12 reviewed Mar 1, 2017

View reviewed changes

Fixed references to make example project work again.

0690fde

mkosieradzki added 2 commits March 1, 2017 14:01

Added Skipping WebSockets tests for platforms without WebSockets client

e742290

Removed redundant else.

be98bc6

mkosieradzki added a commit to mkosieradzki/Proxy that referenced this pull request Mar 1, 2017

Merged changes from aspnet#41

ad8c3c2

Removed workarounds for aspnet#47

mkosieradzki mentioned this pull request Mar 1, 2017

Fixed Cookie support on .NET 4.6 #49

Closed

mikaelm12 approved these changes Mar 1, 2017

View reviewed changes

mikaelm12 merged commit 5d5b6c3 into aspnet:dev Mar 1, 2017

mkosieradzki mentioned this pull request Mar 4, 2017

[Reverse Proxy] WebSockets shutdown handshake is probably wrong microsoft/service-fabric-issues#183

Closed

		@@ -1,7 +1,7 @@
		<Project Sdk="Microsoft.NET.Sdk.Web">
		<Project Sdk="Microsoft.NET.Sdk.Web">

Added support for WebSockets #41

Added support for WebSockets #41

Conversation

mkosieradzki commented Feb 22, 2017

dnfclas commented Feb 22, 2017

Tratcher commented Feb 22, 2017

Tratcher commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkosieradzki commented Feb 22, 2017

mikaelm12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkosieradzki commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkosieradzki commented Feb 23, 2017

mikaelm12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkosieradzki Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

mkosieradzki commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 left a comment

Choose a reason for hiding this comment

mkosieradzki commented Feb 23, 2017

mkosieradzki commented Feb 23, 2017

analogrelay commented Feb 23, 2017

analogrelay commented Feb 23, 2017

mkosieradzki commented Feb 24, 2017 • edited Loading

mikaelm12 commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkosieradzki commented Feb 24, 2017 • edited Loading

mkosieradzki commented Feb 27, 2017

mikaelm12 commented Feb 28, 2017

mikaelm12 commented Feb 28, 2017

mkosieradzki commented Feb 28, 2017

Tratcher Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 commented Mar 1, 2017

mkosieradzki commented Mar 1, 2017 • edited Loading

mkosieradzki commented Mar 1, 2017

mikaelm12 left a comment

Choose a reason for hiding this comment

mkosieradzki Feb 23, 2017 •

edited

Loading

mkosieradzki commented Feb 24, 2017 •

edited

Loading

mkosieradzki commented Feb 24, 2017 •

edited

Loading

Tratcher Feb 28, 2017 •

edited

Loading

mkosieradzki commented Mar 1, 2017 •

edited

Loading