From 46f27cdd0b20afa20ebe7afc63d69de3076adcd6 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Thu, 29 Mar 2018 16:35:31 -0700 Subject: [PATCH] Fix internal HubConnection state when handshake fails (#1774) --- .../HubConnection.cs | 18 ++++++++++-------- .../HubConnectionTests.ConnectionLifecycle.cs | 19 +++++++++++++++++++ .../HubConnectionTests.Protocol.cs | 6 +++--- .../TestConnection.cs | 8 ++++---- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs b/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs index 25be1fe0d7..c05e30158e 100644 --- a/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs +++ b/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs @@ -132,24 +132,26 @@ namespace Microsoft.AspNetCore.SignalR.Client // Start the connection var connection = _connectionFactory(); await connection.StartAsync(_protocol.TransferFormat); - _connectionState = new ConnectionState(connection, this); + var startingConnectionState = new ConnectionState(connection, this); // From here on, if an error occurs we need to shut down the connection because // we still own it. try { Log.HubProtocol(_logger, _protocol.Name, _protocol.Version); - await HandshakeAsync(); + await HandshakeAsync(startingConnectionState); } catch (Exception ex) { Log.ErrorStartingConnection(_logger, ex); // Can't have any invocations to cancel, we're in the lock. - await _connectionState.Connection.DisposeAsync(); + await startingConnectionState.Connection.DisposeAsync(); throw; } + // Set this at the end to avoid setting internal state until the connection is real + _connectionState = startingConnectionState; _connectionState.ReceiveTask = ReceiveLoop(_connectionState); Log.Started(_logger); } @@ -494,15 +496,15 @@ namespace Microsoft.AspNetCore.SignalR.Client } } - private async Task HandshakeAsync() + private async Task HandshakeAsync(ConnectionState startingConnectionState) { // Send the Handshake request Log.SendingHubHandshake(_logger); var handshakeRequest = new HandshakeRequestMessage(_protocol.Name, _protocol.Version); - HandshakeProtocol.WriteRequestMessage(handshakeRequest, _connectionState.Connection.Transport.Output); + HandshakeProtocol.WriteRequestMessage(handshakeRequest, startingConnectionState.Connection.Transport.Output); - var sendHandshakeResult = await _connectionState.Connection.Transport.Output.FlushAsync(CancellationToken.None); + var sendHandshakeResult = await startingConnectionState.Connection.Transport.Output.FlushAsync(CancellationToken.None); if (sendHandshakeResult.IsCompleted) { @@ -514,7 +516,7 @@ namespace Microsoft.AspNetCore.SignalR.Client { while (true) { - var result = await _connectionState.Connection.Transport.Input.ReadAsync(); + var result = await startingConnectionState.Connection.Transport.Input.ReadAsync(); var buffer = result.Buffer; var consumed = buffer.Start; var examined = buffer.End; @@ -551,7 +553,7 @@ namespace Microsoft.AspNetCore.SignalR.Client } finally { - _connectionState.Connection.Transport.Input.AdvanceTo(consumed, examined); + startingConnectionState.Connection.Transport.Input.AdvanceTo(consumed, examined); } } } diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.ConnectionLifecycle.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.ConnectionLifecycle.cs index 37a51a4337..2c8e91906a 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.ConnectionLifecycle.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.ConnectionLifecycle.cs @@ -115,6 +115,25 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests }); } + [Fact] + public async Task StartAsyncWithFailedHandshakeCanBeStopped() + { + var testConnection = new TestConnection(autoHandshake: false); + await AsyncUsing(new HubConnection(() => testConnection, new JsonHubProtocol()), async connection => + { + testConnection.Transport.Input.Complete(); + try + { + await connection.StartAsync(); + } + catch + { } + + await connection.StopAsync(); + Assert.True(testConnection.Started.IsCompleted); + }); + } + [Theory] [MemberData(nameof(MethodsNamesThatRequireActiveConnection))] public async Task MethodsThatRequireStartedConnectionFailIfConnectionNotYetStarted(string name) diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs index 2f2db62e1f..336e8d95e1 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs @@ -45,7 +45,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests [Fact] public async Task ClientSendsHandshakeMessageWhenStartingConnection() { - var connection = new TestConnection(autoNegotiate: false); + var connection = new TestConnection(autoHandshake: false); var hubConnection = CreateHubConnection(connection); try { @@ -407,7 +407,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests [Fact] public async Task PartialHandshakeResponseWorks() { - var connection = new TestConnection(synchronousCallbacks: true, autoNegotiate: false); + var connection = new TestConnection(synchronousCallbacks: true, autoHandshake: false); var hubConnection = CreateHubConnection(connection); try { @@ -436,7 +436,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests public async Task HandshakeAndInvocationInSameBufferWorks() { var payload = "{}\u001e{\"type\":1, \"target\": \"Echo\", \"arguments\":[\"hello\"]}\u001e"; - var connection = new TestConnection(synchronousCallbacks: true, autoNegotiate: false); + var connection = new TestConnection(synchronousCallbacks: true, autoHandshake: false); var hubConnection = CreateHubConnection(connection); try { diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs index 4a1f8fc7be..c7dc4f2d96 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests { internal class TestConnection : IConnection { - private readonly bool _autoNegotiate; + private readonly bool _autoHandshake; private readonly TaskCompletionSource _started = new TaskCompletionSource(); private readonly TaskCompletionSource _disposed = new TaskCompletionSource(); @@ -38,9 +38,9 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests public IFeatureCollection Features { get; } = new FeatureCollection(); public int DisposeCount => _disposeCount; - public TestConnection(Func onStart = null, Func onDispose = null, bool autoNegotiate = true, bool synchronousCallbacks = false) + public TestConnection(Func onStart = null, Func onDispose = null, bool autoHandshake = true, bool synchronousCallbacks = false) { - _autoNegotiate = autoNegotiate; + _autoHandshake = autoHandshake; _onStart = onStart ?? (() => Task.CompletedTask); _onDispose = onDispose ?? (() => Task.CompletedTask); @@ -64,7 +64,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests await _onStart(); - if (_autoNegotiate) + if (_autoHandshake) { // We can't await this as it will block StartAsync which will block // HubConnection.StartAsync which sends the Handshake in the first place!