Repository navigation
Conversation
ServerCallImpl only reported a call's end when the application closed it or an internal error occurred. When the client cancelled, the transport notified ServerStreamListenerImpl.closed() with a non-OK status, which never reported the call as ended, so calls_failed was not incremented. Report the call as failed on cancellation, and guard all end reports so each call is counted exactly once, whichever of the application's close() or the transport's closed() happens first. Fixes grpc#13063
Contributor
Author
|
@kannanjgithub PTAL |
This branch has not been deployed
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 #13063
Problem
ServerCallImplonly reports the end of a call toCallTracerwhen theapplication closes it (
closeInternal) or when an internal error occurs(
handleInternalError). When the client cancels, the transport callsServerStreamListenerImpl.closed()with a non-OK status. That path callsonCancel()and cancels the context, but never reports the end of the call.The application normally doesn't call
close()after it seesonCancel(),so
calls_startedgoes up whilecalls_faileddoes not.Fix
ServerStreamListenerImpl.closedInternal().closeInternal,handleInternalErrorand the cancellation path) through a single
reportCallEnded()guarded byan
AtomicBoolean, so each call is counted exactly once. The guard isneeded because:
close()after the client cancelled;handleInternalErrorreports the failure and then callsstream.cancel(), which leads toclosed()with a non-OK status;close()and the transport'sclosed()can run ondifferent threads.
The OK branch of
closed()is left unchanged. Aclosed(OK)only happensafter the server has already closed the stream, and its status is not the
RPC's actual status, so the end of the call has already been counted in
closeInternalwith the real status.Behavior changes
close(OK), thecall used to be counted as succeeded. It is now counted as failed,
because the cancellation is seen first.
close(OK)without sending a response used to be counted as bothfailed (from
handleInternalError) and succeeded (from thefinallyincloseInternal). It is now counted only as failed. This was not agoal of this change; it follows from the count-once guard.
Testing
New tests in
ServerCallImplTest:callTracer_clientCancelled_reportsCallFailed: cancellation alone countsone failure. This test fails without the fix.
callTracer_clientCancelledThenClosed_reportsCallFailedOnce: cancel thenclose(OK)gives failed 1, succeeded 0.callTracer_closedThenClientCancelled_reportsCallSucceededOnce:close(OK)then cancel gives succeeded 1, failed 0.