Fix for #1967: Sanitizing message failures (#1990)

There seems to be just 1 case not covered by the `IncludeDetailsInExceptions` option on `ConnectionMultiplexer` - this remedies that. The option is on by default so this shouldn't break people like I thought initially.

Overall, we should probably also move this option to `ConfigurationOptions` and defaults if we do that (#1987).

Author: NickCraver

docs/ReleaseNotes.md

@@ -15,6 +15,7 @@
   - Fixes subscription routing on clusters (spreading instead of choosing 1 node)
   - More correctly reconnects subscriptions on connection failures, including to other endpoints
 - Adds "(vX.X.X)" version suffix to the default client ID so server-side `CLIENT LIST` can more easily see what's connected (#1985 via NickCraver)
+- Fix for including (or not including) key names on some message failures (#1990 via NickCraver)
 
 ## 2.2.88
 

src/StackExchange.Redis/Message.cs

@@ -403,9 +403,18 @@ public virtual void AppendStormLog(StringBuilder sb)
         /// </summary>
         public void SetInternalCall() => Flags |= InternalCallFlag;
 
+        /// <summary>
+        /// Gets a string representation of this message: "[{DB}]:{CommandAndKey} ({resultProcessor})"
+        /// </summary>
         public override string ToString() =>
             $"[{Db}]:{CommandAndKey} ({resultProcessor?.GetType().Name ?? "(n/a)"})";
 
+        /// <summary>
+        /// Gets a string representation of this message without the key: "[{DB}]:{Command} ({resultProcessor})"
+        /// </summary>
+        public string ToStringCommandOnly() =>
+            $"[{Db}]:{Command} ({resultProcessor?.GetType().Name ?? "(n/a)"})";
+
         public void SetResponseReceived() => performance?.SetResponseReceived();
 
         bool ICompletable.TryComplete(bool isAsync) { Complete(); return true; }
@@ -562,10 +571,10 @@ internal bool ComputeResult(PhysicalConnection connection, in RawResult result)
             }
         }
 
-        internal void Fail(ConnectionFailureType failure, Exception innerException, string annotation)
+        internal void Fail(ConnectionFailureType failure, Exception innerException, string annotation, ConnectionMultiplexer muxer)
         {
             PhysicalConnection.IdentifyFailureType(innerException, ref failure);
-            resultProcessor?.ConnectionFail(this, failure, innerException, annotation);
+            resultProcessor?.ConnectionFail(this, failure, innerException, annotation, muxer);
         }
 
         internal virtual void SetExceptionAndComplete(Exception exception, PhysicalBridge bridge)
@@ -717,7 +726,7 @@ internal void WriteTo(PhysicalConnection physical)
             catch (Exception ex) when (ex is not RedisCommandException) // these have specific meaning; don't wrap
             {
                 physical?.OnInternalError(ex);
-                Fail(ConnectionFailureType.InternalFailure, ex, null);
+                Fail(ConnectionFailureType.InternalFailure, ex, null, physical?.BridgeCouldBeNull?.Multiplexer);
             }
         }
 

src/StackExchange.Redis/PhysicalBridge.cs

@@ -668,7 +668,7 @@ private WriteResult WriteMessageInsideLock(PhysicalConnection physical, Message
                         // we screwed up; abort; note that WriteMessageToServer already
                         // killed the underlying connection
                         Trace("Unable to write to server");
-                        message.Fail(ConnectionFailureType.ProtocolFailure, null, "failure before write: " + result.ToString());
+                        message.Fail(ConnectionFailureType.ProtocolFailure, null, "failure before write: " + result.ToString(), Multiplexer);
                         message.Complete();
                         return result;
                     }
@@ -1425,7 +1425,7 @@ private WriteResult WriteMessageToServerInsideWriteLock(PhysicalConnection conne
             catch (RedisCommandException ex) when (!isQueued)
             {
                 Trace("Write failed: " + ex.Message);
-                message.Fail(ConnectionFailureType.InternalFailure, ex, null);
+                message.Fail(ConnectionFailureType.InternalFailure, ex, null, Multiplexer);
                 message.Complete();
                 // this failed without actually writing; we're OK with that... unless there's a transaction
 
@@ -1440,7 +1440,7 @@ private WriteResult WriteMessageToServerInsideWriteLock(PhysicalConnection conne
             catch (Exception ex)
             {
                 Trace("Write failed: " + ex.Message);
-                message.Fail(ConnectionFailureType.InternalFailure, ex, null);
+                message.Fail(ConnectionFailureType.InternalFailure, ex, null, Multiplexer);
                 message.Complete();
 
                 // we're not sure *what* happened here; probably an IOException; kill the connection

src/StackExchange.Redis/RedisBatch.cs

@@ -27,13 +27,13 @@ public void Execute()
                 var server = multiplexer.SelectServer(message);
                 if (server == null)
                 {
-                    FailNoServer(snapshot);
+                    FailNoServer(multiplexer, snapshot);
                     throw ExceptionFactory.NoConnectionAvailable(multiplexer, message, server);
                 }
                 var bridge = server.GetBridge(message);
                 if (bridge == null)
                 {
-                    FailNoServer(snapshot);
+                    FailNoServer(multiplexer, snapshot);
                     throw ExceptionFactory.NoConnectionAvailable(multiplexer, message, server);
                 }
 
@@ -58,7 +58,7 @@ public void Execute()
             {
                 if (!pair.Key.TryEnqueue(pair.Value, pair.Key.ServerEndPoint.IsReplica))
                 {
-                    FailNoServer(pair.Value);
+                    FailNoServer(multiplexer, pair.Value);
                 }
             }
         }
@@ -89,12 +89,12 @@ internal override Task<T> ExecuteAsync<T>(Message message, ResultProcessor<T> pr
         internal override T ExecuteSync<T>(Message message, ResultProcessor<T> processor, ServerEndPoint server = null) =>
             throw new NotSupportedException("ExecuteSync cannot be used inside a batch");
 
-        private static void FailNoServer(List<Message> messages)
+        private static void FailNoServer(ConnectionMultiplexer muxer, List<Message> messages)
         {
             if (messages == null) return;
             foreach(var msg in messages)
             {
-                msg.Fail(ConnectionFailureType.UnableToResolvePhysicalConnection, null, "unable to write batch");
+                msg.Fail(ConnectionFailureType.UnableToResolvePhysicalConnection, null, "unable to write batch", muxer);
                 msg.Complete();
             }
         }

src/StackExchange.Redis/RedisTransaction.cs

@@ -523,7 +523,7 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes
                     {
                         if (op?.Wrapped is Message inner)
                         {
-                            inner.Fail(ConnectionFailureType.ProtocolFailure, null, "Transaction failure");
+                            inner.Fail(ConnectionFailureType.ProtocolFailure, null, "Transaction failure", connection?.BridgeCouldBeNull?.Multiplexer);
                             inner.Complete();
                         }
                     }

src/StackExchange.Redis/ResultProcessor.cs

@@ -5,6 +5,7 @@
 using System.IO;
 using System.Linq;
 using System.Net;
+using System.Text;
 using System.Text.RegularExpressions;
 using Pipelines.Sockets.Unofficial.Arenas;
 
@@ -150,16 +151,22 @@ public static readonly HashEntryArrayProcessor
             HashEntryArray = new HashEntryArrayProcessor();
 
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "Conditionally run on instance")]
-        public void ConnectionFail(Message message, ConnectionFailureType fail, Exception innerException, string annotation)
+        public void ConnectionFail(Message message, ConnectionFailureType fail, Exception innerException, string annotation, ConnectionMultiplexer muxer)
         {
             PhysicalConnection.IdentifyFailureType(innerException, ref fail);
 
-            string exMessage = fail.ToString() + (message == null ? "" : (" on " + (
-                fail == ConnectionFailureType.ProtocolFailure ? message.ToString() : message.CommandAndKey)));
-            if (!string.IsNullOrWhiteSpace(annotation)) exMessage += ", " + annotation;
-
-            var ex = innerException == null ? new RedisConnectionException(fail, exMessage)
-                : new RedisConnectionException(fail, exMessage, innerException);
+            var sb = new StringBuilder(fail.ToString());
+            if (message is not null)
+            {
+                sb.Append(" on ");
+                sb.Append(muxer?.IncludeDetailInExceptions == true ? message.ToString() : message.ToStringCommandOnly());
+            }
+            if (!string.IsNullOrWhiteSpace(annotation))
+            {
+                sb.Append(", ");
+                sb.Append(annotation);
+            }
+            var ex = new RedisConnectionException(fail, sb.ToString(), innerException);
             SetException(message, ex);
         }
 

tests/StackExchange.Redis.Tests/ExceptionFactoryTests.cs

@@ -212,5 +212,27 @@ public void NoConnectionException(bool abortOnConnect, int connCount, int comple
                 ClearAmbientFailures();
             }
         }
+
+        [Theory]
+        [InlineData(true, ConnectionFailureType.ProtocolFailure, "ProtocolFailure on [0]:GET myKey (StringProcessor), my annotation")]
+        [InlineData(true, ConnectionFailureType.ConnectionDisposed, "ConnectionDisposed on [0]:GET myKey (StringProcessor), my annotation")]
+        [InlineData(false, ConnectionFailureType.ProtocolFailure, "ProtocolFailure on [0]:GET (StringProcessor), my annotation")]
+        [InlineData(false, ConnectionFailureType.ConnectionDisposed, "ConnectionDisposed on [0]:GET (StringProcessor), my annotation")]
+        public void MessageFail(bool includeDetail, ConnectionFailureType failType, string messageStart)
+        {
+            using var muxer = Create(shared: false);
+            muxer.IncludeDetailInExceptions = includeDetail;
+
+            var message = Message.Create(0, CommandFlags.None, RedisCommand.GET, (RedisKey)"myKey");
+            var resultBox = SimpleResultBox<string>.Create();
+            message.SetSource(ResultProcessor.String, resultBox);
+
+            message.Fail(failType, null, "my annotation", muxer as ConnectionMultiplexer);
+
+            resultBox.GetResult(out var ex);
+            Assert.NotNull(ex);
+
+            Assert.StartsWith(messageStart, ex.Message);
+        }
     }
 }