WIP

Revert previous changes for spring-projectsgh-31104 and apply them on DispatcherServlet

Author: bclozel

spring-webmvc/src/main/java/org/springframework/web/servlet/DispatcherServlet.java

@@ -1338,6 +1338,14 @@ protected ModelAndView processHandlerException(HttpServletRequest request, HttpS
 
 		// 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
+		// throws an exception and skips error handling entirely otherwise
+		try {
+			response.reset();
+		}
+		catch (IllegalStateException illegalStateException) {
+			throw ex;
+		}
 
 		// Check registered HandlerExceptionResolvers...
 		ModelAndView exMv = null;

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

@@ -401,9 +401,9 @@ protected ModelAndView doResolveHandlerMethodException(HttpServletRequest reques
 
 		ServletWebRequest webRequest = new ServletWebRequest(request, response);
 		ModelAndViewContainer mavContainer = new ModelAndViewContainer();
+
 		ArrayList<Throwable> exceptions = new ArrayList<>();
 		try {
-			response.reset();
 			if (logger.isDebugEnabled()) {
 				logger.debug("Using @ExceptionHandler " + exceptionHandlerMethod);
 			}

spring-webmvc/src/test/java/org/springframework/web/servlet/DispatcherServletTests.java

@@ -70,12 +70,15 @@
 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 void webDavMethod() throws Exception {
 		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 SimpleUrlHandlerMapping handlerMapping() {
 		}
 	}
 
+	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");
+		}
+	}
+
 }

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java

@@ -385,38 +385,6 @@ void resolveExceptionViaMappedHandler() throws Exception {
 		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);