adding some smarts to GetFeatures (#2191)

@NickCraver - This will fix #2016 

The issue is that when `GetFeatures` does the server selection, it uses `PING` as the command it's asking about, so when the multiplexer responds with a server, it responds with any server. Of course, then when it goes to execute the command, it's been explicitly handed a server, so it honors that choice but checks it to make sure it's a valid server to send the command to, so if you're executing a write command and `GetFeatuers` just so happened to output a read-only replica, when the multiplexer does the 'is this a valid server?' check, it determines it is not and propagates the error. that's what causes the blowup. By passing in the RedisCommand to `GetFeatures`, we allow it to make an informed choice of server for that Redis Command.

The other option is to simply not pass the server on down the line when we we've done a feature check, and allow the muxer to make it's own decision, this would cause an extra run of `Select` which the current pattern e.g. see [sort](https://github.com/StackExchange/StackExchange.Redis/blob/main/src/StackExchange.Redis/RedisDatabase.cs#L1816) tries to avoid

One weird case, with SORT/SORT_RO, if the RO command is what we prefer to run (because we don't have a destination key to output it to), and `SORT_RO` is not available (because they aren't on Redis 7+), we cannot trust the output of GetFeatures, so I just set the selected server back to null and let the muxer figure it out down the line.

Co-authored-by: Nick Craver <[email protected]>

Author: slorello89

docs/ReleaseNotes.md

@@ -4,6 +4,7 @@
 
 - Fix [#2182](https://github.com/StackExchange/StackExchange.Redis/issues/2182): Be more flexible in which commands are "primary only" in order to support users with replicas that are explicitly configured to allow writes ([#2183 by slorello89](https://github.com/StackExchange/StackExchange.Redis/pull/2183))
 - Adds: `IConnectionMultiplexer` now implements `IAsyncDisposable` ([#2161 by kimsey0](https://github.com/StackExchange/StackExchange.Redis/pull/2161))
+- Fix [#2016](https://github.com/StackExchange/StackExchange.Redis/issues/2016): Align server selection with supported commands (e.g. with writable servers) to reduce `Command cannot be issued to a replica` errors ([#2191 by slorello89](https://github.com/StackExchange/StackExchange.Redis/pull/2191))
 
 ## 2.6.48
 

src/Directory.Build.props

@@ -7,7 +7,6 @@
   <ItemGroup>
     <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" PrivateAssets="all" />
     <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="all" />
-    <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" PrivateAssets="All" />
     <PackageReference Include="Nerdbank.GitVersioning" PrivateAssets="all" Condition=" '$(DEVCONTAINER)' != 'true' " />
   </ItemGroup>
 </Project>

src/StackExchange.Redis/RedisBase.cs

@@ -62,9 +62,9 @@ internal virtual Task<T> ExecuteAsync<T>(Message? message, ResultProcessor<T>? p
             return multiplexer.ExecuteSyncImpl<T>(message, processor, server, defaultValue);
         }
 
-        internal virtual RedisFeatures GetFeatures(in RedisKey key, CommandFlags flags, out ServerEndPoint? server)
+        internal virtual RedisFeatures GetFeatures(in RedisKey key, CommandFlags flags, RedisCommand command, out ServerEndPoint? server)
         {
-            server = multiplexer.SelectServer(RedisCommand.PING, flags, key);
+            server = multiplexer.SelectServer(command, flags, key);
             var version = server == null ? multiplexer.RawConfig.DefaultVersion : server.Version;
             return new RedisFeatures(version);
         }

src/StackExchange.Redis/RedisDatabase.cs

@@ -635,7 +635,7 @@ public Task<bool> HyperLogLogAddAsync(RedisKey key, RedisValue[] values, Command
 
         public long HyperLogLogLength(RedisKey key, CommandFlags flags = CommandFlags.None)
         {
-            var features = GetFeatures(key, flags, out ServerEndPoint? server);
+            var features = GetFeatures(key, flags, RedisCommand.PFCOUNT, out ServerEndPoint? server);
             var cmd = Message.Create(Database, flags, RedisCommand.PFCOUNT, key);
             // technically a write / primary-only command until 2.8.18
             if (server != null && !features.HyperLogLogCountReplicaSafe) cmd.SetPrimaryOnly();
@@ -649,7 +649,7 @@ public long HyperLogLogLength(RedisKey[] keys, CommandFlags flags = CommandFlags
             var cmd = Message.Create(Database, flags, RedisCommand.PFCOUNT, keys);
             if (keys.Length != 0)
             {
-                var features = GetFeatures(keys[0], flags, out server);
+                var features = GetFeatures(keys[0], flags, RedisCommand.PFCOUNT, out server);
                 // technically a write / primary-only command until 2.8.18
                 if (server != null && !features.HyperLogLogCountReplicaSafe) cmd.SetPrimaryOnly();
             }
@@ -658,7 +658,7 @@ public long HyperLogLogLength(RedisKey[] keys, CommandFlags flags = CommandFlags
 
         public Task<long> HyperLogLogLengthAsync(RedisKey key, CommandFlags flags = CommandFlags.None)
         {
-            var features = GetFeatures(key, flags, out ServerEndPoint? server);
+            var features = GetFeatures(key, flags, RedisCommand.PFCOUNT, out ServerEndPoint? server);
             var cmd = Message.Create(Database, flags, RedisCommand.PFCOUNT, key);
             // technically a write / primary-only command until 2.8.18
             if (server != null && !features.HyperLogLogCountReplicaSafe) cmd.SetPrimaryOnly();
@@ -672,7 +672,7 @@ public Task<long> HyperLogLogLengthAsync(RedisKey[] keys, CommandFlags flags = C
             var cmd = Message.Create(Database, flags, RedisCommand.PFCOUNT, keys);
             if (keys.Length != 0)
             {
-                var features = GetFeatures(keys[0], flags, out server);
+                var features = GetFeatures(keys[0], flags, RedisCommand.PFCOUNT, out server);
                 // technically a write / primary-only command until 2.8.18
                 if (server != null && !features.HyperLogLogCountReplicaSafe) cmd.SetPrimaryOnly();
             }
@@ -773,7 +773,7 @@ public Task<long> KeyDeleteAsync(RedisKey[] keys, CommandFlags flags = CommandFl
 
         private RedisCommand GetDeleteCommand(RedisKey key, CommandFlags flags, out ServerEndPoint? server)
         {
-            var features = GetFeatures(key, flags, out server);
+            var features = GetFeatures(key, flags, RedisCommand.UNLINK, out server);
             if (server != null && features.Unlink && multiplexer.CommandMap.IsAvailable(RedisCommand.UNLINK))
             {
                 return RedisCommand.UNLINK;
@@ -1036,7 +1036,7 @@ public Task KeyRestoreAsync(RedisKey key, byte[] value, TimeSpan? expiry = null,
 
         public TimeSpan? KeyTimeToLive(RedisKey key, CommandFlags flags = CommandFlags.None)
         {
-            var features = GetFeatures(key, flags, out ServerEndPoint? server);
+            var features = GetFeatures(key, flags, RedisCommand.TTL, out ServerEndPoint? server);
             Message msg;
             if (server != null && features.MillisecondExpiry && multiplexer.CommandMap.IsAvailable(RedisCommand.PTTL))
             {
@@ -1049,7 +1049,7 @@ public Task KeyRestoreAsync(RedisKey key, byte[] value, TimeSpan? expiry = null,
 
         public Task<TimeSpan?> KeyTimeToLiveAsync(RedisKey key, CommandFlags flags = CommandFlags.None)
         {
-            var features = GetFeatures(key, flags, out ServerEndPoint? server);
+            var features = GetFeatures(key, flags, RedisCommand.TTL, out ServerEndPoint? server);
             Message msg;
             if (server != null && features.MillisecondExpiry && multiplexer.CommandMap.IsAvailable(RedisCommand.PTTL))
             {
@@ -3278,7 +3278,7 @@ private Message GetExpiryMessage(in RedisKey key,
             server = null;
             if ((milliseconds % 1000) != 0)
             {
-                var features = GetFeatures(key, flags, out server);
+                var features = GetFeatures(key, flags, RedisCommand.PEXPIRE, out server);
                 if (server is not null && features.MillisecondExpiry && multiplexer.CommandMap.IsAvailable(millisecondsCommand))
                 {
                     return when switch
@@ -3675,10 +3675,17 @@ private Message GetSortedSetAddMessage(RedisKey key, RedisValue member, double s
         private Message GetSortMessage(RedisKey destination, RedisKey key, long skip, long take, Order order, SortType sortType, RedisValue by, RedisValue[]? get, CommandFlags flags, out ServerEndPoint? server)
         {
             server = null;
-            var command = destination.IsNull && GetFeatures(key, flags, out server).ReadOnlySort
+            var command = destination.IsNull && GetFeatures(key, flags, RedisCommand.SORT_RO, out server).ReadOnlySort
                 ? RedisCommand.SORT_RO
                 : RedisCommand.SORT;
 
+            //if SORT_RO is not available, we cannot issue the command to a read-only replica
+            if (command == RedisCommand.SORT)
+            {
+                server = null;
+            }
+
+
             // most common cases; no "get", no "by", no "destination", no "skip", no "take"
             if (destination.IsNull && skip == 0 && take == -1 && by.IsNull && (get == null || get.Length == 0))
             {
@@ -4338,7 +4345,7 @@ private Message GetStringGetWithExpiryMessage(RedisKey key, CommandFlags flags,
             {
                 throw new NotSupportedException("This operation is not possible inside a transaction or batch; please issue separate GetString and KeyTimeToLive requests");
             }
-            var features = GetFeatures(key, flags, out server);
+            var features = GetFeatures(key, flags, RedisCommand.PTTL, out server);
             processor = StringGetWithExpiryProcessor.Default;
             if (server != null && features.MillisecondExpiry && multiplexer.CommandMap.IsAvailable(RedisCommand.PTTL))
             {
@@ -4495,7 +4502,7 @@ private Message GetStringSetAndGetMessage(
                 throw new ArgumentOutOfRangeException(nameof(pageSize));
             if (!multiplexer.CommandMap.IsAvailable(command)) return null;
 
-            var features = GetFeatures(key, flags, out server);
+            var features = GetFeatures(key, flags, RedisCommand.SCAN, out server);
             if (!features.Scan) return null;
 
             if (CursorUtils.IsNil(pattern)) pattern = (byte[]?)null;

src/StackExchange.Redis/RedisServer.cs

@@ -742,7 +742,7 @@ internal override Task<T> ExecuteAsync<T>(Message? message, ResultProcessor<T>?
             return base.ExecuteSync<T>(message, processor, server, defaultValue);
         }
 
-        internal override RedisFeatures GetFeatures(in RedisKey key, CommandFlags flags, out ServerEndPoint server)
+        internal override RedisFeatures GetFeatures(in RedisKey key, CommandFlags flags, RedisCommand command, out ServerEndPoint server)
         {
             server = this.server;
             return server.GetFeatures();

src/StackExchange.Redis/RedisSubscriber.cs

@@ -307,7 +307,7 @@ private Message CreatePingMessage(CommandFlags flags)
             bool usePing = false;
             if (multiplexer.CommandMap.IsAvailable(RedisCommand.PING))
             {
-                try { usePing = GetFeatures(default, flags, out _).PingOnSubscriber; }
+                try { usePing = GetFeatures(default, flags, RedisCommand.PING, out _).PingOnSubscriber; }
                 catch { }
             }