Change the insertion order of `ExceptionHandlerFunction`

[minwoox]

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.

[ks-yim]

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?

[minwoox]

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? 🤔

[minwoox]

Let me change the label to discussion because others might have different points of view. 🤔 cc @trustin, @ikhoon and @jrhee17

[ikhoon]

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?

[minwoox]

should we move the Thrift error handler into ThriftCallService?

That's a good point. +1 for moving it.

[ks-yim]

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.

[minwoox]

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.

[ks-yim]

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

[minwoox]

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. 😆