Repository navigation
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
|
Hi team, any update on this PR? |
Hi @mehdiyari, no updates, this is still in review. |
| try { | ||
| call.sendMessage(message); | ||
| } catch (IllegalStateException e) { | ||
| if (e.getMessage() != null && e.getMessage().contains("call was cancelled")) { |
There was a problem hiding this comment.
It's preferred not to branch on the exception message, because the message isn't part of gRPC's API contract.
A better approach would be to track the cancelled state ourselves. This IllegalStateException is only thrown after ClientCall.cancel() has been called. Network errors and deadlines report through onClose() instead. In our case the cancel most likely comes from our own channel.shutdownNow() in GrpcCallProvider.resetChannel(). So if we mark the streams or channel as cancelled before calling shutdownNow(), we can skip sendMessage() and halfClose() without catching anything. Relying on onClose() alone wouldn't be enough, because it's delivered asynchronously after the cancel.
If we don't want to do that refactor now: there's no error code or exception subtype to check instead. To make the message check safer, the tests should run against real gRPC instead of mocking new IllegalStateException("call was cancelled"). As written, if gRPC rewords the message, the tests still pass and the crashes come back. A test using a real in-process channel (start, cancel(), then sendMessage()) would catch a reworded message.
| // Simulate gRPC ClientCallImpl.sendMessageInternal() throwing on cancelled call | ||
| Mockito.doThrow(new IllegalStateException("call was cancelled")) | ||
| .when(mockCall) | ||
| .sendMessage(Mockito.any()); |
There was a problem hiding this comment.
To make the message check safer, the tests should run against real gRPC instead of mocking new IllegalStateException("call was cancelled"). As written, if gRPC rewords the message, the tests still pass and the crashes come back. A test using a real in-process channel (start, cancel(), then sendMessage()) would catch a reworded message.
| Runnable delayedDrainRunnable = | ||
| () -> { | ||
| try { | ||
| throw new IllegalStateException("call was cancelled"); |
There was a problem hiding this comment.
To make the message check safer, the tests should run against real gRPC instead of mocking new IllegalStateException("call was cancelled"). As written, if gRPC rewords the message, the tests still pass and the crashes come back. A test using a real in-process channel (start, cancel(), then sendMessage()) would catch a reworded message.
gRPC calls (
ClientCall) can be cancelled due to network changes, timeouts, or channel resets. After a call is cancelled, all subsequent operations throwIllegalStateException: call was cancelledexception. In the SDK, there are paths where operations run on a already cancelled call. When these exceptions are unhandled, they go up toAsyncQueueand triggerAsyncQueue.panic(), crashing the application.This was first reported in #6883, where
call.halfCloseoperation is ran on a call that was already cancelled, causing crashes. This issue was closed by #6894, which wrapped the operation in a try/catch, and logged the error instead. This did not fully fix the issue, since 1) there are other other operations that can run on a cancelled call, which were not wrapped in try/catch (e.g.writeRequest->call.sendMessage()), 2) operations can run in theAsyncQueueexecutor, and cannot be easily wrapped in a try/catch. These cases were reported recently in #8601.This change addresses the two additional cases; the
call.sendMessage()call is guarded, and aguardedGrpcExecutorwas created to catch thecall was cancelledexceptions from gRPC channels.Notes:
guardedGrpcExecutoronly catchesIllegalStateException: call was cancelledwith message containingcall was cancelled. Any other exceptions here that are causing crashes, will still cause crashes.