fix: cancel stream attempts abandoned by the establishment timeout - #388
Merged
Merged
Conversation
Since oxia-db#370, read, list, rangeScan and getShardAssignments bound the establishment of each attempt with requestTimeout. When it fires, TIMEOUT is reported to the observer, but the gRPC call is not cancelled: its later events are dropped, yet it stays open on the server. getShardAssignments is bounded by the subscription max age, while read, list and rangeScan have no deadline. So a server that hangs but still passes the connection health checks accumulates one open call per timed-out attempt, until it answers. Run each attempt in its own cancellable gRPC context, and cancel it once the error has been reported. The error is reported first, so that the observer gets TIMEOUT rather than the CANCELLED status of the cancelled call. The context is cancelled without a cause, since a TimeoutException cause would close the call with DEADLINE_EXCEEDED. For list and rangeScan, CancelableStreamObserver.cancel() can't be used, as it does nothing once the observer is terminated. The *TimesOutSilentAttempt tests now check that the server sees the cancellation. Their silent handlers no longer block, since a blocked handler also holds back the server's onCancel callback. Signed-off-by: Matteo Merli <mmerli@apache.org>
…bd2ec2 Signed-off-by: Matteo Merli <mmerli@apache.org>
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.
Motivation
Since #370,
read,list,rangeScanandgetShardAssignmentsbound the establishment of each attempt withrequestTimeout. When it fires,TIMEOUTis reported to the observer, but the gRPC call is not cancelled: its later events are dropped byGuardedStreamObserveror by the terminatedCancelableStreamObserver, yet it stays open on the server.getShardAssignmentsis bounded by the subscription max age, whileread,listandrangeScanhave no deadline. So a server that hangs but still passes the connection health checks accumulates one open call per timed-out attempt, until it answers.Modifications
Apply to these four RPCs the approach that #386 uses for
getNotifications: run each attempt in its ownContext.current().withCancellation(), and cancel it in.exceptionally, once the error has been reported to the observer.TIMEOUTrather than theCANCELLEDstatus of the cancelled call.nullcause: with aTimeoutExceptioncause, gRPC would close the call withDEADLINE_EXCEEDED.listandrangeScan,CancelableStreamObserver.cancel()can't be used, as it does nothing once the observer is terminated.Only the attempt that timed out can be left open:
TIMEOUTis not retryable, so it is always the last attempt, and earlier attempts end when their call is closed or fails to start. The contexts of successful attempts are not cancelled, which doesn't leak: a call registers its cancellation listener on the context when it starts, and removes it when it closes.Verification
*TimesOutSilentAttempttests of these four RPCs now check that the server sees the cancellation. Their silent handlers no longer block on a latch, which also held back the server'sonCancelcallback, and registerServerCallStreamObserver.setOnCancelHandlerinstead. Without the fix, all four fail on the new check; with it, they passed 10 repeated runs.GrpcRpcProviderTestand the rest of the unit tests pass locally. The Docker-based ITs are left to CI.