[Blazor][Fixes #12054] ComponentHub reliability improvements.

javiercn · javiercn · commit e8c99b2d6b61 · 2019-07-19T02:48:32.000-07:00
* Validates StartCircuit is called once per circuit.
* NOOPs when other hub methods are called before start circuit and
  returns an error to the client.
diff --git a/src/Components/Server/src/ComponentHub.cs b/src/Components/Server/src/ComponentHub.cs
@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Runtime.CompilerServices;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Components.Server.Circuits;
 using Microsoft.AspNetCore.Http;
@@ -73,6 +74,13 @@ public override Task OnDisconnectedAsync(Exception exception)
         /// </summary>
         public string StartCircuit(string uriAbsolute, string baseUriAbsolute)
         {
+            if (CircuitHost != null)
+            {
+                Log.CircuitAlreadyInitialized(_logger, CircuitHost.CircuitId);
+                NotifyClientError(Clients.Caller, $"The circuit host '{CircuitHost.CircuitId}' has already been initialized.");
+                return null;
+            }
+
             var circuitClient = new CircuitClientProxy(Clients.Caller, Context.ConnectionId);
             if (DefaultCircuitFactory.ResolveComponentMetadata(Context.GetHttpContext(), circuitClient).Count == 0)
             {
@@ -129,26 +137,60 @@ public async Task<bool> ConnectCircuit(string circuitId)
         /// </summary>
         public void BeginInvokeDotNetFromJS(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson)
         {
-            _ = EnsureCircuitHost().BeginInvokeDotNetFromJS(callId, assemblyName, methodIdentifier, dotNetObjectId, argsJson);
+            if (CircuitHost == null)
+            {
+                Log.CircuitHostNotInitialized(_logger);
+                _ = NotifyClientError(Clients.Caller, "Circuit not initialized.");
+                return;
+            }
+
+            _ = CircuitHost.BeginInvokeDotNetFromJS(callId, assemblyName, methodIdentifier, dotNetObjectId, argsJson);
         }
 
+        /// <summary>
+        /// Intended for framework use only. Applications should not call this method directly.
+        /// </summary>
         public void EndInvokeJSFromDotNet(long asyncHandle, bool succeeded, string arguments)
         {
-            _ = EnsureCircuitHost().EndInvokeJSFromDotNet(asyncHandle, succeeded, arguments);
+            if (CircuitHost == null)
+            {
+                Log.CircuitHostNotInitialized(_logger);
+                _ = NotifyClientError(Clients.Caller, "Circuit not initialized.");
+                return;
+            }
+
+            _ = CircuitHost.EndInvokeJSFromDotNet(asyncHandle, succeeded, arguments);
         }
 
+        /// <summary>
+        /// Intended for framework use only. Applications should not call this method directly.
+        /// </summary>
         public void DispatchBrowserEvent(string eventDescriptor, string eventArgs)
         {
-            _ = EnsureCircuitHost().DispatchEvent(eventDescriptor, eventArgs);
+            if (CircuitHost == null)
+            {
+                Log.CircuitHostNotInitialized(_logger);
+                _ = NotifyClientError(Clients.Caller, "Circuit not initialized.");
+                return;
+            }
+
+            _ = CircuitHost.DispatchEvent(eventDescriptor, eventArgs);
         }
 
         /// <summary>
         /// Intended for framework use only. Applications should not call this method directly.
         /// </summary>
         public void OnRenderCompleted(long renderId, string errorMessageOrNull)
         {
+            if (CircuitHost == null)
+            {
+                Log.CircuitHostNotInitialized(_logger);
+                NotifyClientError(Clients.Caller, "Circuit not initialized.");
+                return;
+            }
+
             Log.ReceivedConfirmationForBatch(_logger, renderId);
-            EnsureCircuitHost().Renderer.OnRenderCompleted(renderId, errorMessageOrNull);
+            CircuitHost.Renderer.OnRenderCompleted(renderId, errorMessageOrNull);
         }
 
         private async void CircuitHost_UnhandledException(object sender, UnhandledExceptionEventArgs e)
@@ -161,14 +203,14 @@ private async void CircuitHost_UnhandledException(object sender, UnhandledExcept
                 Log.UnhandledExceptionInCircuit(_logger, circuitId, (Exception)e.ExceptionObject);
                 if (_options.DetailedErrors)
                 {
-                    await circuitHost.Client.SendAsync("JS.Error", e.ExceptionObject.ToString());
+                    await NotifyClientError(circuitHost.Client, e.ExceptionObject.ToString());
                 }
                 else
                 {
                     var message = $"There was an unhandled exception on the current circuit, so this circuit will be terminated. For more details turn on " +
                         $"detailed exceptions in '{typeof(CircuitOptions).Name}.{nameof(CircuitOptions.DetailedErrors)}'";
 
-                    await circuitHost.Client.SendAsync("JS.Error", message);
+                    await NotifyClientError(circuitHost.Client, message);
                 }
 
                 // We generally can't abort the connection here since this is an async
@@ -181,17 +223,8 @@ private async void CircuitHost_UnhandledException(object sender, UnhandledExcept
             }
         }
 
-        private CircuitHost EnsureCircuitHost()
-        {
-            var circuitHost = CircuitHost;
-            if (circuitHost == null)
-            {
-                var message = $"The {nameof(CircuitHost)} is null. This is due to an exception thrown during initialization.";
-                throw new InvalidOperationException(message);
-            }
-
-            return circuitHost;
-        }
+        private static Task NotifyClientError(IClientProxy client, string error) =>
+            client.SendAsync("JS.Error", error);
 
         private static class Log
         {
@@ -207,6 +240,13 @@ private static class Log
             private static readonly Action<ILogger, string, Exception> _failedToTransmitException =
                 LoggerMessage.Define<string>(LogLevel.Debug, new EventId(4, "FailedToTransmitException"), "Failed to transmit exception to client in circuit {CircuitId}");
 
+            private static readonly Action<ILogger, string, Exception> _circuitAlreadyInitialized =
+                LoggerMessage.Define<string>(LogLevel.Debug, new EventId(5, "CircuitAlreadyInitialized"), "The circuit host '{CircuitId}' has already been initialized");
+
+            private static readonly Action<ILogger, string, Exception> _circuitHostNotInitialized =
+                LoggerMessage.Define<string>(LogLevel.Debug, new EventId(6, "CircuitHostNotInitialized"), "Call to '{CallSite}' received before the circuit host initialization.");
+
+
             public static void NoComponentsRegisteredInEndpoint(ILogger logger, string endpointDisplayName)
             {
                 _noComponentsRegisteredInEndpoint(logger, endpointDisplayName, null);
@@ -226,6 +266,10 @@ public static void FailedToTransmitException(ILogger logger, string circuitId, E
             {
                 _failedToTransmitException(logger, circuitId, transmissionException);
             }
+
+            public static void CircuitAlreadyInitialized(ILogger logger, string circuitId) => _circuitAlreadyInitialized(logger, circuitId, null);
+
+            public static void CircuitHostNotInitialized(ILogger logger, [CallerMemberName] string callSite = "") => _circuitHostNotInitialized(logger, callSite, null);
         }
     }
 }
diff --git a/src/Components/test/E2ETest/ServerExecutionTests/ComponentHubReliabilityTest.cs b/src/Components/test/E2ETest/ServerExecutionTests/ComponentHubReliabilityTest.cs
@@ -0,0 +1,120 @@
+// 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 System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Ignitor;
+using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures;
+using Microsoft.AspNetCore.Components.Rendering;
+using Microsoft.AspNetCore.SignalR.Client;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests
+{
+    public class ComponentHubReliabilityTest : IClassFixture<AspNetSiteServerFixture>
+    {
+        private static readonly TimeSpan DefaultLatencyTimeout = TimeSpan.FromMilliseconds(500);
+        private readonly AspNetSiteServerFixture _serverFixture;
+
+        public ComponentHubReliabilityTest(AspNetSiteServerFixture serverFixture)
+        {
+            serverFixture.BuildWebHostMethod = TestServer.Program.BuildWebHost;
+            _serverFixture = serverFixture;
+            CreateDefaultConfiguration();
+        }
+
+        public BlazorClient Client { get; set; }
+
+        private IList<Batch> Batches { get; set; } = new List<Batch>();
+        private IList<string> Errors { get; set; } = new List<string>();
+
+        private void CreateDefaultConfiguration()
+        {
+            Client = new BlazorClient() { DefaultLatencyTimeout = DefaultLatencyTimeout };
+            Client.RenderBatchReceived += (id, rendererId, data) => Batches.Add(new Batch(id, rendererId, data));
+            Client.OnCircuitError += (error) => Errors.Add(error);
+        }
+
+        [Fact]
+        public async Task CannotStartMultipleCircuits()
+        {
+            // Arrange
+            var expectedError = "The circuit host '.*?' has already been initialized.";
+            var rootUri = _serverFixture.RootUri;
+            var baseUri = new Uri(rootUri, "/subdir");
+            Assert.True(await Client.ConnectAsync(baseUri, prerendered: false), "Couldn't connect to the app");
+            Assert.Single(Batches);
+
+            // Act
+            await Client.ExpectCircuitError(() => Client.HubConnection.SendAsync(
+                "StartCircuit",
+                baseUri.GetLeftPart(UriPartial.Authority),
+                baseUri));
+
+            // Assert
+            var actualError = Assert.Single(Errors);
+            Assert.Matches(expectedError, actualError);
+        }
+
+        [Fact]
+        public async Task CannotInvokeJSInteropBeforeInitialization()
+        {
+            // Arrange
+            var expectedError = "Circuit not initialized.";
+            var rootUri = _serverFixture.RootUri;
+            var baseUri = new Uri(rootUri, "/subdir");
+            Assert.True(await Client.ConnectAsync(baseUri, prerendered: false, connectAutomatically: false));
+            Assert.Empty(Batches);
+
+            // Act
+            await Client.ExpectCircuitError(() => Client.HubConnection.SendAsync(
+                "BeginInvokeDotNetFromJS",
+                "",
+                "",
+                "",
+                0,
+                ""));
+
+            // Assert
+            var actualError = Assert.Single(Errors);
+            Assert.Equal(expectedError, actualError);
+        }
+
+        [Fact]
+        public async Task CannotInvokeOnRenderCompletedInitialization()
+        {
+            // Arrange
+            var expectedError = "Circuit not initialized.";
+            var rootUri = _serverFixture.RootUri;
+            var baseUri = new Uri(rootUri, "/subdir");
+            Assert.True(await Client.ConnectAsync(baseUri, prerendered: false, connectAutomatically: false));
+            Assert.Empty(Batches);
+
+            // Act
+            await Client.ExpectCircuitError(() => Client.HubConnection.SendAsync(
+                "OnRenderCompleted",
+                5,
+                null));
+
+            // Assert
+            var actualError = Assert.Single(Errors);
+            Assert.Equal(expectedError, actualError);
+        }
+
+        private class Batch
+        {
+            public Batch(int id, int rendererId, byte [] data)
+            {
+                Id = id;
+                RendererId = rendererId;
+                Data = data;
+            }
+
+            public int Id { get; }
+            public int RendererId { get; }
+            public byte[] Data { get; }
+        }
+    }
+}
diff --git a/src/Components/test/testassets/Ignitor/BlazorClient.cs b/src/Components/test/testassets/Ignitor/BlazorClient.cs
@@ -40,6 +40,8 @@ public BlazorClient()
 
         private CancellableOperation NextBatchReceived { get; set; }
 
+        private CancellableOperation NextErrorReceived { get; set; }
+
         private CancellableOperation NextJSInteropReceived { get; set; }
 
         private CancellableOperation NextDotNetInteropCompletionReceived { get; set; }
@@ -52,7 +54,7 @@ public BlazorClient()
 
         public event Action<string> DotNetInteropCompletion;
 
-        public event Action<Error> OnCircuitError;
+        public event Action<string> OnCircuitError;
 
         public string CircuitId { get; set; }
 
@@ -98,6 +100,18 @@ public Task PrepareForNextDotNetInterop()
             return NextDotNetInteropCompletionReceived.Completion.Task;
         }
 
+        public Task PrepareForNextCircuitError()
+        {
+            if (NextErrorReceived?.Completion != null)
+            {
+                throw new InvalidOperationException("Invalid state previous task not completed");
+            }
+
+            NextErrorReceived = new CancellableOperation(DefaultLatencyTimeout);
+
+            return NextErrorReceived.Completion.Task;
+        }
+
         public async Task ClickAsync(string elementId)
         {
             if (!Hive.TryFindElementById(elementId, out var elementNode))
@@ -139,6 +153,13 @@ public async Task ExpectDotNetInterop(Func<Task> action)
             await task;
         }
 
+        public async Task ExpectCircuitError(Func<Task> action)
+        {
+            var task = WaitForCircuitError();
+            await action();
+            await task;
+        }
+
         private Task WaitForRenderBatch(TimeSpan? timeout = null)
         {
             if (ImplicitWait)
@@ -180,7 +201,20 @@ private async Task WaitForDotNetInterop()
             }
         }
 
-        public async Task<bool> ConnectAsync(Uri uri, bool prerendered)
+        private async Task WaitForCircuitError()
+        {
+            if (ImplicitWait)
+            {
+                if (DefaultLatencyTimeout == null)
+                {
+                    throw new InvalidOperationException("Implicit wait without DefaultLatencyTimeout is not allowed.");
+                }
+
+                await PrepareForNextCircuitError();
+            }
+        }
+
+        public async Task<bool> ConnectAsync(Uri uri, bool prerendered, bool connectAutomatically = true)
         {
             var builder = new HubConnectionBuilder();
             builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IHubProtocol, IgnitorMessagePackHubProtocol>());
@@ -193,9 +227,14 @@ public async Task<bool> ConnectAsync(Uri uri, bool prerendered)
             HubConnection.On<int, string, string>("JS.BeginInvokeJS", OnBeginInvokeJS);
             HubConnection.On<string>("JS.EndInvokeDotNet", OnEndInvokeDotNet);
             HubConnection.On<int, int, byte[]>("JS.RenderBatch", OnRenderBatch);
-            HubConnection.On<Error>("JS.OnError", OnError);
+            HubConnection.On<string>("JS.Error", OnError);
             HubConnection.Closed += OnClosedAsync;
 
+            if (!connectAutomatically)
+            {
+                return true;
+            }
+
             // Now everything is registered so we can start the circuit.
             if (prerendered)
             {
@@ -264,9 +303,18 @@ private void OnRenderBatch(int browserRendererId, int batchId, byte[] batchData)
             }
         }
 
-        private void OnError(Error error)
+        private void OnError(string error)
         {
-            OnCircuitError?.Invoke(error);
+            try
+            {
+                OnCircuitError?.Invoke(error);
+
+                NextErrorReceived?.Completion?.TrySetResult(null);
+            }
+            catch (Exception e)
+            {
+                NextErrorReceived?.Completion?.TrySetResult(e);
+            }
         }
 
         private Task OnClosedAsync(Exception ex)
