Repository navigation
Fail connection-dropped commands with RedisConnectionException, never a raw transport exception or a cancellation - #3287
Merged
Merged
Conversation
… raw or cancelled (#3283) When a connection failed, most queued commands failed with RedisConnectionException but a few received the raw transport exception (IOException, or the RESPite writer's InvalidOperationException) or a TaskCanceledException with no token in play. Three sources, all confirmed with the reporter's repro: - RecordConnectionFailed builds its detailed RedisConnectionException only for the bridge's current connection and only on the first report; a repeat report (the write path and the read loop both reporting), or one against a connection the bridge has already replaced, drained the queue with the raw inner exception - or with null, which the result box turns into a cancellation (every EOF path in the read loop reports with no exception). - The write-lock catch fails the message properly and then reports the connection, which drained the same still-queued message again with the raw exception; last write wins. - FailDueToNoConnection, TimedOutBeforeWrite and the fail-fast bin completed the message via Cancel(); ExecuteAsyncImpl then races to fault the task with the real exception, and the box's thread-pool continuation sometimes wins. A batch has no caller translating the result at all. Fix: the drain always uses a RedisConnectionException (wrapping whatever it was given, with the failure type), and the bridge's refusals set the exception the caller path would have built instead of cancelling. Message.Cancel itself is unchanged; transactions rely on it for aborted conditions. ConnectionFailureExceptionTests covers the drain (null and IOException inner, on a bridge-less connection, i.e. the non-current path) and the no-connection refusal observed at the message's own task. Reporter's repro, 90 s: 884 TaskCanceledException + 415 InvalidOperationException before; none after. Fixes #3283
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3283
Causes (all three in the report confirmed)
RecordConnectionFailedbuilds its detailedRedisConnectionExceptiononly for the bridge's current connection and only on the first report. A repeat report (write path and read loop both reporting), or one against a connection the bridge has already replaced, drains_writtenAwaitingResponsewith the raw inner exception, or withnull, whichTaskResultBoxturns intoTaskCanceledException. Every EOF path in the read loop reports with no exception.InvalidOperationExceptionin the reported stack trace.Cancel().FailDueToNoConnection,TimedOutBeforeWriteand the fail-fast bin complete the message as cancelled.ExecuteAsyncImplthen tries to fault the task with the real exception, but the box's continuation is queued to the thread pool and sometimes gets there first. A batch has no such translation at all.Changes
RecordConnectionFailed: if the exception chosen for the drain is not already aRedisException, wrap it in aRedisConnectionExceptioncarrying the failure type and the original asInnerException.PhysicalBridge: the three refusal paths set the same exception the caller path would have built (ExceptionFactory.NoConnectionAvailable/Timeout) instead of cancelling.Message.Cancelitself is untouched; transactions rely on it for aborted conditions.ConnectionFailureExceptionTests: the drain withnulland with anIOExceptioninner, on a bridge-less dummy connection (exactly the non-current path), plus the no-connection refusal observed at the message's own task rather than through the caller's translation.Verification
TaskCanceledException, 415InvalidOperationExceptionokandRedisConnectionExceptionFailoverTests.SubscriptionsSurvivePrimarySwitchAsync, pre-existing flake, same rate onmain)dotnet build Build.csproj -c Release /p:CI=trueNote:
TimedOutBeforeWritenow builds the full timeout exception in the bridge while the single-command caller still builds its own from the result, so that path allocates twice. It only runs when the write lock cannot be taken within the timeout; left as is rather than restructure the caller.Related: #3282 (#3286), the batch hang from the same report series.