java-spring-web icon indicating copy to clipboard operation
java-spring-web copied to clipboard

Errors happening in the controller in Spring boot should not create follows-from span

Open mabn opened this issue 8 years ago • 0 comments

This is related to this comment: https://github.com/opentracing-contrib/java-spring-web/pull/1#issuecomment-280684207 and this spring issue: https://jira.spring.io/browse/SPR-15265

Basically now in Spring Boot with embedded container the request which throws exception from the controller creates two spans, the timeline is following:

  1. span GET /url is started
  2. exception thrown inside controller
  3. span GET /url is finished
  4. dispatcher servlet forwards to /error, span GET /error is started as follows-from the GET /url span
  5. span GET /error is finished

I'd expect that there is a span which spans (duh!) the whole processing of the request - one that is started in 1) and finished in 5). Ideally the first span (GET /url) would not finish in 3) but in 5).

I did some debugging and it seems that the forward to /error happens in tomcat code - at the end of org.apache.catalina.core.StandardHostValve.throwable():

// A custom error-page has not been defined for the exception
// that was thrown during request processing. Check if an
// error-page for error code 500 was specified and if so,
// send that page back as the response.
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
// The response is an error
response.setError();

status(request, response);

It's similar in Jetty but in this case this behaviour is a part of spring code - the forward happens in org.eclipse.jetty.servlet.ServletHandler.doHandle():

if (!response.isCommitted())
{
    baseRequest.getResponse().getHttpFields().put(HttpHeader.CONNECTION,HttpHeaderValue.CLOSE);
    if (th instanceof UnavailableException)
    {
        UnavailableException ue = (UnavailableException)th;
        if (ue.isPermanent())
            response.sendError(HttpServletResponse.SC_NOT_FOUND);
        else
            response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
    }
    else
        response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); // <----- HERE
}

Now - there are several cases where this behaviour is not present - e.g. if the response is committed (above code). But also (see org.springframework.web.servlet.DispatcherServlet.processDispatchResult):

  • if exception extends ModelAndViewDefiningException - it defines where to forward
  • if it can be resolved by any of registered HandlerExceptionResolver

The case of HandlerExceptionResolver is interesting because this the way to handle exceptions (http://docs.spring.io/spring/docs/current/spring-framework-reference/htmlsingle/#mvc-ann-exceptionhandler).

Registering such handler completely changes the order of span starts/stops to the expected one (A start, B start, B finish, A finish). The forward to /error still happens (this time inside org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle) but before the original request is "completed". But only if the exception handler returned response text - if only response code is set then the forward won't happen.

I have hard time believing what's going on in this code...

But there's more. The above behaviour happens when I use @ResponseStatus:

@ResponseStatus(value = HttpStatus.I_AM_A_TEAPOT, reason = "Teapot failure")
@ExceptionHandler(Exception.class)
public void mapit() {}

But if instead I return ResponseEntity the forward is gone and there's only 1 span (see ServletInvocableHandlerMethod.setResponseStatus()):

@ExceptionHandler(Exception.class)
public ResponseEntity<String> mapit() {
    return ResponseEntity.status(418).body("rotfl");
}

So I guess this will be my new workaround. Using @ResponseBody also works this way.

But now the exception is not really captured, it has to be extracted from request.getAttribute(DispatcherServlet.EXCEPTION_ATTRIBUTE).

TL,DR: To avoid redirect to /error:

  • define @ExceptionHandler
  • don't use @ResponseStatus or make sure that reason is an empty string

See also: http://stackoverflow.com/questions/28902374/spring-boot-rest-service-exception-handling#answer-30193013 (I haven't checked if those properties work)

mabn avatar Mar 02 '17 03:03 mabn