From f568d119808468ff1582264f5cafeaa4c5866b51 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 27 Mar 2019 17:23:50 +0000 Subject: [PATCH 1/3] Make StartCircuit not block the SignalR message loop. Fixes #8274 --- .../Server/src/Circuits/CircuitHost.cs | 31 ++++++++++----- src/Components/Server/src/ComponentHub.cs | 5 +-- .../Server/test/Circuits/CircuitHostTest.cs | 39 ++++++++++++++++++- 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/Components/Server/src/Circuits/CircuitHost.cs b/src/Components/Server/src/Circuits/CircuitHost.cs index b8d479a555d9..13d12f726474 100644 --- a/src/Components/Server/src/Circuits/CircuitHost.cs +++ b/src/Components/Server/src/Circuits/CircuitHost.cs @@ -104,21 +104,32 @@ public Task> PrerenderComponentAsync(Type componentType, Par }); } - public async Task InitializeAsync(CancellationToken cancellationToken) + public void Initialize(CancellationToken cancellationToken) { - await Renderer.InvokeAsync(async () => + // This Initialize method is fire-and-forget as far as the caller is concerned, because + // if it was to await completion, it would be blocking the SignalR message loop. This could + // lead to deadlock, e.g., if the init process itself waited for an incoming SignalR message + // such as the result of a JSInterop call. + Renderer.InvokeAsync(async () => { - SetCurrentCircuitHost(this); - - for (var i = 0; i < Descriptors.Count; i++) + try { - var (componentType, domElementSelector) = Descriptors[i]; - await Renderer.AddComponentAsync(componentType, domElementSelector); - } + SetCurrentCircuitHost(this); - await OnCircuitOpenedAsync(cancellationToken); + for (var i = 0; i < Descriptors.Count; i++) + { + var (componentType, domElementSelector) = Descriptors[i]; + await Renderer.AddComponentAsync(componentType, domElementSelector); + } - await OnConnectionUpAsync(cancellationToken); + await OnCircuitOpenedAsync(cancellationToken); + + await OnConnectionUpAsync(cancellationToken); + } + catch (Exception ex) + { + Renderer_UnhandledException(this, ex); + } }); _initialized = true; diff --git a/src/Components/Server/src/ComponentHub.cs b/src/Components/Server/src/ComponentHub.cs index 1603ac602d7e..ce60cc805473 100644 --- a/src/Components/Server/src/ComponentHub.cs +++ b/src/Components/Server/src/ComponentHub.cs @@ -64,7 +64,7 @@ public override Task OnDisconnectedAsync(Exception exception) /// /// Intended for framework use only. Applications should not call this method directly. /// - public async Task StartCircuit(string uriAbsolute, string baseUriAbsolute) + public string StartCircuit(string uriAbsolute, string baseUriAbsolute) { var circuitClient = new CircuitClientProxy(Clients.Caller, Context.ConnectionId); @@ -76,8 +76,7 @@ public async Task StartCircuit(string uriAbsolute, string baseUriAbsolut circuitHost.UnhandledException += CircuitHost_UnhandledException; - // If initialization fails, this will throw. The caller will fail if they try to call into any interop API. - await circuitHost.InitializeAsync(Context.ConnectionAborted); + circuitHost.Initialize(Context.ConnectionAborted); _circuitRegistry.Register(circuitHost); diff --git a/src/Components/Server/test/Circuits/CircuitHostTest.cs b/src/Components/Server/test/Circuits/CircuitHostTest.cs index 3b907e8592fb..8e3709cac35b 100644 --- a/src/Components/Server/test/Circuits/CircuitHostTest.cs +++ b/src/Components/Server/test/Circuits/CircuitHostTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Text.Encodings.Web; using System.Threading; using System.Threading.Tasks; @@ -39,7 +40,7 @@ public async Task DisposeAsync_DisposesResources() } [Fact] - public async Task InitializeAsync_InvokesHandlers() + public void Initialize_InvokesHandlers() { // Arrange var cancellationToken = new CancellationToken(); @@ -74,13 +75,47 @@ public async Task InitializeAsync_InvokesHandlers() var circuitHost = TestCircuitHost.Create(handlers: new[] { handler1.Object, handler2.Object }); // Act - await circuitHost.InitializeAsync(cancellationToken); + circuitHost.Initialize(cancellationToken); // Assert handler1.VerifyAll(); handler2.VerifyAll(); } + [Fact] + public void Initialize_ReportsAsyncExceptions() + { + // Arrange + var handler = new Mock(MockBehavior.Strict); + var tcs = new TaskCompletionSource(); + var reportedErrors = new List(); + + handler + .Setup(h => h.OnCircuitOpenedAsync(It.IsAny(), It.IsAny())) + .Returns(tcs.Task) + .Verifiable(); + + var circuitHost = TestCircuitHost.Create(handlers: new[] { handler.Object }); + circuitHost.UnhandledException += (sender, errorInfo) => + { + Assert.Same(circuitHost, sender); + reportedErrors.Add(errorInfo); + }; + + // Act + circuitHost.Initialize(new CancellationToken()); + handler.VerifyAll(); + + // Assert: there was no synchronous exception + Assert.Empty(reportedErrors); + + // Act/Assert: if the handler throws later, that gets reported + var ex = new InvalidTimeZoneException(); + tcs.SetException(ex); + Assert.Same(ex, reportedErrors.Single().ExceptionObject); + Assert.False(reportedErrors.Single().IsTerminating); + } + [Fact] public async Task DisposeAsync_InvokesCircuitHandler() { From 50c31b89bb58b1127513b6997a468ae95cb929e5 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 28 Mar 2019 12:29:46 +0000 Subject: [PATCH 2/3] CR: Redesign --- .../Server/src/Circuits/CircuitHost.cs | 24 +++++++++---------- src/Components/Server/src/ComponentHub.cs | 6 ++++- .../Server/test/Circuits/CircuitHostTest.cs | 20 ++++++++++------ 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/Components/Server/src/Circuits/CircuitHost.cs b/src/Components/Server/src/Circuits/CircuitHost.cs index 13d12f726474..b5801e2ee3d0 100644 --- a/src/Components/Server/src/Circuits/CircuitHost.cs +++ b/src/Components/Server/src/Circuits/CircuitHost.cs @@ -104,35 +104,35 @@ public Task> PrerenderComponentAsync(Type componentType, Par }); } - public void Initialize(CancellationToken cancellationToken) + public async Task InitializeAsync(CancellationToken cancellationToken) { - // This Initialize method is fire-and-forget as far as the caller is concerned, because - // if it was to await completion, it would be blocking the SignalR message loop. This could - // lead to deadlock, e.g., if the init process itself waited for an incoming SignalR message - // such as the result of a JSInterop call. - Renderer.InvokeAsync(async () => + await Renderer.InvokeAsync(async () => { try { SetCurrentCircuitHost(this); + _initialized = true; // We're ready to accept incoming JSInterop calls from here on + await OnCircuitOpenedAsync(cancellationToken); + await OnConnectionUpAsync(cancellationToken); + + // We add the root components *after* the circuit is flagged as open. + // That's because AddComponentAsync waits for quiescence, which can take + // arbitrarily long. In the meantime we might need to be receiving and + // processing incoming JSInterop calls or similar. for (var i = 0; i < Descriptors.Count; i++) { var (componentType, domElementSelector) = Descriptors[i]; await Renderer.AddComponentAsync(componentType, domElementSelector); } - - await OnCircuitOpenedAsync(cancellationToken); - - await OnConnectionUpAsync(cancellationToken); } catch (Exception ex) { + // We have to handle all our own errors here, because the upstream caller + // has to fire-and-forget this Renderer_UnhandledException(this, ex); } }); - - _initialized = true; } public async void BeginInvokeDotNetFromJS(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) diff --git a/src/Components/Server/src/ComponentHub.cs b/src/Components/Server/src/ComponentHub.cs index ce60cc805473..76fb91374def 100644 --- a/src/Components/Server/src/ComponentHub.cs +++ b/src/Components/Server/src/ComponentHub.cs @@ -76,7 +76,11 @@ public string StartCircuit(string uriAbsolute, string baseUriAbsolute) circuitHost.UnhandledException += CircuitHost_UnhandledException; - circuitHost.Initialize(Context.ConnectionAborted); + // Fire-and-forget the initialization process, because we can't block the + // SignalR message loop (we'd get a deadlock if any of the initialization + // logic relied on receiving a subsequent message from SignalR), and it will + // take care of its own errors anyway. + _ = circuitHost.InitializeAsync(Context.ConnectionAborted); _circuitRegistry.Register(circuitHost); diff --git a/src/Components/Server/test/Circuits/CircuitHostTest.cs b/src/Components/Server/test/Circuits/CircuitHostTest.cs index 8e3709cac35b..c97ca35e2727 100644 --- a/src/Components/Server/test/Circuits/CircuitHostTest.cs +++ b/src/Components/Server/test/Circuits/CircuitHostTest.cs @@ -40,7 +40,7 @@ public async Task DisposeAsync_DisposesResources() } [Fact] - public void Initialize_InvokesHandlers() + public async Task Initialize_InvokesHandlers() { // Arrange var cancellationToken = new CancellationToken(); @@ -75,7 +75,7 @@ public void Initialize_InvokesHandlers() var circuitHost = TestCircuitHost.Create(handlers: new[] { handler1.Object, handler2.Object }); // Act - circuitHost.Initialize(cancellationToken); + await circuitHost.InitializeAsync(cancellationToken); // Assert handler1.VerifyAll(); @@ -83,7 +83,7 @@ public void Initialize_InvokesHandlers() } [Fact] - public void Initialize_ReportsAsyncExceptions() + public async Task Initialize_ReportsOwnAsyncExceptions() { // Arrange var handler = new Mock(MockBehavior.Strict); @@ -103,15 +103,21 @@ public void Initialize_ReportsAsyncExceptions() }; // Act - circuitHost.Initialize(new CancellationToken()); - handler.VerifyAll(); + var initializeAsyncTask = circuitHost.InitializeAsync(new CancellationToken()); - // Assert: there was no synchronous exception + // Assert: No synchronous exceptions + handler.VerifyAll(); Assert.Empty(reportedErrors); - // Act/Assert: if the handler throws later, that gets reported + // Act: Trigger async exception var ex = new InvalidTimeZoneException(); tcs.SetException(ex); + + // Assert: The top-level task still succeeds, because the intended usage + // pattern is fire-and-forget. + await initializeAsyncTask; + + // Assert: The async exception was reported via the side-channel Assert.Same(ex, reportedErrors.Single().ExceptionObject); Assert.False(reportedErrors.Single().IsTerminating); } From a3e16949b03f54481214c2d7a6246f5b1e528913 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 28 Mar 2019 12:41:35 +0000 Subject: [PATCH 3/3] Fix test names --- src/Components/Server/test/Circuits/CircuitHostTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Components/Server/test/Circuits/CircuitHostTest.cs b/src/Components/Server/test/Circuits/CircuitHostTest.cs index c97ca35e2727..559399d9e90e 100644 --- a/src/Components/Server/test/Circuits/CircuitHostTest.cs +++ b/src/Components/Server/test/Circuits/CircuitHostTest.cs @@ -40,7 +40,7 @@ public async Task DisposeAsync_DisposesResources() } [Fact] - public async Task Initialize_InvokesHandlers() + public async Task InitializeAsync_InvokesHandlers() { // Arrange var cancellationToken = new CancellationToken(); @@ -83,7 +83,7 @@ public async Task Initialize_InvokesHandlers() } [Fact] - public async Task Initialize_ReportsOwnAsyncExceptions() + public async Task InitializeAsync_ReportsOwnAsyncExceptions() { // Arrange var handler = new Mock(MockBehavior.Strict);