From 0d7c9b7c937b46fb82db304cbc6cbd41a08ed139 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Wed, 30 Aug 2023 15:42:15 +0200 Subject: [PATCH] Attempt to reset Servlet response in DispatcherServlet This is a follow-up change related to gh-31104. This change reverts the changes previously made in `ExceptionHandlerExceptionResolver` and instead attempts to reset the response directly in `DispatcherServlet` in order to cover all types or exception handling. Unlike the previous change, we decided to continue even if the response was already committed: exception handlers will have a chance to be called, even if it means they'll have to operate on a garbled response. This change will cause less disruption, in case existing exception handlers were relying on this behavior. See gh-31104 --- .../web/servlet/DispatcherServlet.java | 7 +++ .../ExceptionHandlerExceptionResolver.java | 2 +- .../web/servlet/DispatcherServletTests.java | 48 +++++++++++++++++++ ...xceptionHandlerExceptionResolverTests.java | 32 ------------- 4 files changed, 56 insertions(+), 33 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/DispatcherServlet.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/DispatcherServlet.java index aeb2a025a3..693c1f1f08 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/DispatcherServlet.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/DispatcherServlet.java @@ -1338,6 +1338,13 @@ public class DispatcherServlet extends FrameworkServlet { // Success and error responses may use different content types request.removeAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE); + // Reset the response if the response is not committed already + try { + response.reset(); + } + catch (IllegalStateException illegalStateException) { + // the response is already committed, leave it to exception handlers anyway + } // Check registered HandlerExceptionResolvers... ModelAndView exMv = null; diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java index 0bfa199090..067fb5af9c 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java @@ -401,9 +401,9 @@ public class ExceptionHandlerExceptionResolver extends AbstractHandlerMethodExce ServletWebRequest webRequest = new ServletWebRequest(request, response); ModelAndViewContainer mavContainer = new ModelAndViewContainer(); + ArrayList exceptions = new ArrayList<>(); try { - response.reset(); if (logger.isDebugEnabled()) { logger.debug("Using @ExceptionHandler " + exceptionHandlerMethod); } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/DispatcherServletTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/DispatcherServletTests.java index ce8f961677..e9c784f64e 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/DispatcherServletTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/DispatcherServletTests.java @@ -70,12 +70,15 @@ import org.springframework.web.util.pattern.PathPatternParser; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; /** + * Tests for {@link DispatcherServlet}. + * * @author Rod Johnson * @author Juergen Hoeller * @author Rob Harrop @@ -887,6 +890,39 @@ public class DispatcherServletTests { assertThat(response.getContentAsString()).isEqualTo("body"); } + @Test + public void shouldResetResponseIfNotCommitted() throws Exception { + StaticWebApplicationContext context = new StaticWebApplicationContext(); + context.setServletContext(getServletContext()); + context.registerSingleton("/error", ErrorController.class); + DispatcherServlet servlet = new DispatcherServlet(context); + servlet.init(servletConfig); + + MockHttpServletRequest request = new MockHttpServletRequest(getServletContext(), "GET", "/error"); + MockHttpServletResponse response = new MockHttpServletResponse(); + assertThatThrownBy(() -> servlet.service(request, response)).isInstanceOf(ServletException.class) + .hasCauseInstanceOf(IllegalArgumentException.class); + assertThat(response.getContentAsByteArray()).isEmpty(); + assertThat(response.getStatus()).isEqualTo(200); + } + + @Test + public void shouldAttemptToResetResponseIfCommitted() throws Exception { + StaticWebApplicationContext context = new StaticWebApplicationContext(); + context.setServletContext(getServletContext()); + context.registerSingleton("/error", ErrorController.class); + DispatcherServlet servlet = new DispatcherServlet(context); + servlet.init(servletConfig); + + MockHttpServletRequest request = new MockHttpServletRequest(getServletContext(), "GET", "/error"); + request.setAttribute("commit", true); + MockHttpServletResponse response = new MockHttpServletResponse(); + assertThatThrownBy(() -> servlet.service(request, response)).isInstanceOf(ServletException.class) + .hasCauseInstanceOf(IllegalArgumentException.class); + assertThat(response.getContentAsByteArray()).isNotEmpty(); + assertThat(response.getStatus()).isEqualTo(400); + } + public static class ControllerFromParent implements Controller { @@ -934,4 +970,16 @@ public class DispatcherServletTests { } } + private static class ErrorController implements Controller { + @Override + public ModelAndView handleRequest(HttpServletRequest request, HttpServletResponse response) throws Exception { + response.setStatus(400); + if (request.getAttribute("commit") != null) { + response.flushBuffer(); + } + response.getWriter().write("error"); + throw new IllegalArgumentException("test error"); + } + } + } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java index 386891ddee..dfc9febb1d 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java @@ -385,38 +385,6 @@ public class ExceptionHandlerExceptionResolverTests { assertExceptionHandledAsBody(mav, "DefaultTestExceptionResolver: IllegalStateException"); } - @Test // gh-30702 - void attemptToResetResponseBeforeResolveException() throws Exception { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class); - this.resolver.setMappedHandlerClasses(HttpRequestHandler.class); - this.resolver.setApplicationContext(ctx); - this.resolver.afterPropertiesSet(); - - IllegalStateException ex = new IllegalStateException(); - ResourceHttpRequestHandler handler = new ResourceHttpRequestHandler(); - this.response.getWriter().print("test"); - ModelAndView mav = this.resolver.resolveException(this.request, this.response, handler, ex); - - assertExceptionHandledAsBody(mav, "DefaultTestExceptionResolver: IllegalStateException"); - } - - @Test // gh-30702 - void attemptToResetResponseBeforeResolveExceptionFails() throws Exception { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class); - this.resolver.setMappedHandlerClasses(HttpRequestHandler.class); - this.resolver.setApplicationContext(ctx); - this.resolver.afterPropertiesSet(); - - IllegalStateException ex = new IllegalStateException(); - ResourceHttpRequestHandler handler = new ResourceHttpRequestHandler(); - this.response.getWriter().print("test"); - this.response.setCommitted(true); - ModelAndView mav = this.resolver.resolveException(this.request, this.response, handler, ex); - - assertThat(mav).as("Exception was handled").isNull(); - assertThat(this.response.getContentAsString()).isEqualTo("test"); - } - private void assertMethodProcessorCount(int resolverCount, int handlerCount) { assertThat(this.resolver.getArgumentResolvers().getResolvers()).hasSize(resolverCount);