Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mabn opened this issue Mar 2, 2017 · 0 comments

Comments

@mabn
Copy link

mabn commented Mar 2, 2017

This is related to this comment: #1 (comment) 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:https://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:https://stackoverflow.com/questions/28902374/spring-boot-rest-service-exception-handling#answer-30193013 (I haven't checked if those properties work)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant