Double error response handling on suspended methods

[nilskohrs]

The combination of spring-boot-starter-web, kotlinx-coroutines-reactor, a suspended controller method and handling a 500 error response through ResponseEntityExceptionHandler causes double exception handling and response bodies.

The first response body is generated by the ProblemDetail handled with ResponseEntityExceptionHandler.handleException(...), the second is generated by BasicErrorController

Reproduction steps (tested on version 3.0.0 and 3.1.2):

• Create spring initializr project with Kotlin and spring-boot-starter-web
• Add org.jetbrains.kotlinx:kotlinx-coroutines-reactor dependency
• Run following code

@SpringBootApplication
class DemoApplication
fun main(args: Array<String>) {
    runApplication<DemoApplication>(*args)
}

@RestController
class Controller {
    @GetMapping("normal")
    fun normalScenario() {
        throw IllegalStateException("Returns problemdetail")
    }
    @GetMapping("suspend")
    suspend fun errorScenario() {
        throw IllegalStateException("Double exception handling")
    }
}

@ControllerAdvice
class ExceptionHandler : ResponseEntityExceptionHandler() {
    @ExceptionHandler(Exception::class)
    fun handleUnexpectedTechnicalException(
        ex: Exception,
        request: WebRequest
    ): ResponseEntity<Any>? {
        return handleException(
            ErrorResponseException(
                HttpStatus.INTERNAL_SERVER_ERROR,
                ProblemDetail.forStatus(500),
                ex
            ), request
        )
    }
}

Endpoint /normal returns the expected Problem Detail

Endpoint /suspend with postman returns double body response (body size + ProblemDetail and then body size + BasicErrorController's response). Since this response is an invalid http response, a proxy like fiddler is needed to see this double response being returned.

HTTP/1.1 500
Content-Type: application/problem+json
Transfer-Encoding: chunked
Date: Tue, 08 Aug 2023 09:26:30 GMT
Connection: close

59
{"type":"about:blank","title":"Internal Server Error","status":500,"instance":"/suspend"}
6c
{"timestamp":"2023-08-08T09:26:30.705+00:00","status":500,"error":"Internal Server Error","path":"/suspend"}

Endpoint /suspend with chrome triggers following error:
s.e.ErrorMvcAutoConfiguration$StaticView : Cannot render error page for request [/suspend] as the response has already been committed. As a result, the response may have the wrong status code.

[nilskohrs]

I've found out that it's being triggered by setting the attribute jakarta.servlet.error.exception on the request. This happens in in ResponseEntityExceptionHandler.handleExceptionInternal(...)

...
if (statusCode.equals(HttpStatus.INTERNAL_SERVER_ERROR)) {
	request.setAttribute(WebUtils.ERROR_EXCEPTION_ATTRIBUTE, ex, WebRequest.SCOPE_REQUEST);
}
...

So I'm for now using a workaround by removing that attribute after calling ResponseEntityExceptionHandler.handleException(...)

@ControllerAdvice
class ExceptionHandler : ResponseEntityExceptionHandler() {
    @ExceptionHandler(Exception::class)
    fun handleUnexpectedTechnicalException(
        ex: Exception,
        request: WebRequest
    ): ResponseEntity<Any>? {
        return handleException(
            ErrorResponseException(
                HttpStatus.INTERNAL_SERVER_ERROR,
                ProblemDetail.forStatus(500),
                ex
            ), request
        ).also {
            request.removeAttribute(WebUtils.ERROR_EXCEPTION_ATTRIBUTE, WebRequest.SCOPE_REQUEST)
        }
    }
}

[nilskohrs]

org.apache.catalina.connector.CoyoteAdapter does check the request attribute jakarta.servlet.error.exception to start the second error handling in asyncDispatch(...)

...
if (request.isAsyncDispatching()) {
    connector.getService().getContainer().getPipeline().getFirst().invoke(request, response);
    Throwable t = (Throwable) request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
    if (t != null) {
        asyncConImpl.setErrorState(t, true);
    }
}
...

[nilskohrs]

Just tested it with Jetty and Undertow. Both handle the exception correctly

[wilkinsona]

Thanks for tracking that down, @nilskohrs. As the problem occurs with Tomcat but not with Jetty and Undertow, it would appear to be an issue with how Tomcat performs error handling for async requests (/cc @markt-asf). Please turn the steps and code snippets above into a complete yet minimal example and then open a Tomcat issue so that they can investigate.

[nilskohrs]

Bug reported with Tomcat: https://bz.apache.org/bugzilla/show_bug.cgi?id=66875

[markt-asf]

@wilkinsona you might want to review the Tomcat bug report. There was a Tomcat issue that has been fixed but there might be a Spring issue as well. The AsyncContext isn't being completed/dispatched in the AsyncListener's onError() event. It depends on what is considered a framework responsibility and what is an application responsibility

[wilkinsona]

Thanks, @markt-asf. I'll re-open this and we can transfer it to the Framework team so that they can review the behavior of org.springframework.web.context.request.async.StandardServletAsyncWebRequest (I assume that's relevant AsyncListener implementation here).

[jhoeller]

@rstoyanchev according to https://bz.apache.org/bugzilla/show_bug.cgi?id=66875#c11, our StandardServletAsyncWebRequest.onError implementation should call asyncContext.complete() if that context is still non-null. Let's sort this out for 6.1.3 and backport it to 6.0.16 and 5.3.32 from there.

[injae-kim]

Hi everyone! based on above #31541 (comment), I create simple fix PR #31953!

PTAL when you have some times 🙇 thank you!

[rstoyanchev]

StandardServletAsyncWebRequest is only an adapter that delegates Servlet callbacks to DeferredResult, DeferredResultProcessingInterceptor, WebAsyncTask, CallableProcessingInterceptor, and the like. It does not know if the request should complete immediately. In this case, onError is after we've fully handled a controller exception through an ASYNC dispatch, but it may also come from the Servlet container unrelated to the controller in which case we would still want follow-up handling even if the response may no longer be usable. In short, I don't think we should complete the AsyncContext from StandardServletAsyncWebRequest.

From Mark's comment on the Tomcat issue:

1. When Spring sets jakarta.servlet.error.exception that triggers Tomcat's internal error handling. Whether Spring should do that and whether that should have the effect it has are the first issue.

The request attribute was added a long time ago in 48b963a to assist with Servlet container defined error pages, and when it's only the response status that is set, it allows Boot's error handling to add a response body. However, with RFC 7807 responses we now set a response body, and shouldn't be setting the attribute if that's the case.

I verified that the fix on Tomcat's side apache/tomcat@276e04f does preclude the additional ERROR dispatch in this scenario, but we can also make a change to not even set the attribute when there is a response body.

[injae-kim]

Aha~ thank you @rstoyanchev for kind and detailed explanation! 👍

Anyway I'm big fan of spring-framework and want to contribute continuously so I'll try to resolve another issue~!