Fix internal HubConnection state when handshake fails (#1774)

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);
                     }
                 }
             }

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)

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
                 {

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<object> _started = new TaskCompletionSource<object>();
         private readonly TaskCompletionSource<object> _disposed = new TaskCompletionSource<object>();
 
@@ -38,9 +38,9 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
         public IFeatureCollection Features { get; } = new FeatureCollection();
         public int DisposeCount => _disposeCount;
 
-        public TestConnection(Func<Task> onStart = null, Func<Task> onDispose = null, bool autoNegotiate = true, bool synchronousCallbacks = false)
+        public TestConnection(Func<Task> onStart = null, Func<Task> 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!