Skip to content

Cancel DelayedClientCall when application listener throws#12761

Merged
kannanjgithub merged 3 commits intogrpc:masterfrom
jnowjack-lucidchart:jnowjack-exception-handling-in-delayed-client-call
Apr 22, 2026
Merged

Cancel DelayedClientCall when application listener throws#12761
kannanjgithub merged 3 commits intogrpc:masterfrom
jnowjack-lucidchart:jnowjack-exception-handling-in-delayed-client-call

Conversation

@jnowjack-lucidchart
Copy link
Copy Markdown
Contributor

Align DelayedClientCall.DelayedListener with ClientCallImpl's existing behavior for listener exceptions. When the application listener throws from onHeaders/onMessage/onReady, catch the Throwable, cancel the call with CANCELLED (cause = the throwable), and swallow subsequent callbacks. When onClose throws, log and continue, matching ClientCallImpl.closeObserver. If onClose arrives from the transport after a prior callback threw, override its status/trailers with the captured CANCELLED so a server-supplied OK can't mask the local failure.

Previously, a throw from the application listener escaped to the callExecutor's uncaught-exception handler. The real call was not cancelled and the transport kept delivering callbacks to an already broken listener, different from how the same bug behaves on a normal ClientCallImpl, and a timing-dependent inconsistency depending on whether callbacks arrived before or after setCall + drain completed.

Trade-off: listener-callback throws are no longer visible to the executor's UncaughtExceptionHandler (they're attached as Status.cause instead). This matches ClientCallImpl and is the intended behavior.

Exception handling for the outer drainPendingCalls loop (realCall.sendMessage/request/halfClose/cancel) remains unaddressed; that TODO is preserved.

Note:
This change only handles exceptions thrown by the application listener. I don't try and solve the problems that #12737 is attempting to fix. My motivation is to fix the root cause behind bazelbuild/bazel#29316

Align DelayedClientCall.DelayedListener with ClientCallImpl's existing
behavior for listener exceptions. When the application listener throws
from onHeaders/onMessage/onReady, catch the Throwable, cancel the call
with CANCELLED (cause = the throwable), and swallow subsequent
callbacks. When onClose throws, log and continue, matching
ClientCallImpl.closeObserver. If onClose arrives from the transport
after a prior callback threw, override its status/trailers with the
captured CANCELLED so a server-supplied OK can't mask the local
failure.

Previously, a throw from the application listener escaped to the
callExecutor's uncaught-exception handler. The real call was not
cancelled and the transport kept delivering callbacks to an already
broken listener, different from how the same bug behaves on a normal
ClientCallImpl, and a timing-dependent inconsistency depending on
whether callbacks arrived before or after setCall + drain completed.

Trade-off: listener-callback throws are no longer visible to the
executor's UncaughtExceptionHandler (they're attached as Status.cause
instead). This matches ClientCallImpl and is the intended behavior.

Exception handling for the outer drainPendingCalls loop
(realCall.sendMessage/request/halfClose/cancel) remains unaddressed;
that TODO is preserved.
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Apr 16, 2026

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jnowjack-lucidchart / name: Jacob Nowjack (8e59f3e)
  • ✅ login: kannanjgithub / name: Kannan J (736f00a, bcaf12c)

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aligns DelayedClientCall.DelayedListener behavior with ClientCallImpl when the application ClientCall.Listener throws from callbacks, ensuring the call is cancelled and subsequent callbacks are suppressed/normalized instead of escaping to the executor’s uncaught exception handler.

Changes:

  • Add exception-handling wrappers for onHeaders, onMessage, and onReady that cancel the call with CANCELLED (cause = thrown exception) and swallow later callbacks.
  • Override transport-delivered onClose(Status.OK, …) after a prior listener exception to prevent server OK from masking a local failure; keep onClose exception behavior as “log and continue.”
  • Add unit tests covering pending vs pass-through callback paths and the onClose override behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
core/src/main/java/io/grpc/internal/DelayedClientCall.java Wraps listener callback dispatch to catch listener-thrown exceptions, cancel the real call, and normalize subsequent callbacks/onClose to match ClientCallImpl semantics.
core/src/test/java/io/grpc/internal/DelayedClientCallTest.java Adds targeted tests for listener-throwing scenarios (pending + pass-through) and verifies cancellation + callback swallowing/onClose override.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread core/src/main/java/io/grpc/internal/DelayedClientCall.java Outdated
Comment on lines +543 to +547
// Ideally exceptionStatus == status, as exceptionStatus was passed to cancel().
// However the cancel is racy and this onClose may have already been queued when the
// cancellation occurred. Since other callbacks throw away data if exceptionStatus !=
// null, it is semantically essential that we _not_ use a status provided by the
// server.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@kannanjgithub
Copy link
Copy Markdown
Contributor

/gcbrun

@kannanjgithub kannanjgithub added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 22, 2026
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 22, 2026
@kannanjgithub kannanjgithub merged commit 7561d0b into grpc:master Apr 22, 2026
22 of 23 checks passed
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.

4 participants