Armeria: Have armeria always add a fallback Service for not found requests?

[anuraaga]

I found in https://github.com/openzipkin/zipkin/pull/2348 that there is somewhat confusing behavior regarding ServerBuilder.decorator - as it only decorates mapped services, requests that aren't mapped and returned as NOT_FOUND are not handled by the decorator. If the decorator is for metrics collection, and has logic for not found requests, it doesn't work.

Does it make sense to have armeria always add a service at the end for not_found requests to make sure behavior is consistent with mapped requests?

[anuraaga]

It might cause some spam for LoggingDecorator users, but on the flip side I think it is actually expected for them to be logged just like any other error status.

[anuraaga]

For reference, this workaround works ok to define a fallback in user code using spring

https://github.com/openzipkin/zipkin/pull/2348/commits/8a12efbd378055b46ab6a22d22c2c5b2507bccb1

[trustin]

Thanks for reporting. It is currently hard-coded in HttpServerHandler, but I think we should provide some extension point for the error responses such as:

• The fallback responses by HttpServerHandler.
• The responses triggered by HttpStatusException.

Other error responses represented as HttpResponse and HttpResponseException could remain untouched, I guess.

[trustin]

Also, we could add the stack trace of the cause of the failure if HttpStatusException has a cause and verboseResponses flag is on. Therefore, the forementioned extension point has to be given with the cause.

[trustin]

Here's the proposal:

When a service throws an HttpStatusException, HttpServerHandler queries the router again with a (GET|HEAD) /:render_status request. Armeria's router will handle this by returning a special HttpService which calls a new interface HttpStatusRenderer:

interface HttpStatusRenderer {
    HttpResponse render(ServiceRequestContext ctx, HttpStatusException cause) throws Exception;
}

404 errors are translated into HttpStatusException.of(404) and thus will be handled in the same way.

The forementioned special HttpService is bound at exact:/:render_status internally when a Server is built. Therefore, it will be decorated by a route decorator bound at catchAll or prefix:/. As a result, the known issue with CorsDecorator is solved.

Note that it is not possible for a remote peer to send a request to /:render_status, because Armeria reject a path whose first component contains a colon.

A user can specify an HttpStatusRender via a builder method in ServerBuilder or VirtualHostBuilder:

Server.builder()
      .statusRenderer((ctx, cause) -> {
          return HttpResponse.of(cause.httpStatus());
      });

Thoughts? @minwoox @ikhoon @anuraaga

[trustin]

The proposal above has an issue where a request log does not contain the information about the original request. Let me think a little bit more about this.

[trustin]

Perhaps we could use /:render_status only for querying the Router, but not for creating a context.

[anuraaga]

Is the proposal to allow users to remap exceptions other than 404? For 404 I think it works to call the fallback service without reissuing the request. I guess supporting rerendering of non-404 could be a different layer?

[trustin]

The proposal intends to provide a universal way to handle both 404 and HttpStatusException, but I found that 404 and HttpStatusException are different; If HttpStatusException is raised, it means the service has been invoked already, so issuing another request will create an extra request log.

So.. let me drop the proposal and just make the following changes:

• Modify ServerBuilder and VirtualHostBuilder so that a 404-responding service is bound automatically at prefix:/ as the last service, if not duplicate.

[trustin]

WRT custom status rendering, here, we could translate an HttpStatusException into an HttpResponse using a Function<HttpStatusException, HttpResponse>.

The 404-responding service could raise HttpStatusException.of(404) to take advantage of this feature.