fix #2134 by disposing httpconnection if start fails (#2137) (#2188)

src/Microsoft.AspNetCore.SignalR.Client/HttpConnectionFactory.cs

@@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.SignalR.Client
             {
                 throw new ArgumentNullException(nameof(loggerFactory));
             }
-            
+
             _httpConnectionOptions = options.Value;
             _loggerFactory = loggerFactory;
         }
@@ -44,8 +44,17 @@ namespace Microsoft.AspNetCore.SignalR.Client
         public async Task<ConnectionContext> ConnectAsync(TransferFormat transferFormat, CancellationToken cancellationToken = default)
         {
             var connection = new HttpConnection(_httpConnectionOptions, _loggerFactory);
-            await connection.StartAsync(transferFormat, cancellationToken);
-            return connection;
+            try
+            {
+                await connection.StartAsync(transferFormat, cancellationToken);
+                return connection;
+            }
+            catch
+            {
+                // Make sure the connection is disposed, in case it allocated any resources before failing.
+                await connection.DisposeAsync();
+                throw;
+            }
         }
 
         /// <inheritdoc />

test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionFactoryTests.cs

@@ -0,0 +1,32 @@
+using System;
+using System.Net.Http;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Connections;
+using Microsoft.AspNetCore.Http.Connections.Client;
+using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Extensions.Options;
+using Xunit;
+
+namespace Microsoft.AspNetCore.SignalR.Client.Tests
+{
+    public class HttpConnectionFactoryTests
+    {
+        [Fact]
+        public async Task ConnectionIsDisposedIfItFailsToStartAsync()
+        {
+            var testHandler = new TestHttpMessageHandler(autoNegotiate: false, handleFirstPoll: false);
+            testHandler.OnRequest((req, next, ct) => Task.FromException<HttpResponseMessage>(new Exception("BOOM")));
+
+            var factory = new HttpConnectionFactory(Options.Create(new HttpConnectionOptions() {
+                Url = new Uri("http://example.com"),
+                HttpMessageHandlerFactory = _ => testHandler
+            }), NullLoggerFactory.Instance);
+
+            // We don't care about the specific exception
+            await Assert.ThrowsAnyAsync<Exception>(() => factory.ConnectAsync(TransferFormat.Text));
+
+            // We care that the handler (and by extension the client) was disposed
+            Assert.True(testHandler.Disposed);
+        }
+    }
+}

test/Microsoft.AspNetCore.SignalR.Client.Tests/TestHttpMessageHandler.cs

@@ -16,6 +16,8 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
 
         private List<Func<RequestDelegate, RequestDelegate>> _middleware = new List<Func<RequestDelegate, RequestDelegate>>();
 
+        public bool Disposed { get; private set; }
+
         public IReadOnlyList<HttpRequestMessage> ReceivedRequests
         {
             get
@@ -52,6 +54,12 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
             }
         }
 
+        protected override void Dispose(bool disposing)
+        {
+            Disposed = true;
+            base.Dispose(disposing);
+        }
+
         protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
         {
             await Task.Yield();