Fix for #1988 - respect SELECT missing from command map

This resolves #1988 by respecting when the `SELECT` command has been disabled. It's memoized which seems bit extreme here but that's because we're inside the lock and every op matters. Measured locally to ensure no regressions.
StackExchange · Mar 6, 2022 · cf4daaa · cf4daaa
1 parent f1a84f9
commit cf4daaa
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 7 deletions.
diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs
@@ -1372,7 +1372,7 @@ private WriteResult WriteMessageToServerInsideWriteLock(PhysicalConnection conne
                 LastCommand = cmd;
                 bool isMasterOnly = message.IsMasterOnly();
 
-                if (isMasterOnly && ServerEndPoint.IsReplica && (ServerEndPoint.ReplicaReadOnly || !ServerEndPoint.AllowReplicaWrites))
+                if (isMasterOnly && !ServerEndPoint.SupportsPrimaryWrites)
                 {
                     throw ExceptionFactory.MasterOnly(Multiplexer.IncludeDetailInExceptions, message.Command, message, ServerEndPoint);
                 }
@@ -1447,7 +1447,10 @@ private WriteResult WriteMessageToServerInsideWriteLock(PhysicalConnection conne
                     case RedisCommand.UNKNOWN:
                     case RedisCommand.DISCARD:
                     case RedisCommand.EXEC:
-                        connection.SetUnknownDatabase();
+                        if (ServerEndPoint.SupportsDatabases)
+                        {
+                            connection.SetUnknownDatabase();
+                        }
                         break;
                 }
                 return WriteResult.Success;

diff --git a/src/StackExchange.Redis/PhysicalConnection.cs b/src/StackExchange.Redis/PhysicalConnection.cs
@@ -573,7 +573,8 @@ internal Message GetSelectDatabaseCommand(int targetDatabase, Message message)
             }
             int available = serverEndpoint.Databases;
 
-            if (!serverEndpoint.HasDatabases) // only db0 is available on cluster/twemproxy/envoyproxy
+            // Only db0 is available on cluster/twemproxy/envoyproxy
+            if (!serverEndpoint.SupportsDatabases)
             {
                 if (targetDatabase != 0)
                 { // should never see this, since the API doesn't allow it; thus not too worried about ExceptionFactory

diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs
@@ -32,9 +32,9 @@ internal sealed partial class ServerEndPoint : IDisposable
 
         private int databases, writeEverySeconds;
         private PhysicalBridge interactive, subscription;
-        private bool isDisposed;
+        private bool isDisposed, replicaReadOnly, isReplica, allowReplicaWrites;
+        private bool? supportsDatabases, supportsPrimaryWrites;
         private ServerType serverType;
-        private bool replicaReadOnly, isReplica;
         private volatile UnselectableFlags unselectableReasons;
         private Version version;
 
@@ -76,15 +76,25 @@ public ServerEndPoint(ConnectionMultiplexer multiplexer, EndPoint endpoint)
         public int Databases { get { return databases; } set { SetConfig(ref databases, value); } }
 
         public EndPoint EndPoint { get; }
+        /// <summary>
+        /// Whether this endpoint supports databases at all.
+        /// Note that some servers are cluster but present as standalone (e.g. Redis Enterprise), so we respect
+        /// <see cref="RedisCommand.SELECT"/> being disabled here as a performance workaround.
+        /// </summary>
+        /// <remarks>
+        /// This is memoized because it's accessed on hot paths inside the write lock.
+        /// </remarks>
+        public bool SupportsDatabases =>
+            supportsDatabases ??= (serverType == ServerType.Standalone && Multiplexer.RawConfig.CommandMap.IsAvailable(RedisCommand.SELECT));
 
-        public bool HasDatabases => serverType == ServerType.Standalone;
 
         public bool IsConnected => interactive?.IsConnected == true;
         public bool IsSubscriberConnected => subscription?.IsConnected == true;
 
         public bool SupportsSubscriptions => Multiplexer.CommandMap.IsAvailable(RedisCommand.SUBSCRIBE);
 
         public bool IsConnecting => interactive?.IsConnecting == true;
+        public bool SupportsPrimaryWrites => supportsPrimaryWrites ??= (!IsReplica || !ReplicaReadOnly || AllowReplicaWrites);
 
         private readonly List<TaskCompletionSource<string>> _pendingConnectionMonitors = new List<TaskCompletionSource<string>>();
 
@@ -176,7 +186,15 @@ public bool ReplicaReadOnly
             set => SetConfig(ref replicaReadOnly, value);
         }
 
-        public bool AllowReplicaWrites { get; set; }
+        public bool AllowReplicaWrites
+        {
+            get => allowReplicaWrites;
+            set
+            {
+                allowReplicaWrites = value;
+                ClearMemoized();
+            }
+        }
 
         public Version Version
         {
@@ -946,10 +964,17 @@ private void SetConfig<T>(ref T field, T value, [CallerMemberName] string caller
             {
                 Multiplexer.Trace(caller + " changed from " + field + " to " + value, "Configuration");
                 field = value;
+                ClearMemoized();
                 Multiplexer.ReconfigureIfNeeded(EndPoint, false, caller);
             }
         }
 
+        private void ClearMemoized()
+        {
+            supportsDatabases = null;
+            supportsPrimaryWrites = null;
+        }
+
         /// <summary>
         /// For testing only
         /// </summary>