Client reconnects when state's available on the server #7395

pranavkm · 2019-02-08T19:21:18Z

Do a reconnect if we have state on the server

Fixes #7537

pranavkm · 2019-02-12T15:14:30Z

Ping

src/Components/Browser.JS/src/Boot.Server.ts

src/Components/Browser.JS/src/Platform/Circuits/CircuitHandler.ts

src/Components/Server/src/Circuits/CircuitRegistry.cs

src/Components/test/testassets/ComponentsApp.Server/wwwroot/index.html

SteveSandersonMS · 2019-02-12T16:11:33Z

@pranavkm Hope you don't mind the big pile of comments on the TypeScript! Besides that stylistic stuff, this really looks like it's heading in the right direction 👍

pranavkm

🆙 📅 There's a minimal UI that attempts to do an auto-reconnect.

src/Components/Browser.JS/src/Boot.Server.ts

src/Components/Browser.JS/src/Platform/Circuits/AutoReconnectCircuitHandler.ts

pranavkm · 2019-02-14T15:20:02Z

src/Components/Browser.JS/src/Platform/Circuits/AutoReconnectCircuitHandler.ts

+    this.intervalHandle = null;
+  }
+  onConnectionUp() {
+    this.modal.style.display = 'none';


Testing this is going to be tricky. With a local host, reconnect simulated using a forced disconnect is almost instant. I could try adding another handler that does busy waiting until the test has an opportunity to inspect things. Any suggestions on a better plan to test the UI?

Since we have an internal-for-testing method _internal.forceCloseConnection, we could go further with that pattern and change it to a pair of methods like _internal.goOffline (disconnects, and sets a flag that makes reconnect into a no-op), and _internal.goOnline (clears the flag so that reconnect now does its normal thing).

Unresolving just so you see this. Totally understand if you don't feel there's time to change this further before preview 3!

Another possibility I've looked into, but don't have a definitive answer about, is whether Selenium can make the browser switch into "offline" mode then back "online" later. That would be ideal, but people on the internets seem disagreed about whether this is supported and what the code to do it would be.

Would something like this work?

Extend ComponentHub and add an additional InterruptConnection method.

Have that method close the connection (as it's the same client proxy).

Register that on the functional tests.

Call that from your code to trigger it.

rynowak · 2019-02-15T06:06:26Z

I'm going to limit myself to looking at the C# part of this because steve has that other stuff on lock.

src/Components/Server/src/Circuits/DelegatingClientProxy.cs

src/Components/Server/src/Circuits/DisconnectedCircuitRegistry.cs

src/Components/Server/src/ComponentsHub.cs

src/Components/Browser.JS/jest.config.js

pranavkm · 2019-02-27T22:07:32Z

🆙 📅

rynowak · 2019-03-04T21:04:44Z

src/Components/Server/src/Circuits/CircuitHandler.cs

+        /// While <see cref="OnConnectionUpAsync(Circuit, CancellationToken)"/> is always invoked when a client
+        /// connects to the server, <see cref="OnConnectionDownAsync(Circuit, CancellationToken)"/> may not be invoked
+        /// if a client reconnects prior to the server identifying a client disconnect.
+        /// </remarks>


@pranavkm - per offline discussion revert this

rynowak · 2019-03-04T21:07:18Z

src/Components/Server/src/Circuits/CircuitHost.cs

+
+        private async Task OnCircuitDownAsync()
+        {
+            for (var i = 0; i < _circuitHandlers.Length; i++)


this isn't protected by the lock.. is this an issue? or is this protected by the design of the circuit pool?

Neither of the circuit events have concurrency issues - the server controls when a circuit is being discarded so it doesn't need protection.

rynowak · 2019-03-04T21:10:35Z

src/Components/Server/src/Circuits/CircuitRegistry.cs

+                if (circuitHost == null)
+                {
+                    // Failed to connect. Nothing to do here.
+                    return null;


null task :( that's pretty unconventional.

It's async method, so it's a null value (not null Task).

rynowak · 2019-03-04T21:10:58Z

src/Components/Server/src/Circuits/CircuitRegistry.cs

+                    {
+                        // During reconnects, we may transition from Connect->Connect i.e.without ever having invoking OnConnectionDownAsync during
+                        // a formal client disconnect. To allow authors of CircuitHandlers to have reasonable expectations will pair the connection up with a connection down.
+                        await circuitHost.OnConnectionDownAsync(cancellationToken);


analogrelay · 2019-03-04T21:46:34Z

Helix failure is https://github.com/aspnet/AspNetCore-Internal/issues/1879 - Unrelated.
AspNetCore-ci failure is an out of date ref-asm file since a public API was added.

analogrelay · 2019-03-04T22:27:01Z

/azp run AspNetCore-helix-test

azure-pipelines · 2019-03-04T22:27:14Z

Azure Pipelines successfully started running 1 pipeline(s).

pranavkm · 2019-03-05T01:19:43Z

@anurse any idea what failed with the helix run? All I see is

E:\A\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19105.2\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(71,5): error : Job '44eeb84b-c926-4086-9905-d39616de4913' has 1 failed work items. [E:\A\_work\1\s\eng\helix\helix.proj]
E:\A\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19105.2\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(71,5): error : Job '1861a084-5a19-46c4-af02-0fabc5334641' has 1 failed work items. [E:\A\_work\1\s\eng\helix\helix.proj]

analogrelay · 2019-03-05T16:36:49Z

For future reference, click any of the mc.dot.net links above those messages (they're all the same) when they occur to get to a screen that lets you drill down into failures.

#8094 is tracking work to get this data better integrated into AzDO

pranavkm requested a review from SteveSandersonMS as a code owner February 8, 2019 19:21

pranavkm requested a review from rynowak February 8, 2019 19:21

Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 10, 2019