[Blazor][Fixes #12788] Fixes blazor reconnection tests #12813

javiercn · 2019-08-01T18:51:42Z

Enables a customer websocket feature in tests that can be used to trigger errors on the server at will.
Enables conditionally turning errors through cookies so that normal tests aren't affected and run against the product bits.
Cleans up dead code in blazor.server.js that is not longer required.
Moves the reconnection tests out of ComponentsApp.App into basic test app.
- Where are my karma points for this @SteveSandersonMS?

src/Components/test/E2ETest/ServerExecutionTests/ServerAuthTest.cs

rynowak · 2019-08-01T19:07:05Z

src/Components/test/E2ETest/ServerExecutionTests/ServerReconnectionTest.cs

+
+            Browser.Manage().Cookies.DeleteCookieNamed("WebSockets.Identifier");
+            Browser.Manage().Cookies.AddCookie(new Cookie("WebSockets.Identifier", SessionIdentifier));
+            Browser.Navigate().Refresh();


Do we want to just replace this with navigation + reload? (line 34)

Did you mean line 32? I don't see why

I mean line 30. Do we need to navigate to the root and then trigger a refresh?

In the server-side case yes because the connection already got established by the time we set the cookies.

These tests don't run client-side.

src/Components/test/E2ETest/ServerExecutionTests/ServerReconnectionTest.cs

src/Components/test/E2ETest/ServerExecutionTests/ServerSideAppTest.cs

src/Components/test/testassets/BasicTestApp/ReconnectTicker.razor

src/Components/test/testassets/TestServer/Infrastructure/InterruptibleWebSocket.cs

rynowak · 2019-08-01T19:11:38Z

src/Components/test/testassets/TestServer/Infrastructure/InterruptibleWebSocket.cs

+                return await await Task.WhenAny(
+                    _disabledReceiveTask.Task,
+                    Socket.ReceiveAsync(buffer, cancellationToken));
+            }


@anurse @BrennanConroy

...ents/test/testassets/TestServer/Infrastructure/InterruptibleWebSocketAppBuilderExtensions.cs

src/Components/test/testassets/TestServer/Infrastructure/InterruptibleWebSocket.cs

rynowak · 2019-08-01T19:13:10Z

src/Components/test/testassets/TestServer/Infrastructure/InterruptibleWebSocket.cs

+
+namespace Components.TestServer.Infrastructure
+{
+    public class InterruptibleWebSocket : WebSocket


I think this deserves some comments and how it works and why it's needed.

I'll add a comment explaining how this works

Definitely. Without studying this in detail, I have absolutely no idea why this is the solution to our problems. I don't doubt that it works, but I can't guess whether this is exploiting some random implementation detail of SignalR that might change at any time, or whether it's fundamental to the idea of WebSockets that they can be "interrupted" in some officially understood way.

Added some comments.

it's fundamental to the idea of WebSockets that they can be "interrupted" in some officially understood way.

Is exactly that, if we throw an exception from the transport that signalr is not aware of, it will propagate it. I chose to throw an IOException but I could have chosen to throws something else. The one that signalr catches and handles gracefully is WebsocketException when reason = ClosedWithoutSendingCloseFrame

The one that signalr catches and handles gracefully is WebsocketException when reason = ClosedWithoutSendingCloseFrame

This is a behavior we've gone back and forth about. Customers have been adamant that this kind of error is too noisy, but it still feels wrong that we suppress it. The reality is that any failure to complete the closing process is considered an error by the spec.

Also, the underlying WebSocket implementation won't ever actually produce IOExceptions. It explicitly wraps them in WebSocketExceptions with the error code ConectionClosedPrematurely.

Perhaps having a way for a Hub to indicate that it's willing to see even these more "noisy" failures is appropriate. For a system like Blazor it makes sense to want to tell the difference here. I think we should've left this error in the OnDisconnectedAsync handler instead of suppressing it. Maybe 3.0 can justify changing that behavior but we'd need quite a bit of backup to get that through ask mode.

I do have an alternative for graceful reconection that uses the send beacon API
#12853

@anurse This might be something you folks want to consider doing in signalr directly as it turns out it works pretty well, and would give you the ability to signal the end of the connection in most/all transports. (Given that you have a ping for all of them).

This might be something you folks want to consider doing in signalr directly as it turns out it works pretty well

Yep, we've talked about using sendBeacon. In the current landscape I think it's entirely appropriate for Blazor to use it because of the limitations I described above (SignalR suppresses ungraceful disconnect errors in WebSockets).

However, I do think that the WebSocket close handshake (or lack thereof) should be enough for SignalR to distinguish graceful vs ungraceful disconnects. In fact, it is, but SignalR doesn't manifest it properly to a consumer (such as Blazor). In those cases, sendBeacon shouldn't even be necessary. A little more investigation may be needed (perhaps you've already done it and I didn't find the results) since I don't know off-hand if the close handshake works when the browser closes the tab, but we (SignalR) should be doing that work.

Anyway, glad to hear you have an alternative in the works. The rest is on us I think.

src/Components/test/testassets/TestServer/Infrastructure/InterruptibleWebSocketFeature.cs

SteveSandersonMS · 2019-08-02T09:11:34Z

Where are my karma points for this @SteveSandersonMS?

💯 🥇

src/Components/test/E2ETest/ServerExecutionTests/CircuitGracefulTerminationTests.cs

SteveSandersonMS · 2019-08-02T09:36:57Z

This may well be good (though to be clear, I'll await more explanation before thinking I understand it).

However, do you think you'd be able to check the current real-world behavior of the product code before we consider this to be a solution?

analogrelay · 2019-08-03T06:17:11Z

However, do you think you'd be able to check the current real-world behavior of the product code before we consider this to be a solution?

I think this is the right approach. We need to dive in to the real-world behavior and SignalR likely also needs to consider changes here.

javiercn · 2019-08-03T17:32:16Z

I rebased this PR and removed all the copied websocket code. This should be good to go. Independently of what we end up doing with graceful disconnection, we want to take this PR as it offers advantages for testing reconnection. (Mainly no test stuff in product code) and cleans up our tests.

rynowak · 2019-08-03T19:18:47Z

src/Components/test/E2ETest/Tests/AuthTest.cs

+            Browser.Manage().Cookies.DeleteCookieNamed("WebSockets.Identifier");
+            Browser.Manage().Cookies.AddCookie(new Cookie("WebSockets.Identifier", SessionIdentifier));
+            Browser.Navigate().Refresh();
+


Is there a drawback to this running for the client-side tests?

These test don't run on the client, when they do, we can simply key this off to whether or not we are running server-side.

rynowak · 2019-08-03T19:20:25Z

src/Shared/E2ETesting/WaitAssert.cs

+            string script,
+            Func<object, T> converter = null)
+        {
+            if(!script.StartsWith("return "))


rynowak · 2019-08-03T19:20:51Z

src/Shared/E2ETesting/WaitAssert.cs

+            if(!script.StartsWith("return "))
+            {
+                script = $"return {script}";
+            }


Do we need this level of cleverness?

Yes, because it saves you from a common pitfall. (I spent like 1h on this the other day, couldn't figure out why I was only seeing null)

rynowak

This seems like a much more reasonable set of pieces to maintain. Thanks for digging into this further.

Is the idea that using fault injection in the websocket is a much better approach than forcing the client-side code to disconnect? What are the advantages?

javiercn · 2019-08-03T19:24:03Z

Is the idea that using fault injection in the websocket is a much better approach than forcing the client-side code to disconnect? What are the advantages?

No test code in product code
We can decide for how long we fail
The failure pattern, etc.

rynowak · 2019-08-03T19:39:27Z

No test code in product code

This was true before too.

Can you go into more detail about the other things?

javiercn · 2019-08-03T19:46:18Z

This was true before too.

No it wasn't

https://github.com/aspnet/AspNetCore/pull/12813/files#diff-fdbaeed1d036d944c000f3ef1aa7d066L92

Can you go into more detail about the other things?

We simply disable the socket completely right now, but with this approach we can prevent the client from reconnecting as many times as we want, or force intermittent re-connections.

By controlling it on the server we get more a more realistic scenario than triggering the connection close from the client side. We could for example choose to trigger the connection failure at different points.

rynowak · 2019-08-03T21:25:55Z

No it wasn't

Yeah, you're right! Sorry for mixing that up.

SteveSandersonMS · 2019-08-05T13:12:55Z

I'm guessing that whether or not we proceed with this PR depends on whether we're proceeding with #12853.

javiercn · 2019-08-05T13:40:34Z

@SteveSandersonMS I still want to get this one in, as I think it cleans-up test code from product code and that's worth it.

javiercn · 2019-08-05T13:41:09Z

@SteveSandersonMS It also represents the scenario more accurately than the client closing the connection.

SteveSandersonMS · 2019-08-05T14:51:08Z

If it's a tradeoff between:

Having the forceDisconnect method on _internal in the blazor.server.js
Or having all this infrastructure for interrupting a websocket connection

... then given how massively more complex #2 is I'm not totally convinced it's a pragmatic choice. The internals of how websockets work are meant to be transparent to us. If this capability existed in SignalR then sure we'd use it, but since it's not I'm wary about us owning it. Let's discuss in the sync later.

javiercn · 2019-08-05T21:20:46Z

Closing as we'll leave the test code as-is around and we have an alternative approach

javiercn requested a review from SteveSandersonMS as a code owner August 1, 2019 18:51

javiercn requested a review from rynowak August 1, 2019 18:51

javiercn force-pushed the javiercn/fix-reconnection-tests branch from 9648c6d to f1f7b49 Compare August 1, 2019 18:56

javiercn requested review from BrennanConroy and analogrelay August 1, 2019 18:56

rynowak reviewed Aug 1, 2019

View reviewed changes

src/Components/test/E2ETest/ServerExecutionTests/ServerAuthTest.cs Outdated Show resolved Hide resolved

rynowak reviewed Aug 1, 2019

View reviewed changes

Pilchie added the area-blazor Includes: Blazor, Razor Components label Aug 1, 2019

SteveSandersonMS reviewed Aug 2, 2019

View reviewed changes

src/Components/test/E2ETest/ServerExecutionTests/CircuitGracefulTerminationTests.cs Outdated Show resolved Hide resolved

SteveSandersonMS mentioned this pull request Aug 2, 2019

Understand when disconnections are graceful or not #12828

Closed

javiercn force-pushed the javiercn/fix-reconnection-tests branch from f1f7b49 to 84148c4 Compare August 2, 2019 12:28

javiercn added 5 commits August 3, 2019 10:11

Fix E2E tests

9a27516

Remove unnecessary test hook

a288a60

Address feedback

dbe2503

Cleanups

6fbb9c0

Updated with feedback

b38dabe

javiercn force-pushed the javiercn/fix-reconnection-tests branch from 84148c4 to b38dabe Compare August 3, 2019 17:25

rynowak reviewed Aug 3, 2019

View reviewed changes

src/Shared/E2ETesting/WaitAssert.cs

string script,

Func<object, T> converter = null)

{

if(!script.StartsWith("return "))

Copy link

Member

rynowak Aug 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format

rynowak reviewed Aug 3, 2019

View reviewed changes

javiercn closed this Aug 5, 2019

dougbu deleted the javiercn/fix-reconnection-tests branch May 18, 2020 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Blazor][Fixes #12788] Fixes blazor reconnection tests #12813

[Blazor][Fixes #12788] Fixes blazor reconnection tests #12813

javiercn commented Aug 1, 2019

rynowak Aug 1, 2019

javiercn Aug 1, 2019

rynowak Aug 3, 2019

javiercn Aug 3, 2019

rynowak Aug 1, 2019

rynowak Aug 1, 2019

javiercn Aug 1, 2019

SteveSandersonMS Aug 2, 2019

javiercn Aug 2, 2019

analogrelay Aug 3, 2019

javiercn Aug 3, 2019

analogrelay Aug 4, 2019

SteveSandersonMS commented Aug 2, 2019

SteveSandersonMS commented Aug 2, 2019

analogrelay commented Aug 3, 2019

javiercn commented Aug 3, 2019

rynowak Aug 3, 2019

javiercn Aug 3, 2019

rynowak Aug 3, 2019

rynowak Aug 3, 2019

javiercn Aug 3, 2019

rynowak left a comment

javiercn commented Aug 3, 2019

rynowak commented Aug 3, 2019

javiercn commented Aug 3, 2019

rynowak commented Aug 3, 2019

SteveSandersonMS commented Aug 5, 2019

javiercn commented Aug 5, 2019

javiercn commented Aug 5, 2019

SteveSandersonMS commented Aug 5, 2019

javiercn commented Aug 5, 2019 •

edited

Loading

[Blazor][Fixes #12788] Fixes blazor reconnection tests #12813

[Blazor][Fixes #12788] Fixes blazor reconnection tests #12813

Conversation

javiercn commented Aug 1, 2019

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

Choose a reason for hiding this comment

SteveSandersonMS commented Aug 2, 2019

SteveSandersonMS commented Aug 2, 2019

analogrelay commented Aug 3, 2019

javiercn commented Aug 3, 2019

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

rynowak left a comment

Choose a reason for hiding this comment

javiercn commented Aug 3, 2019

rynowak commented Aug 3, 2019

javiercn commented Aug 3, 2019

rynowak commented Aug 3, 2019

SteveSandersonMS commented Aug 5, 2019

javiercn commented Aug 5, 2019

javiercn commented Aug 5, 2019

SteveSandersonMS commented Aug 5, 2019

javiercn commented Aug 5, 2019 • edited Loading

javiercn commented Aug 5, 2019 •

edited

Loading