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.

Comment From: 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)
        }
    }
}

Comment From: 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);
    }
}
...

Comment From: nilskohrs

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

Comment From: 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.

Comment From: nilskohrs

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

Comment From: 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

Comment From: 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).

Comment From: 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.

Comment From: injae-kim

Hi everyone! based on above https://github.com/spring-projects/spring-framework/issues/31541#issuecomment-1870120560, I create simple fix PR https://github.com/spring-projects/spring-framework/pull/31953!

PTAL when you have some times 🙇 thank you!

Comment From: 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 48b963aaa5e122d8e21967c363233df245755d7e 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 https://github.com/apache/tomcat/commit/276e04f3d5ddab68914be9234bdca8fc06bf6412 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.

Comment From: 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~!