diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs index 7cf39edeb..eb81a8170 100644 --- a/src/StackExchange.Redis/PhysicalBridge.cs +++ b/src/StackExchange.Redis/PhysicalBridge.cs @@ -193,17 +193,18 @@ private WriteResult QueueOrFailMessage(Message message) } // Anything else goes in the bin - we're just not ready for you yet - message.Cancel(); - Multiplexer.OnMessageFaulted(message, null); - message.Complete(null); - return WriteResult.NoConnectionAvailable; + return FailDueToNoConnection(message); } private WriteResult FailDueToNoConnection(Message message) { - message.Cancel(); - Multiplexer.OnMessageFaulted(message, null); - message.Complete(null); + // fail with the exception the caller would otherwise construct from the result, rather than Cancel(): + // a cancelled box surfaces as TaskCanceledException, and although ExecuteAsyncImpl then tries to + // fault the task with the real exception, the box's continuation can get there first (#3283) - and a + // batch has no caller translating the result at all + var ex = ExceptionFactory.NoConnectionAvailable(Multiplexer, message, ServerEndPoint); + Multiplexer.OnMessageFaulted(message, ex); + message.SetExceptionAndComplete(ex, null); return WriteResult.NoConnectionAvailable; } @@ -1368,9 +1369,10 @@ public bool HasPendingCallerFacingItems() private WriteResult TimedOutBeforeWrite(Message message) { - message.Cancel(); - Multiplexer.OnMessageFaulted(message, null); - message.Complete(null); + // as FailDueToNoConnection: a real exception, not Cancel() + var ex = ExceptionFactory.Timeout(Multiplexer, null, message, ServerEndPoint, WriteResult.TimeoutBeforeWrite, this); + Multiplexer.OnMessageFaulted(message, ex); + message.SetExceptionAndComplete(ex, null); return WriteResult.TimeoutBeforeWrite; } diff --git a/src/StackExchange.Redis/PhysicalConnection.cs b/src/StackExchange.Redis/PhysicalConnection.cs index c960e4230..9e5eab6a1 100644 --- a/src/StackExchange.Redis/PhysicalConnection.cs +++ b/src/StackExchange.Redis/PhysicalConnection.cs @@ -696,6 +696,14 @@ void AddData(string? lk, string? sk, string? v) } var ex = innerException is RedisException ? innerException : outerException; + if (ex is not RedisException) + { + // the detailed exception above is built once, for the bridge's *current* connection; a repeat + // report for the same connection, or one the bridge has already replaced, must still fail + // whatever is queued - and with a RedisConnectionException, not the raw transport fault, nor + // (when there is no exception at all, e.g. EOF) a TaskCanceledException nobody asked for (#3283) + ex = new RedisConnectionException(failureType, CommandFlags.None, failureType + " on " + ToString(), innerException); + } nextMessage = Interlocked.Exchange(ref _awaitingToken, null); if (nextMessage is not null) diff --git a/tests/StackExchange.Redis.Tests/ConnectionFailureExceptionTests.cs b/tests/StackExchange.Redis.Tests/ConnectionFailureExceptionTests.cs new file mode 100644 index 000000000..79a8d84e1 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/ConnectionFailureExceptionTests.cs @@ -0,0 +1,78 @@ +using System; +using System.IO; +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// A command that fails because its connection failed must fail with a +/// carrying the transport fault as its inner exception: never the raw transport exception, and never a +/// when no caller token was involved (#3283). +/// +public class ConnectionFailureExceptionTests(ITestOutputHelper log) +{ + private static (Message Message, Task Task) CreateTrackedPing() + { + var message = Message.Create(-1, CommandFlags.None, RedisCommand.PING); + var box = TaskResultBox.Create(out var tcs, null); + message.SetSource(box, ResultProcessor.DemandOK); + return (message, tcs.Task); + } + + private static async Task AwaitFailure(Task task) + { + var completed = await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(10))); + Assert.Same(task, completed); + return await Assert.ThrowsAnyAsync(() => task); + } + + [Theory] + [InlineData(false)] // EOF-style report: no exception at all (used to surface as TaskCanceledException) + [InlineData(true)] // transport fault (used to surface raw when the detailed report was not built) + public async Task DrainedMessagesFailWithRedisConnectionException(bool withInner) + { + // a dummy connection has no bridge, so it is never the bridge's *current* connection and the detailed + // first-report exception is not built: this is exactly the path that handed out raw/null exceptions + using var stream = new MemoryStream(); + using var connection = PhysicalConnection.Dummy(stream); + var (message, task) = CreateTrackedPing(); + connection.EnqueueInsideWriteLock(message, enforceMuxer: false); + + var inner = withInner ? new IOException("simulated transport fault") : null; + connection.RecordConnectionFailed(ConnectionFailureType.SocketClosed, inner); + + var ex = await AwaitFailure(task); + log.WriteLine(ex.ToString()); + var conn = Assert.IsType(ex); + Assert.Equal(ConnectionFailureType.SocketClosed, conn.FailureType); + Assert.Same(inner, conn.InnerException); + } + + [Fact] + public async Task NoConnectionFailsWithRedisConnectionExceptionNotCancellation() + { + using var server = new InProcessTestServer(log); + var config = server.GetClientConfig(withPubSub: false, defaultOnly: true); + config.BacklogPolicy = BacklogPolicy.FailFast; + config.AbortOnConnectFail = false; + config.AllowSimulateConnectionFailure = true; + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + var endpoint = conn.GetServerSnapshot()[0]; + var bridge = endpoint.GetBridge(ConnectionType.Interactive)!; + + conn.AllowConnect = false; + endpoint.SimulateConnectionFailure(SimulatedFailureType.All); + Assert.False(conn.IsConnected); + + // write at the bridge, so we observe the message's own outcome rather than the caller's translation of + // the write result (which is what hid this for single commands - most of the time) + var (message, task) = CreateTrackedPing(); + Assert.Equal(WriteResult.NoConnectionAvailable, await bridge.TryWriteAsync(message, isReplica: false)); + + var ex = await AwaitFailure(task); + log.WriteLine(ex.ToString()); + Assert.IsType(ex); + Assert.False(task.IsCanceled); + } +}