Localize locks and synchronize RedisSentinelWorker using internal oLock

Author: mythz

src/ServiceStack.Redis/RedisSentinel.cs

@@ -23,7 +23,7 @@ public class RedisSentinel : IRedisSentinel
         public static string DefaultMasterName = "mymaster";
         public static string DefaultAddress = "127.0.0.1:26379";
 
-        private object oLock = new object();
+        private readonly object oLock = new object();
         private bool isDisposed = false;
 
         private readonly string masterName;
@@ -308,6 +308,7 @@ private RedisSentinelWorker GetValidSentinelWorker()
                 {
                     this.worker = GetNextSentinel();
                     GetRedisManager();
+
                     this.worker.BeginListeningForConfigurationChanges();
                     this.failures = 0; //reset
                     return this.worker;
@@ -332,29 +333,23 @@ private RedisSentinelWorker GetValidSentinelWorker()
         public RedisEndpoint GetMaster()
         {
             var sentinelWorker = GetValidSentinelWorker();
-            lock (sentinelWorker)
-            {
-                var host = sentinelWorker.GetMasterHost(masterName);
+            var host = sentinelWorker.GetMasterHost(masterName);
 
-                if (ScanForOtherSentinels && DateTime.UtcNow - lastSentinelsRefresh > RefreshSentinelHostsAfter)
-                {
-                    RefreshActiveSentinels();
-                }
-
-                return host != null
-                    ? (HostFilter != null ? HostFilter(host) : host).ToRedisEndpoint()
-                    : null;
+            if (ScanForOtherSentinels && DateTime.UtcNow - lastSentinelsRefresh > RefreshSentinelHostsAfter)
+            {
+                RefreshActiveSentinels();
             }
+
+            return host != null
+                ? (HostFilter != null ? HostFilter(host) : host).ToRedisEndpoint()
+                : null;
         }
 
         public List<RedisEndpoint> GetSlaves()
         {
             var sentinelWorker = GetValidSentinelWorker();
-            lock (sentinelWorker)
-            {
-                var hosts = sentinelWorker.GetSlaveHosts(masterName);
-                return ConfigureHosts(hosts).Map(x => x.ToRedisEndpoint());
-            }
+            var hosts = sentinelWorker.GetSlaveHosts(masterName);
+            return ConfigureHosts(hosts).Map(x => x.ToRedisEndpoint());
         }
 
         /// <summary>
@@ -369,23 +364,32 @@ private bool ShouldRetry()
 
         private RedisSentinelWorker GetNextSentinel()
         {
-            lock (oLock)
+            RedisSentinelWorker disposeWorker = null;
+
+            try
             {
-                if (this.worker != null)
+                lock (oLock)
                 {
-                    this.worker.Dispose();
-                    this.worker = null;
-                }
+                    if (this.worker != null)
+                    {
+                        disposeWorker = this.worker;
+                        this.worker = null;
+                    }
 
-                if (++sentinelIndex >= SentinelEndpoints.Length)
-                    sentinelIndex = 0;
+                    if (++sentinelIndex >= SentinelEndpoints.Length)
+                        sentinelIndex = 0;
 
-                var sentinelWorker = new RedisSentinelWorker(this, SentinelEndpoints[sentinelIndex])
-                {
-                    OnSentinelError = OnSentinelError
-                };
+                    var sentinelWorker = new RedisSentinelWorker(this, SentinelEndpoints[sentinelIndex])
+                    {
+                        OnSentinelError = OnSentinelError
+                    };
 
-                return sentinelWorker;
+                    return sentinelWorker;
+                }
+            }
+            finally
+            {
+                disposeWorker?.Dispose();
             }
         }
 
@@ -406,19 +410,13 @@ private void OnSentinelError(Exception ex)
         public void ForceMasterFailover()
         {
             var sentinelWorker = GetValidSentinelWorker();
-            lock (sentinelWorker)
-            {
-                sentinelWorker.ForceMasterFailover(masterName);
-            }
+            sentinelWorker.ForceMasterFailover(masterName);
         }
 
         public SentinelInfo GetSentinelInfo()
         {
             var sentinelWorker = GetValidSentinelWorker();
-            lock (sentinelWorker)
-            {
-                return sentinelWorker.GetSentinelInfo();
-            }
+            return sentinelWorker.GetSentinelInfo();
         }
 
         public void Dispose()

src/ServiceStack.Redis/RedisSentinelWorker.cs

@@ -9,6 +9,11 @@ internal class RedisSentinelWorker : IDisposable
     {
         protected static readonly ILog Log = LogManager.GetLogger(typeof(RedisSentinelWorker));
 
+        static int IdCounter = 0;
+        public int Id { get; }
+
+        private readonly object oLock = new object();
+
         private readonly RedisSentinel sentinel;
         private readonly RedisClient sentinelClient;
         private RedisPubSubServer sentinePubSub;
@@ -17,6 +22,7 @@ internal class RedisSentinelWorker : IDisposable
 
         public RedisSentinelWorker(RedisSentinel sentinel, RedisEndpoint sentinelEndpoint)
         {
+            this.Id = Interlocked.Increment(ref IdCounter);
             this.sentinel = sentinel;
             this.sentinelClient = new RedisClient(sentinelEndpoint) {
                 Db = 0, //Sentinel Servers doesn't support DB, reset to 0
@@ -95,7 +101,10 @@ internal string GetMasterHost(string masterName)
 
         private string GetMasterHostInternal(string masterName)
         {
-            var masterInfo = sentinelClient.SentinelGetMasterAddrByName(masterName);
+            List<string> masterInfo;
+            lock (oLock)
+                masterInfo = sentinelClient.SentinelGetMasterAddrByName(masterName);
+
             return masterInfo.Count > 0
                 ? SanitizeMasterConfig(masterInfo)
                 : null;
@@ -114,12 +123,21 @@ private string SanitizeMasterConfig(List<string> masterInfo)
 
         internal List<string> GetSentinelHosts(string masterName)
         {
-            return SanitizeHostsConfig(this.sentinelClient.SentinelSentinels(sentinel.MasterName));
+            List<Dictionary<string, string>> sentinelSentinels;
+            lock (oLock)
+                sentinelSentinels = this.sentinelClient.SentinelSentinels(sentinel.MasterName);
+
+            return SanitizeHostsConfig(sentinelSentinels);
         }
 
         internal List<string> GetSlaveHosts(string masterName)
         {
-            return SanitizeHostsConfig(this.sentinelClient.SentinelSlaves(sentinel.MasterName));
+            List<Dictionary<string, string>> sentinelSlaves;
+
+            lock (oLock)
+                sentinelSlaves = sentinelClient.SentinelSlaves(sentinel.MasterName);
+
+            return SanitizeHostsConfig(sentinelSlaves);
         }
 
         private List<string> SanitizeHostsConfig(IEnumerable<Dictionary<string, string>> slaves)
@@ -146,27 +164,30 @@ public void BeginListeningForConfigurationChanges()
         {
             try
             {
-                if (this.sentinePubSub == null)
+                lock (oLock)
                 {
-                    var sentinelManager = new BasicRedisClientManager(sentinel.SentinelHosts, sentinel.SentinelHosts) 
+                    if (this.sentinePubSub == null)
                     {
-                        //Use BasicRedisResolver which doesn't validate non-Master Sentinel instances
-                        RedisResolver = new BasicRedisResolver(sentinel.SentinelEndpoints, sentinel.SentinelEndpoints)
-                    };
-                    this.sentinePubSub = new RedisPubSubServer(sentinelManager)
-                    {
-                        HeartbeatInterval = null,
-                        IsSentinelSubscription = true,
-                        ChannelsMatching = new[] { RedisPubSubServer.AllChannelsWildCard },
-                        OnMessage = SentinelMessageReceived
-                    };
+                        var sentinelManager = new BasicRedisClientManager(sentinel.SentinelHosts, sentinel.SentinelHosts)
+                        {
+                            //Use BasicRedisResolver which doesn't validate non-Master Sentinel instances
+                            RedisResolver = new BasicRedisResolver(sentinel.SentinelEndpoints, sentinel.SentinelEndpoints)
+                        };
+                        this.sentinePubSub = new RedisPubSubServer(sentinelManager)
+                        {
+                            HeartbeatInterval = null,
+                            IsSentinelSubscription = true,
+                            ChannelsMatching = new[] { RedisPubSubServer.AllChannelsWildCard },
+                            OnMessage = SentinelMessageReceived
+                        };
+                    }
                 }
+
                 this.sentinePubSub.Start();
             }
             catch (Exception ex)
             {
-                Log.Error("Error Subscribing to Redis Channel on {0}:{1}"
-                    .Fmt(this.sentinelClient.Host, this.sentinelClient.Port), ex);
+                Log.Error($"Error Subscribing to Redis Channel on {this.sentinelClient.Host}:{this.sentinelClient.Port}", ex);
 
                 if (OnSentinelError != null)
                     OnSentinelError(ex);
@@ -175,7 +196,8 @@ public void BeginListeningForConfigurationChanges()
 
         public void ForceMasterFailover(string masterName)
         {
-            this.sentinelClient.SentinelFailover(masterName);
+            lock (oLock)
+                this.sentinelClient.SentinelFailover(masterName);
         }
 
         public void Dispose()