armeria
armeria copied to clipboard
Change the insertion order of `ExceptionHandlerFunction`
Let's look at the following example:
1: ServerBuilder sb = ...
2: sb.annotatedService()
3: .decorator(delegate -> (ctx, req) -> {
4: return delegate.serve(ctx, req).mapHeaders(headers -> {
5: System.err.println("This isn't called.");
6: return headers;
7: });
8: })
9: .exceptionHandlers((ctx, req, cause) -> HttpResponse.of(200))
10: .build(new Object() {
11: @Get("/foo")
12: public HttpResponse foo(String foo) {
13: throw new IllegalArgumentException();
14: }
15: });
The fifth line is not called because the specified exception handler is attached before the decorator so mapHeaders
is not applied:
A request flow:
request -> ExceptionHandlerFunction -> decorator -> Request(Response)Converter -> Annotated service
The reason why the ExceptionHandlerFunction
is located before the decorator is that we wanted to catch the exception raised in the decorator.
However, it has this bypassing problem, and also ExceptionHandlerFunction
is not installed with the Request(Response)Converter
at the same level which users expect so.
I thought this was an intended behavior.
Just to clarify the issue description, ExceptionHandlerFunction
was supposed to always be applied at the same level with {Request, Response}Converter
?
I thought this was an intended behavior.
Yeah, that was the intention but as described it has problems.
ExceptionHandlerFunction was supposed to always be applied at the same level with {Request, Response}Converter?
I believe that users expect them to be installed at the same level. Not before after the decorators. Do you have any concerns about this? 🤔
Let me change the label to discussion because others might have different points of view. 🤔 cc @trustin, @ikhoon and @jrhee17
Another considering point could be the consistency of behavior of error handlers.
- Thrift: start <-> routing decorators <-> Thrift error handler <-> service (rpc)decorators <-> service
- gRPC: start <-> routing decorators <-> service decorators <-> gRPC error handlers <-> service
- Annotated(current): start <-> routing decorators <-> Annotated error handler <-> service decorators <-> service
- Server: start <-> error handler <-> routing decorators <-> service decorators <-> service
Technically it would be difficult to move the location of ServerErrorHandler
and GrpcStatusFunction
.
If we decided to place annotated error handler right next to a service, should we move the Thrift error handler into ThriftCallService
?
should we move the Thrift error handler into ThriftCallService?
That's a good point. +1 for moving it.
Do you have any concerns about this? 🤔
We are currently using ExceptionHandlerFunction
to catch exceptions raised in some service decorators (e.g. AuthService
). Does this change imply that those exceptions raised in service decorators should be handled in ServerErrorHandler
?
Sounds like so but the following sentence confuses me. 😅 Maybe I am missing something?
I guess we wanted to catch the exception raised in the decorator.
Sorry about it. 😅 I updated the sentence.
The reason why the
ExceptionHandlerFunction
is located before the decorator is that we wanted to catch the exception raised in the decorator.
Agree with the direction. I was quite confused with the differentiation between ServerErrorHandler
and ExceptionHandlerFunction
in the past and the change will make their roles more clear.
I was just worried that this can be a breaking change. cc. @ghkim3221
Thanks for your opinion. 😄
I was just worried that this can be a breaking change.
Yeah, it could be a breaking change so we might be able to change that in 2.0 Or, we can fix it right now if everyone thinks that it's a bug. 😆