Change pool handling in new RedisManagerPool, return new (unmanaged) client when pool exceeded instead of blocking

Author: mythz

src/ServiceStack.Redis/RedisManagerPool.cs

@@ -11,16 +11,13 @@ namespace ServiceStack.Redis
 {
     public class RedisPoolConfig
     {
-        public const int DefaultPoolTimeoutMs = 2000;
-        public const int DefaultMaxPoolSize = 50;
+        public const int DefaultMaxPoolSize = 20;
 
         public RedisPoolConfig()
         {
             MaxPoolSize = DefaultMaxPoolSize;
-            PoolTimeout = TimeSpan.FromMilliseconds(DefaultPoolTimeoutMs);
         }
 
-        public TimeSpan PoolTimeout { get; set; }        
         public int MaxPoolSize { get; set; }
     }
 
@@ -50,7 +47,6 @@ public partial class RedisManagerPool
 
         public Action<IRedisNativeClient> ConnectionFilter { get; set; }
 
-        public int PoolTimeoutMs { get; set; }        
         public int MaxPoolSize { get; set; }
 
         public RedisManagerPool() : this(RedisNativeClient.DefaultHost) {}
@@ -74,7 +70,6 @@ public RedisManagerPool(IEnumerable<string> hosts, RedisPoolConfig config)
 
             this.OnFailover = new List<Action<IRedisClientsManager>>();
 
-            this.PoolTimeoutMs = (int) config.PoolTimeout.TotalMilliseconds;
             this.MaxPoolSize = config.MaxPoolSize;
 
             clients = new RedisClient[MaxPoolSize];
@@ -121,9 +116,13 @@ public IRedisClient GetClient()
                 RedisClient inActiveClient;
                 while ((inActiveClient = GetInActiveClient()) == null)
                 {
-                    // wait for a connection, cry out if made to wait too long
-                    if (!Monitor.Wait(clients, PoolTimeoutMs))
-                        throw new TimeoutException(PoolTimeoutError);
+                    //Create new client outside of pool when max pool size exceeded
+                    var nextIndex = poolIndex % clients.Length;
+                    var nextHost = Hosts[nextIndex];
+                    var newClient = InitNewClient(nextHost);
+                    //Don't handle callbacks for new client outside pool
+                    newClient.ClientManager = null; 
+                    return newClient;
                 }
 
                 poolIndex++;
@@ -188,21 +187,13 @@ private void InitClient(RedisClient client)
 
         public void DisposeClient(RedisNativeClient client)
         {
-            lock (clients)
+            for (var i = 0; i < clients.Length; i++)
             {
-                for (var i = 0; i < clients.Length; i++)
-                {
-                    var writeClient = clients[i];
-                    if (client != writeClient) continue;
-                    client.Active = false;
-                    Monitor.PulseAll(clients);
-                    return;
-                }
+                var writeClient = clients[i];
+                if (client != writeClient) continue;
+                client.Active = false;
+                return;
             }
-
-            //Client not found in any pool, pulse both pools.
-            lock (clients)
-                Monitor.PulseAll(clients);
         }
 
         /// <summary>
@@ -211,11 +202,7 @@ public void DisposeClient(RedisNativeClient client)
         /// <param name="client">The client.</param>
         public void DisposeWriteClient(RedisNativeClient client)
         {
-            lock (clients)
-            {
-                client.Active = false;
-                Monitor.PulseAll(clients);
-            }
+            client.Active = false;
         }
 
         public Dictionary<string, string> GetStats()
@@ -263,14 +250,12 @@ private void AssertValidPool()
         public int[] GetClientPoolActiveStates()
         {
             var activeStates = new int[clients.Length];
-            lock (clients)
+            for (int i = 0; i < clients.Length; i++)
             {
-                for (int i = 0; i < clients.Length; i++)
-                {
-                    activeStates[i] = clients[i] == null
-                        ? -1
-                        : clients[i].Active ? 1 : 0;
-                }
+                var client = clients[i];
+                activeStates[i] = client == null
+                    ? -1
+                    : client.Active ? 1 : 0;
             }
             return activeStates;
         }

src/ServiceStack.Redis/RedisNativeClient.cs

@@ -1984,6 +1984,11 @@ public RedisPipelineCommand CreatePipelineCommand()
 
         internal bool IsDisposed { get; set; }
 
+        public bool IsManagedClient
+        {
+            get { return ClientManager != null; }
+        }
+
         public void Dispose()
         {
             Dispose(true);

tests/ServiceStack.Redis.Tests/RedisManagerPoolTests.cs

@@ -158,7 +158,7 @@ public void Can_have_different_pool_size_and_host_configurations()
 		}
 
 		[Test]
-		public void Does_block_ReadWrite_clients_pool()
+		public void Does_not_block_ReadWrite_clients_pool()
 		{
             using (var manager = new RedisManagerPool(
                     hosts,
@@ -174,6 +174,11 @@ public void Does_block_ReadWrite_clients_pool()
 				var client3 = manager.GetClient();
 				var client4 = manager.GetClient();
 
+                Assert.That(((RedisClient)client1).IsManagedClient, Is.True);
+                Assert.That(((RedisClient)client2).IsManagedClient, Is.True);
+                Assert.That(((RedisClient)client3).IsManagedClient, Is.True);
+                Assert.That(((RedisClient)client4).IsManagedClient, Is.True); 
+
 				Action func = delegate {
 					Thread.Sleep(delay + TimeSpan.FromSeconds(0.5));
 					client4.Dispose();
@@ -185,13 +190,15 @@ public void Does_block_ReadWrite_clients_pool()
 
 				var client5 = manager.GetClient();
 
-				Assert.That(DateTime.Now - start, Is.GreaterThanOrEqualTo(delay));
+                Assert.That(((RedisClient)client5).IsManagedClient, Is.False); //outside of pool
+
+				Assert.That(DateTime.Now - start, Is.LessThan(delay));
 
 				AssertClientHasHost(client1, hosts[0]);
 				AssertClientHasHost(client2, hosts[1]);
 				AssertClientHasHost(client3, hosts[2]);
 				AssertClientHasHost(client4, hosts[3]);
-				AssertClientHasHost(client5, hosts[3]);
+				AssertClientHasHost(client5, hosts[0]);
 
 				mockFactory.VerifyAll();
 			}