Add redis startup error handling (#41141)

Author: BrennanConroy

src/SignalR/server/StackExchangeRedis/src/Internal/RedisLog.cs

@@ -58,6 +58,9 @@ public static void ConnectingToEndpoints(ILogger logger, EndPointCollection endp
     [LoggerMessage(13, LogLevel.Error, "Error forwarding client result with ID '{InvocationID}' to server.", EventName = "ErrorForwardingResult")]
     public static partial void ErrorForwardingResult(ILogger logger, string invocationId, Exception ex);
 
+    [LoggerMessage(14, LogLevel.Error, "Error connecting to Redis.", EventName = "ErrorConnecting")]
+    public static partial void ErrorConnecting(ILogger logger, Exception ex);
+
     // This isn't DefineMessage-based because it's just the simple TextWriter logging from ConnectionMultiplexer
     public static void ConnectionMultiplexerMessage(ILogger logger, string? message)
     {

src/SignalR/server/StackExchangeRedis/src/RedisHubLifetimeManager.cs

@@ -35,6 +35,7 @@ public class RedisHubLifetimeManager<THub> : HubLifetimeManager<THub>, IDisposab
     private readonly SemaphoreSlim _connectionLock = new SemaphoreSlim(1);
     private readonly IHubProtocolResolver _hubProtocolResolver;
     private readonly ClientResultsManager _clientResultsManager = new();
+    private bool _redisConnectErrorLogged;
 
     private readonly AckHandler _ackHandler;
     private int _internalAckId;
@@ -90,6 +91,7 @@ public RedisHubLifetimeManager(ILogger<RedisHubLifetimeManager<THub>> logger,
     public override async Task OnConnectedAsync(HubConnectionContext connection)
     {
         await EnsureRedisServerConnection();
+
         var feature = new RedisFeature();
         connection.Features.Set<IRedisFeature>(feature);
 
@@ -112,11 +114,17 @@ public override Task OnDisconnectedAsync(HubConnectionContext connection)
     {
         _connections.Remove(connection);
 
-        var tasks = new List<Task>();
+        // If the bus is null then the Redis connection failed to be established and none of the other connection setup ran
+        if (_bus is null)
+        {
+            return Task.CompletedTask;
+        }
 
         var connectionChannel = _channels.Connection(connection.ConnectionId);
+        var tasks = new List<Task>();
+
         RedisLog.Unsubscribe(_logger, connectionChannel);
-        tasks.Add(_bus!.UnsubscribeAsync(connectionChannel));
+        tasks.Add(_bus.UnsubscribeAsync(connectionChannel));
 
         var feature = connection.Features.GetRequiredFeature<IRedisFeature>();
         var groupNames = feature.Groups;
@@ -704,7 +712,21 @@ private async Task EnsureRedisServerConnection()
                 if (_redisServerConnection == null)
                 {
                     var writer = new LoggerTextWriter(_logger);
-                    _redisServerConnection = await _options.ConnectAsync(writer);
+                    try
+                    {
+                        _redisServerConnection = await _options.ConnectAsync(writer);
+                    }
+                    catch (Exception ex)
+                    {
+                        // If the connection hasn't been established yet we shouldn't keep logging the same error over and over
+                        // for every new client connection.
+                        if (!_redisConnectErrorLogged)
+                        {
+                            RedisLog.ErrorConnecting(_logger, ex);
+                            _redisConnectErrorLogged = true;
+                        }
+                        throw;
+                    }
                     _bus = _redisServerConnection.GetSubscriber();
 
                     _redisServerConnection.ConnectionRestored += (_, e) =>

src/SignalR/server/StackExchangeRedis/test/RedisHubLifetimeManagerTests.cs

@@ -7,8 +7,11 @@
 using Microsoft.AspNetCore.SignalR.Specification.Tests;
 using Microsoft.AspNetCore.SignalR.Tests;
 using Microsoft.AspNetCore.Testing;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Extensions.Logging.Testing;
 using Microsoft.Extensions.Options;
+using Moq;
 using Newtonsoft.Json.Linq;
 using Newtonsoft.Json.Serialization;
 using Xunit;
@@ -83,6 +86,45 @@ public async Task CamelCasedJsonIsPreservedAcrossRedisBoundary()
         }
     }
 
+    [Fact]
+    public async Task ErrorFromConnectionFactoryLogsAndAllowsDisconnect()
+    {
+        var server = new TestRedisServer();
+
+        var testSink = new TestSink();
+        var logger = new TestLogger("", testSink, true);
+        var mockLoggerFactory = new Mock<ILoggerFactory>();
+        mockLoggerFactory
+            .Setup(m => m.CreateLogger(It.IsAny<string>()))
+            .Returns((string categoryName) => (ILogger)logger);
+        var loggerT = mockLoggerFactory.Object.CreateLogger<RedisHubLifetimeManager<Hub>>();
+
+        var manager = new RedisHubLifetimeManager<Hub>(
+            loggerT,
+            Options.Create(new RedisOptions()
+            {
+                ConnectionFactory = _ => throw new Exception("throw from connect")
+            }),
+            new DefaultHubProtocolResolver(new IHubProtocol[]
+            {
+            }, NullLogger<DefaultHubProtocolResolver>.Instance));
+
+        using (var client = new TestClient())
+        {
+            var connection = HubConnectionContextUtils.Create(client.Connection);
+
+            var ex = await Assert.ThrowsAsync<Exception>(() => manager.OnConnectedAsync(connection)).DefaultTimeout();
+            Assert.Equal("throw from connect", ex.Message);
+
+            await manager.OnDisconnectedAsync(connection).DefaultTimeout();
+        }
+
+        var logs = testSink.Writes.ToArray();
+        Assert.Single(logs);
+        Assert.Equal("Error connecting to Redis.", logs[0].Message);
+        Assert.Equal("throw from connect", logs[0].Exception.Message);
+    }
+
     public override TestRedisServer CreateBackplane()
     {
         return new TestRedisServer();