Repository navigation
v4: a close with commands in flight no longer completes a call with another call's reply (#3329) - #3332
Merged
Conversation
…ose with commands in flight no longer completes a call with another call's reply RespConnection.Close drained _pending with a TryDequeue loop while the reader, still matching replies from the receive buffer, dequeued from the same ConcurrentQueue. The queue is safe for that; what it cannot promise is which entry each side gets. Close took entries 0..N-1 and the reader then took entry N for reply 0, so GET key N completed - successfully - with key 0's value, and everything behind it was shifted the same way. Measured with the reporter's loop against 4.0.155-alpha: 4,853 wrong replies in 20,000 calls on CloseAsync(allowCommandsToComplete: false), against 616 on main for the same race there. TryTakePending checks closed and dequeues as one step under _pendingLock; Close sets closed and then empties the queue under the same lock, failing what it took outside it. Once a close has begun the reader takes nothing, so a reply arriving late dies with the connection, and the command it answered is failed by Close as before. Same run afterwards: 0. The test is CloseInFlightTests from main (#3330): 20 rounds of 500 in-flight GETs against the real server, closed without waiting; no call may complete with another call's value. See #3329
…raised, not RESPite's InvalidOperationException
RespClientConnection already overrides CreateRemoteClosedFault so a server
close fails commands with RedisConnectionException(SocketClosed), as v3
did. Our own close - a dispose with commands in flight - went through
RespConnection's private fallback, a bare InvalidOperationException("The
connection is closed."), which is what CloseInFlightTests met and what a
caller catching RedisConnectionException around a shutdown would miss.
CreateClosedFault is the local-close counterpart, and the client
overrides it with ConnectionDisposed and a message naming the endpoint.
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.
The
v4fix for #3329 (main: #3330,v2: #3331). It does reproduce here - and far worse than onmain.What happens.
RespConnection.Closefails what is still unanswered by draining_pendingwith aTryDequeueloop. The reader, inDrain, dequeues from the sameConcurrentQueueas it matches replies out of the receive buffer. The queue is safe for two takers; what it cannot promise is which entry each gets. On aCloseAsync(allowCommandsToComplete: false)Close took entries 0..N-1 (tight loop, nothing else in it) and the reader then took entry N for reply 0:GETkey N completed, successfully, with key 0's value, and everything behind it was shifted the same way. The reporter's loop against 4.0.155-alpha: 4,853 wrong replies in 20,000 calls, shifts of 69, 152, 230 positions;mainhas the same race inPhysicalConnectionwith a much smaller window (616 in 20,000).Fix (RESPite,
RespConnection).TryTakePendingchecks closed and dequeues as one step under a new_pendingLock;Closesets_closedand then empties the queue under the same lock, failing what it took outside it (completions run continuations). Once a close has begun the reader takes nothing: a late reply dies with the connection, and the command it answered is failed by Close exactly as before. An uncontendedlockper in-band frame, and nothing else on the hot path. Same run afterwards: 0.Second commit: what those commands fail with.
RespClientConnectionalready overridesCreateRemoteClosedFaultso a server-side close fails commands withRedisConnectionException(SocketClosed), as v3 did. Our own close fell through to RESPite's private fallback, a bareInvalidOperationException("The connection is closed.")- which the ported test rejected, and which a caller catchingRedisConnectionExceptionaround a shutdown would miss.CreateClosedFaultis the local-close counterpart; the client overrides it withConnectionDisposedand a message naming the endpoint.RespConnectionis internal, so no public-API entry.Test.
CloseInFlightTestsfrom #3330, unchanged: 20 rounds × 500 in-flightGETs against the real server, closed without waiting; the only acceptable outcomes are the right value or aRedisConnectionException.Verified locally: library and RESPite build clean on all TFMs; RESPite.Tests 1,977 passed; full net10.0 suite 7,989 passed / 0 failed / 167 skipped.