Skip to content

core: Count client-cancelled server calls as failed in Channelz - #13128

Open
JoeCqupt wants to merge 1 commit into
grpc:masterfrom
JoeCqupt:fix/fix-13063
Open

JoeCqupt wants to merge 1 commit into
grpc:masterfrom
JoeCqupt:fix/fix-13063

Conversation

@JoeCqupt

Copy link
Copy Markdown
Contributor

Fixes #13063

Problem

ServerCallImpl only reports the end of a call to CallTracer when the
application closes it (closeInternal) or when an internal error occurs
(handleInternalError). When the client cancels, the transport calls
ServerStreamListenerImpl.closed() with a non-OK status. That path calls
onCancel() and cancels the context, but never reports the end of the call.
The application normally doesn't call close() after it sees onCancel(),
so calls_started goes up while calls_failed does not.

Fix

  • Report the call as failed in the non-OK branch of
    ServerStreamListenerImpl.closedInternal().
  • Send every end-of-call report (from closeInternal, handleInternalError
    and the cancellation path) through a single reportCallEnded() guarded by
    an AtomicBoolean, so each call is counted exactly once. The guard is
    needed because:
    • the application may still call close() after the client cancelled;
    • handleInternalError reports the failure and then calls
      stream.cancel(), which leads to closed() with a non-OK status;
    • the application's close() and the transport's closed() can run on
      different threads.

The OK branch of closed() is left unchanged. A closed(OK) only happens
after 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
closeInternal with the real status.

Behavior changes

  • If the client cancels and the application later calls close(OK), the
    call used to be counted as succeeded. It is now counted as failed,
    because the cancellation is seen first.
  • Side effect of counting each call only once: a unary call closed with
    close(OK) without sending a response used to be counted as both
    failed (from handleInternalError) and succeeded (from the finally in
    closeInternal). It is now counted only as failed. This was not a
    goal of this change; it follows from the count-once guard.

Testing

New tests in ServerCallImplTest:

  • callTracer_clientCancelled_reportsCallFailed: cancellation alone counts
    one failure. This test fails without the fix.
  • callTracer_clientCancelledThenClosed_reportsCallFailedOnce: cancel then
    close(OK) gives failed 1, succeeded 0.
  • callTracer_closedThenClientCancelled_reportsCallSucceededOnce:
    close(OK) then cancel gives succeeded 1, failed 0.

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
@JoeCqupt

Copy link
Copy Markdown
Contributor Author

@kannanjgithub PTAL

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

channelz: ServerData calls_failed counter not incremented upon client cancellation

1 participant