From 9514a865effc2c1d293e6e28fe940b99bb0a0b93 Mon Sep 17 00:00:00 2001 From: Brennan Date: Wed, 25 Mar 2020 13:13:02 -0700 Subject: [PATCH 1/2] [SignalR] Avoid deadlock with closing and user callbacks (#19612) (#19664) * [SignalR] Avoid deadlock with closing and user callbacks (#19612) * fb --- .../csharp/Client.Core/src/HubConnection.cs | 4 +- .../test/UnitTests/HubConnectionTests.cs | 48 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs b/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs index caf9a4b514..e8d35d753f 100644 --- a/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs +++ b/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs @@ -1207,11 +1207,13 @@ namespace Microsoft.AspNetCore.SignalR.Client finally { invocationMessageChannel.Writer.TryComplete(); - await invocationMessageReceiveTask; timer.Stop(); await timerTask; uploadStreamSource.Cancel(); await HandleConnectionClose(connectionState); + + // await after the connection has been closed, otherwise could deadlock on a user's .On callback(s) + await invocationMessageReceiveTask; } } diff --git a/src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.cs b/src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.cs index 2c9df93cb8..cd76bd7655 100644 --- a/src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.cs +++ b/src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.cs @@ -345,7 +345,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests var complete = await connection.ReadSentJsonAsync().OrTimeout(); Assert.Equal(HubProtocolConstants.CompletionMessageType, complete["type"]); Assert.EndsWith("canceled by client.", ((string)complete["error"])); - } + } } [Fact] @@ -414,6 +414,52 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests } } + [Fact] + public async Task CanAwaitInvokeFromOnHandlerWithServerClosingConnection() + { + using (StartVerifiableLog()) + { + var connection = new TestConnection(); + var hubConnection = CreateHubConnection(connection, loggerFactory: LoggerFactory); + await hubConnection.StartAsync().OrTimeout(); + + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + hubConnection.On("Echo", async msg => + { + try + { + // This should be canceled when the connection is closed + await hubConnection.InvokeAsync("Echo", msg).OrTimeout(); + } + catch (Exception ex) + { + tcs.SetException(ex); + return; + } + + tcs.SetResult(null); + }); + + var closedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + hubConnection.Closed += _ => + { + closedTcs.SetResult(null); + + return Task.CompletedTask; + }; + + await connection.ReceiveJsonMessage(new { type = HubProtocolConstants.InvocationMessageType, target = "Echo", arguments = new object[] { "42" } }).OrTimeout(); + + // Read sent message first to make sure invoke has been processed and is waiting for a response + await connection.ReadSentJsonAsync().OrTimeout(); + await connection.ReceiveJsonMessage(new { type = HubProtocolConstants.CloseMessageType }).OrTimeout(); + + await closedTcs.Task.OrTimeout(); + + await Assert.ThrowsAsync(() => tcs.Task.OrTimeout()); + } + } + private class SampleObject { public SampleObject(string foo, int bar) From 8ad805b4ff762a95740e3fba072d9a964fa52161 Mon Sep 17 00:00:00 2001 From: William Godbe Date: Wed, 25 Mar 2020 15:36:53 -0700 Subject: [PATCH 2/2] Unskip targeting pack tests --- src/Framework/test/TargetingPackTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Framework/test/TargetingPackTests.cs b/src/Framework/test/TargetingPackTests.cs index 8e830707a3..82b43cb831 100644 --- a/src/Framework/test/TargetingPackTests.cs +++ b/src/Framework/test/TargetingPackTests.cs @@ -28,7 +28,7 @@ namespace Microsoft.AspNetCore _targetingPackRoot = Path.Combine(TestData.GetTestDataValue("TargetingPackLayoutRoot"), "packs", "Microsoft.AspNetCore.App.Ref", TestData.GetTestDataValue("TargetingPackVersion")); } - [Fact(Skip="https://github.com/aspnet/AspNetCore/issues/14832")] + [Fact] public void AssembliesAreReferenceAssemblies() { IEnumerable dlls = Directory.GetFiles(_targetingPackRoot, "*.dll", SearchOption.AllDirectories); @@ -55,7 +55,7 @@ namespace Microsoft.AspNetCore }); } - [Fact(Skip="https://github.com/aspnet/AspNetCore/issues/14832")] + [Fact] public void PlatformManifestListsAllFiles() { var platformManifestPath = Path.Combine(_targetingPackRoot, "data", "PlatformManifest.txt");