Feature/async grpc exception handler#6717
Conversation
Hun425
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces asynchronous exception handling to gRPC services. It adds async-first APIs ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant ServerCall
participant ExceptionHandler as Exception Handler
participant DelegateHandler as Delegate Async Handler
participant GrpcDefault as Default Handler
Client->>ServerCall: Call triggers error
ServerCall->>ExceptionHandler: handleAsync(exception)
ExceptionHandler->>ExceptionHandler: peelAndUnwrap(throwable)
ExceptionHandler->>ExceptionHandler: restoreStatus(status)
ExceptionHandler->>DelegateHandler: applyAsync(status, throwable, metadata)
alt Delegate completes with Status
DelegateHandler-->>ExceptionHandler: Status
ExceptionHandler-->>ServerCall: StatusAndMetadata
else Delegate returns null or fails
ExceptionHandler->>GrpcDefault: apply()
GrpcDefault-->>ExceptionHandler: Default Status
ExceptionHandler-->>ServerCall: StatusAndMetadata (fallback)
end
ServerCall->>Client: Close with final Status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java (1)
962-970:⚠️ Potential issue | 🟡 MinorMissing mutual exclusivity check for
asyncExceptionHandler.The deprecated
addExceptionMapping()methods check mutual exclusivity withexceptionHandlerbut not withasyncExceptionHandler. For consistency with the newasyncExceptionHandler()method which checks againstexceptionMappingsBuilder, this should also check the reverse.🛡️ Proposed fix
`@Deprecated` public GrpcServiceBuilder addExceptionMapping(Class<? extends Throwable> exceptionType, Status status) { requireNonNull(exceptionType, "exceptionType"); requireNonNull(status, "status"); checkState(exceptionHandler == null, "addExceptionMapping() and exceptionHandler() are mutually exclusive."); + checkState(asyncExceptionHandler == null, + "addExceptionMapping() and asyncExceptionHandler() are mutually exclusive."); exceptionMappingsBuilder().on(exceptionType, status); return this; }Apply similar changes to the other
addExceptionMappingoverloads at lines 983-993 and 1006-1018.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java` around lines 962 - 970, The addExceptionMapping(...) overloads (the methods named addExceptionMapping that currently check checkState(exceptionHandler == null, ...)) must also check that asyncExceptionHandler is null to enforce mutual exclusivity with asyncExceptionHandler(); update each overload (the ones calling exceptionMappingsBuilder().on(...)) to call checkState(asyncExceptionHandler == null, "addExceptionMapping() and asyncExceptionHandler() are mutually exclusive.") in addition to the existing exceptionHandler check so both deprecated synchronous mappings and the new asyncExceptionHandler cannot be set together; apply the same check to all addExceptionMapping overloads referenced in the diff.
🧹 Nitpick comments (1)
grpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java (1)
194-204: Consider a more specific assertion for the failing handler test.The current assertion
assertThat(e.getStatus().getCode()).isNotNull()is quite weak sinceStatus.Codeis an enum that can never be null. Based on the implementation inAbstractServerCall, when the async handler fails exceptionally, the fallback usesStatus.INTERNAL(for theclose(Throwable, boolean)path) or the original status (for theclose(Status, Metadata)path).Consider asserting the specific expected status code to make the test more meaningful:
💡 Proposed improvement
`@Test` void failingAsyncHandlerFallsBackToOriginalStatus() { final TestServiceBlockingStub client = GrpcClients.newClient(serverWithFailingHandler.httpUri(), TestServiceBlockingStub.class); assertThatThrownBy(() -> client.unaryCall(SimpleRequest.getDefaultInstance())) .isInstanceOfSatisfying(StatusRuntimeException.class, e -> { - // When the async handler fails, the original status should be preserved. - assertThat(e.getStatus().getCode()).isNotNull(); + // When the async handler fails, falls back to INTERNAL status. + assertThat(e.getStatus().getCode()).isEqualTo(Status.Code.INTERNAL); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java` around lines 194 - 204, The test failingAsyncHandlerFallsBackToOriginalStatus currently asserts a non-null Status.Code which is meaningless; update the assertion in AsyncGrpcExceptionHandlerTest.failingAsyncHandlerFallsBackToOriginalStatus to check for the precise expected Status.Code (e.g. assertThat(e.getStatus().getCode()).isEqualTo(Status.INTERNAL) or the explicit original status used by serverWithFailingHandler) by inspecting how serverWithFailingHandler triggers the async failure (whether it falls back to Status.INTERNAL via close(Throwable, boolean) or preserves the original Status via close(Status, Metadata)) and assert that exact code on the caught StatusRuntimeException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java`:
- Around line 962-970: The addExceptionMapping(...) overloads (the methods named
addExceptionMapping that currently check checkState(exceptionHandler == null,
...)) must also check that asyncExceptionHandler is null to enforce mutual
exclusivity with asyncExceptionHandler(); update each overload (the ones calling
exceptionMappingsBuilder().on(...)) to call checkState(asyncExceptionHandler ==
null, "addExceptionMapping() and asyncExceptionHandler() are mutually
exclusive.") in addition to the existing exceptionHandler check so both
deprecated synchronous mappings and the new asyncExceptionHandler cannot be set
together; apply the same check to all addExceptionMapping overloads referenced
in the diff.
---
Nitpick comments:
In
`@grpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java`:
- Around line 194-204: The test failingAsyncHandlerFallsBackToOriginalStatus
currently asserts a non-null Status.Code which is meaningless; update the
assertion in
AsyncGrpcExceptionHandlerTest.failingAsyncHandlerFallsBackToOriginalStatus to
check for the precise expected Status.Code (e.g.
assertThat(e.getStatus().getCode()).isEqualTo(Status.INTERNAL) or the explicit
original status used by serverWithFailingHandler) by inspecting how
serverWithFailingHandler triggers the async failure (whether it falls back to
Status.INTERNAL via close(Throwable, boolean) or preserves the original Status
via close(Status, Metadata)) and assert that exact code on the caught
StatusRuntimeException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb56fc9b-cc27-4859-af9c-d742fa28c8fa
📒 Files selected for processing (7)
grpc/src/main/java/com/linecorp/armeria/common/grpc/AsyncGrpcExceptionHandlerFunction.javagrpc/src/main/java/com/linecorp/armeria/internal/common/grpc/InternalGrpcExceptionHandler.javagrpc/src/main/java/com/linecorp/armeria/internal/server/grpc/AbstractServerCall.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HandlerRegistry.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java (1)
984-995:⚠️ Potential issue | 🟡 MinorMissing mutual exclusivity check for
asyncExceptionHandler.The second
addExceptionMappingoverload (acceptingBiFunction<T, Metadata, Status>) does not enforce mutual exclusivity withasyncExceptionHandler(), unlike the other two overloads at lines 965-968 and 1013-1016. This inconsistency could allow users to accidentally combine both configuration methods.🔧 Proposed fix
`@Deprecated` public <T extends Throwable> GrpcServiceBuilder addExceptionMapping( Class<T> exceptionType, BiFunction<T, Metadata, Status> statusFunction) { requireNonNull(exceptionType, "exceptionType"); requireNonNull(statusFunction, "statusFunction"); checkState(exceptionHandler == null, "addExceptionMapping() and exceptionMapping() are mutually exclusive."); + checkState(asyncExceptionHandler == null, + "addExceptionMapping() and asyncExceptionHandler() are mutually exclusive."); exceptionMappingsBuilder().on(exceptionType, statusFunction); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java` around lines 984 - 995, The addExceptionMapping(Class<T>, BiFunction<T, Metadata, Status>) overload lacks the mutual-exclusivity check against asyncExceptionHandler()—add the same check used in the other overloads to prevent combining exceptionMapping/addExceptionMapping with asyncExceptionHandler; specifically, in GrpcServiceBuilder.addExceptionMapping(...) verify asyncExceptionHandler == null (e.g., call checkState(asyncExceptionHandler == null, "addExceptionMapping() and asyncExceptionHandler() are mutually exclusive.")) before calling exceptionMappingsBuilder().on(...), matching the pattern used with exceptionHandler.
🧹 Nitpick comments (1)
grpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java (1)
49-49: Consider shutting down the executor to avoid thread leaks.The
ASYNC_EXECUTORis a single-threaded executor that is never shut down. While this might not cause issues in practice for tests, it's good hygiene to clean up executor resources. You could use@AfterAllto shut it down or use a daemon thread factory.♻️ Option 1: Use a daemon thread factory
- private static final Executor ASYNC_EXECUTOR = Executors.newSingleThreadExecutor(); + private static final Executor ASYNC_EXECUTOR = Executors.newSingleThreadExecutor(r -> { + final Thread t = new Thread(r, "async-handler-test"); + t.setDaemon(true); + return t; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java` at line 49, The ASYNC_EXECUTOR created by Executors.newSingleThreadExecutor() in AsyncGrpcExceptionHandlerTest is never shut down; update the test to prevent thread leaks by either replacing Executors.newSingleThreadExecutor() with Executors.newSingleThreadExecutor(new ThreadFactoryBuilder().setDaemon(true).build()) or add an `@AfterAll` static method that calls ASYNC_EXECUTOR.shutdownNow()/shutdown() and awaits termination; reference the ASYNC_EXECUTOR constant and the test class AsyncGrpcExceptionHandlerTest when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java`:
- Around line 984-995: The addExceptionMapping(Class<T>, BiFunction<T, Metadata,
Status>) overload lacks the mutual-exclusivity check against
asyncExceptionHandler()—add the same check used in the other overloads to
prevent combining exceptionMapping/addExceptionMapping with
asyncExceptionHandler; specifically, in
GrpcServiceBuilder.addExceptionMapping(...) verify asyncExceptionHandler == null
(e.g., call checkState(asyncExceptionHandler == null, "addExceptionMapping() and
asyncExceptionHandler() are mutually exclusive.")) before calling
exceptionMappingsBuilder().on(...), matching the pattern used with
exceptionHandler.
---
Nitpick comments:
In
`@grpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java`:
- Line 49: The ASYNC_EXECUTOR created by Executors.newSingleThreadExecutor() in
AsyncGrpcExceptionHandlerTest is never shut down; update the test to prevent
thread leaks by either replacing Executors.newSingleThreadExecutor() with
Executors.newSingleThreadExecutor(new
ThreadFactoryBuilder().setDaemon(true).build()) or add an `@AfterAll` static
method that calls ASYNC_EXECUTOR.shutdownNow()/shutdown() and awaits
termination; reference the ASYNC_EXECUTOR constant and the test class
AsyncGrpcExceptionHandlerTest when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d233532c-eed4-4081-b4de-87859763bd14
📒 Files selected for processing (5)
grpc/src/main/java/com/linecorp/armeria/common/grpc/AsyncGrpcExceptionHandlerFunction.javagrpc/src/main/java/com/linecorp/armeria/internal/server/grpc/AbstractServerCall.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/AsyncGrpcExceptionHandlerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java
- grpc/src/main/java/com/linecorp/armeria/common/grpc/AsyncGrpcExceptionHandlerFunction.java
Code reviewFound 1 issue:
armeria/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java Lines 1142 to 1147 in a205622 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Just to make sure I understand the intention of this PR first, is your use-case to make async calls in the exception handler to an external component? I was imagining a normal flow could be: I have no issue going ahead with this PR if it is a valid concern |
|
@jrhee17 Thanks for the review! The preload pattern works for static data, but not when the A local cache with invalidation in front of the remote store |
|
I see, I'm not fixed on this but here are my current thoughts:
AsyncGrpcExceptionHandlerFunction async = (ctx, status, cause, metadata) -> UnmodifiableFuture.completedFuture(syncHandler.apply(ctx, status, cause, metadata));The single async version will be used throughout the internal call path
Let me know what you think cc. @line/dx |
|
If we introduce @Deprecated
Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata);
default CompletableFuture<@Nullable Status> applyAsync(RequestContext ctx, Status status, Throwable cause, Metadata metadata) {
return CompletableFuture.completedFuture(apply(ctx, status, cause, metadata));
}
default GrpcExceptionHandlerFunction orElse(GrpcExceptionHandlerFunction next) {
requireNonNull(next, "next");
if (this == next) {
return this;
}
return new GrpcExceptionHandlerFunction() {
@Override
public @Nullable Status apply(RequestContext ctx, Status status, Throwable cause,
Metadata metadata) {
return GrpcExceptionHandlerFunction.this.apply(ctx, status, cause, metadata);
}
@Override
public CompletableFuture<@Nullable Status> applyAsync(RequestContext ctx, Status status,
Throwable cause,
Metadata metadata) {
return GrpcExceptionHandlerFunction.this.applyAsync(ctx, status, cause, metadata).thenCompose(
newStatus -> {
if (newStatus != null) {
return CompletableFuture.completedFuture(newStatus);
}
return next.applyAsync(ctx, status, cause, metadata);
});
}
};
} |
…er' into feature/async-grpc-exception-handler
|
Pushed the refactor following minwoox's 1.
|
Yeah, we need to keep the existing behavior. Please go ahead. 🙇 |
| * }</pre> | ||
| */ | ||
| @UnstableApi | ||
| static GrpcExceptionHandlerFunction ofAsync(AsyncHandler asyncHandler) { |
There was a problem hiding this comment.
Note) Heads up - to use a functional approach, we'll still need an interface for async error handling cc. @minwoox
There was a problem hiding this comment.
to use a functional approach,
Yeah, we can't use the functional approach without adding the new interface. Do you think it’s worth adding the new interface?
There was a problem hiding this comment.
I think it's worth adding. Added it as a subinterface of GrpcExceptionHandlerFunction,
plus asyncExceptionHandler(...) on both server and client builders.
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
I think the current changes look good overall - left some small suggestions
|
@minwoox This PR is ready to review. PTAL. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6717 +/- ##
============================================
+ Coverage 74.46% 75.00% +0.54%
- Complexity 22234 24757 +2523
============================================
Files 1963 2203 +240
Lines 82437 91938 +9501
Branches 10764 12002 +1238
============================================
+ Hits 61385 68957 +7572
- Misses 15918 17253 +1335
- Partials 5134 5728 +594 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Motivation:
GrpcExceptionHandlerFunction.apply()is synchronous and invoked insideAbstractServerCall.close()on the event loop. When exception handling needs async work (e.g., i18n lookup from a remote store), users are forced to userunBlockingorCompletableFuture.join(), risking event loop starvation.Modifications:
AsyncGrpcExceptionHandlerFunctionreturningCompletableFuture<@Nullable Status>, withorElse()chaining.InternalGrpcExceptionHandlerto support async delegation with sync fallback when the async handler returnsnull.AbstractServerCall.close()andFramedGrpcServicetimeout/cancellation paths. Finalclose()is scheduled onctx.eventLoop().GrpcServiceBuilder.asyncExceptionHandler(). Can be combined withexceptionHandler()— async is tried first, sync is the fallback.nullfallback,orElsechaining, and handler failure.Result:
@UnstableApi.