ExceptionHandlerExceptionResolver results in 200 for some special exceptions

[NiFNi]

I stumbled upon #34264 which caused some bigger issues internally because some server side connection resets resulted in a 200 successful response. While the biggest issue was solved I think that it is quite unsettling that an exception results in a 200 response and this is still the case for the current spring version (6.2.3) shipped with spring boot 3.4.3.

Minimal example which returns a 200 even though an exception is thrown and not properly handled:

@RestController
@ControllerAdvice
public class DemoController {

    private static final Logger log = LoggerFactory.getLogger(DemoController.class);

    @GetMapping
    public String demo() {
        throw new RuntimeException("connection reset by peer");
    }

    @ExceptionHandler(RuntimeException.class)
    public ResponseEntity<Object> handleRuntimeException(RuntimeException e) {
        log.error(e.getMessage());
        throw e;
    }
}

While this usecase might not look to useful we definitely have use cases where we want to handle an exception in some custom way using an exception handler but only in specific cases. So something like this:

    @ExceptionHandler(RuntimeException.class)
    public ResponseEntity<Object> handleRuntimeException(RuntimeException e) {
        if (matchesSomeCriteria(e)) {
            return specialHandling(e);
        }
        throw e;
    }

While investigating I found that this is probably caused here:

spring-framework/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java

Lines 463 to 465 in 75329e6

	if (disconnectedClientHelper.checkAndLogClientDisconnectedException(invocationEx)) {
	return new ModelAndView();
	}

If I understand this correctly the idea is to not continue handling exceptions in case the client closed the connection. Since the check for this is just using some string comparison on the exception message it is quite easy to have an exception matching this as can be seen above.

If no one has ideas on how to fix this properly I would suggest to at least set a 500 error status on the ModelView that is created in the ExceptionHandlerExceptionResolver to make this a little less worrying. While the behaviour might still be unexpected it is at least not returning successful responses for cases where disconnectedClientHelper.checkAndLogClientDisconnectedException returns true even though some internal exception occurred.

If you agree with my findings I can create a PR for this.

[rstoyanchev]

For "connection reset by peer" or "broken pipe" I would expect the exception type is an instance of IOException, and we could add that check to make it more specific.

However, it would be useful to find out more about your case. Is it exactly RuntimeException with the phrase "connection reset by peer", is it raised by a JDK library class, or some other 3rd party library, and also what is the connection it is referring to?

I would also like to confirm that #34264 is not a regression. As far as I can tell it didn't change anything for your case.

[NiFNi]

@rstoyanchev thanks for the reply!

I think we have a small misunderstanding. The actual problem we had is fixed with #34264 being fixed. I just stumbled upon this unexpected behaviour because of that bug. This issue is the suggestion to implement the following improvement: There should not be a case where an exception that contains some "random" string in the message results in a 200.
The example I provided above is mostly for reproduction. But I currently see the following two possibilities where a 200 response is returned even though an exception was thrown:

1. The thrown exception contains "connection reset by peer" or "broken pipe". I can definitely imagine cases where this is done without having any idea that this might cause problems in the Spring framework.
2. There is another bug introduced in the DiconnectedClientHelper which again results in matching exceptions to broadly and by that returning a success response.

So my goal with this issue is to suggest to make this less error prone by making sure that there is no path in the code which can lead to a 200 in case of exceptions.

And if I understand the code in the ExceptionHandlerExceptionResolver correctly the case is about the client closing the connection. In that case the ExceptionHandler chain should not be evaluated anymore since the client is "gone" anyway. And this is achieved by returning an empty response with the status 200. The easiest improvement in my opinion is to make this empty response a 500. In cases where the exception was correctly matched as a client disconnect this does not change anything because there is no client anymore to care about the response. In cases where the exception was incorrectly matched (for whatever reason) we make sure that there is no successful response when actually an error happened.