Update ErrorPageFilter so it won’t try to forward a committed response

In some scenarios, the ErrorPageFilter will want to forward the request
to an error page but the response has already been committed. One common
cause of this is when the filter’s running on WAS. WAS calls
flushBuffer() (which commits the response), upon a clean exit from a
servlet’s service method.

Previously, the filter would attempt the forward, even if the response
was committed. This would result in an IllegalStateException and a
possibly incomplete response that may also have an incorrect status
code.

This commit updates the ErrorPageFilter to check to see if the response
has already been committed before it attempts to forward the request to
the error page. If the response has already been committed, the filter
logs an error and allows the container’s normal handling to kick in.
This prevents an IllegalStateException from being thrown.

This commit also updates the response wrapper to keep track of when
sendError has been called. Now, when flushBuffer is called, if
sendError has been called, the wrapper calls sendError on the wrapped
response. This prevents the wrapper from suppressing an error when the
response is committed before the request handling returns to the error
page filter.

Closes gh-1575

Author: wilkinsona

spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc

@@ -952,8 +952,8 @@ Spring Boot includes auto-configuration support for the following templating eng
  * http://www.thymeleaf.org[Thymeleaf]
  * http://velocity.apache.org[Velocity]
 
-When you're using one of these templating engines with the default configuration, your templates
-will be picked up automatically from `src/main/resources/templates`.
+When you're using one of these templating engines with the default configuration, your
+templates will be picked up automatically from `src/main/resources/templates`.
 
 TIP: JSPs should be avoided if possible, there are several
 <<boot-features-jsp-limitations, known limitations>> when using them with embedded
@@ -988,36 +988,45 @@ support a uniform Java DSL for customizing the error handling. For example:
 
 		@Override
 		public void customize(ConfigurableEmbeddedServletContainer container) {
-            container.addErrorPages(new ErrorPage(HttpStatus.BAD_REQUEST, "/400"));
+			container.addErrorPages(new ErrorPage(HttpStatus.BAD_REQUEST, "/400"));
 		}
 
 	}
 ----
 
-You can also use regular Spring MVC features like http://docs.spring.io/spring/docs/current/spring-framework-reference/htmlsingle/#mvc-exception-handlers[`@ExceptionHandler`
-methods] and http://docs.spring.io/spring/docs/current/spring-framework-reference/htmlsingle/#mvc-ann-controller-advice[`@ControllerAdvice`].
-The `ErrorController` will then pick up any unhandled exceptions.
+You can also use regular Spring MVC features like
+{spring-reference}/#mvc-exceptionhandlers[`@ExceptionHandler` methods] and
+{spring-reference}/#mvc-ann-controller-advice[`@ControllerAdvice`]. The `ErrorController`
+will then pick up any unhandled exceptions.
 
 N.B. if you register an `ErrorPage` with a path that will end up being handled by a
-`Filter` (e.g. as is common with some non-Spring web frameworks,  like Jersey and Wicket),
+`Filter` (e.g. as is common with some non-Spring web frameworks, like Jersey and Wicket),
 then the `Filter` has to be explicitly registered as an `ERROR` dispatcher, e.g.
 
 [source,java,indent=0,subs="verbatim,quotes,attributes"]
 ----
 	@Bean
 	public FilterRegistrationBean myFilter() {
-
-	  FilterRegistrationBean registration = new FilterRegistrationBean();
-	  registration.setFilter(new MyFilter());
-	  ...
-	  registration.setDispatcherTypes(EnumSet.allOf(DispatcherType.class));
-	  return registration;
-
+		FilterRegistrationBean registration = new FilterRegistrationBean();
+		registration.setFilter(new MyFilter());
+		...
+		registration.setDispatcherTypes(EnumSet.allOf(DispatcherType.class));
+		return registration;
 	}
 ----
 
 (the default `FilterRegistrationBean` does not include the `ERROR` dispatcher type).
 
+[[boot-features-error-handling-websphere]]
+===== Error Handling on WebSphere Application Server
+When deployed to a servlet container, a Spring Boot uses its error page filter to
+forward a request with an error status to the appropriate error page. The request can
+only be forwarded to the correct error page if the response has not already been
+committed. By default, WebSphere Application Server 8.0 and later commits the response
+upon  successful completion of a servlet's service method. You should disable this
+behaviour by setting `com.ibm.ws.webcontainer.invokeFlushAfterService` to `false`
+
+
 
 
 [[boot-features-embedded-container]]

spring-boot/src/main/java/org/springframework/boot/context/web/ErrorPageFilter.java

@@ -52,6 +52,7 @@
  *
  * @author Dave Syer
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 @Component
 @Order(Ordered.HIGHEST_PRECEDENCE)
@@ -124,6 +125,12 @@ else if (!request.isAsyncStarted()) {
 	private void handleErrorStatus(HttpServletRequest request,
 			HttpServletResponse response, int status, String message)
 			throws ServletException, IOException {
+
+		if (response.isCommitted()) {
+			handleCommittedResponse(request, null);
+			return;
+		}
+
 		String errorPath = getErrorPath(this.statuses, status);
 		if (errorPath == null) {
 			response.sendError(status, message);
@@ -132,6 +139,7 @@ private void handleErrorStatus(HttpServletRequest request,
 		response.setStatus(status);
 		setErrorAttributes(request, status, message);
 		request.getRequestDispatcher(errorPath).forward(request, response);
+
 	}
 
 	private void handleException(HttpServletRequest request,
@@ -143,33 +151,49 @@ private void handleException(HttpServletRequest request,
 			rethrow(ex);
 			return;
 		}
+		if (response.isCommitted()) {
+			handleCommittedResponse(request, ex);
+			return;
+		}
+
+		forwardToErrorPage(errorPath, request, wrapped, ex);
+	}
+
+	private void forwardToErrorPage(String path, HttpServletRequest request,
+			HttpServletResponse response, Throwable ex) throws ServletException,
+			IOException {
+
 		if (logger.isErrorEnabled()) {
 			String message = "Forwarding to error page from request ["
 					+ request.getServletPath() + request.getPathInfo()
 					+ "] due to exception [" + ex.getMessage() + "]";
 			logger.error(message, ex);
 		}
+
 		setErrorAttributes(request, 500, ex.getMessage());
 		request.setAttribute(ERROR_EXCEPTION, ex);
-		request.setAttribute(ERROR_EXCEPTION_TYPE, type.getName());
-		forwardToErrorPage(errorPath, request, wrapped, ex);
+		request.setAttribute(ERROR_EXCEPTION_TYPE, ex.getClass().getName());
+
+		response.reset();
+		response.sendError(500, ex.getMessage());
+		request.getRequestDispatcher(path).forward(request, response);
 	}
 
-	private void forwardToErrorPage(String path, HttpServletRequest request,
-			HttpServletResponse response, Throwable ex) throws ServletException,
-			IOException {
-		if (response.isCommitted()) {
-			String message = "Cannot forward to error page for" + request.getRequestURI()
-					+ " (response is committed), so this response may have "
-					+ "the wrong status code";
+	private void handleCommittedResponse(HttpServletRequest request, Throwable ex) {
+		String message = "Cannot forward to error page for request to "
+				+ request.getRequestURI() + " as the response has already been"
+				+ " committed. As a result, the response may have the wrong status"
+				+ " code. If your application is running on WebSphere Application"
+				+ " Server you may be able to resolve this problem by setting "
+				+ " com.ibm.ws.webcontainer.invokeFlushAfterService to false";
+		if (ex == null) {
+			logger.error(message);
+		}
+		else {
 			// User might see the error page without all the data here but throwing the
 			// exception isn't going to help anyone (we'll log it to be on the safe side)
 			logger.error(message, ex);
-			return;
 		}
-		response.reset();
-		response.sendError(500, ex.getMessage());
-		request.getRequestDispatcher(path).forward(request, response);
 	}
 
 	private String getErrorPath(Map<Integer, String> map, Integer status) {
@@ -243,6 +267,8 @@ private static class ErrorWrapperResponse extends HttpServletResponseWrapper {
 
 		private String message;
 
+		private boolean errorToSend;
+
 		public ErrorWrapperResponse(HttpServletResponse response) {
 			super(response);
 			this.status = response.getStatus();
@@ -257,13 +283,24 @@ public void sendError(int status) throws IOException {
 		public void sendError(int status, String message) throws IOException {
 			this.status = status;
 			this.message = message;
+
+			this.errorToSend = true;
 		}
 
 		@Override
 		public int getStatus() {
 			return this.status;
 		}
 
+		@Override
+		public void flushBuffer() throws IOException {
+			if (this.errorToSend && !isCommitted()) {
+				((HttpServletResponse) getResponse())
+						.sendError(this.status, this.message);
+			}
+			super.flushBuffer();
+		}
+
 		public String getMessage() {
 			return this.message;
 		}

spring-boot/src/test/java/org/springframework/boot/context/web/ErrorPageFilterTests.java

@@ -34,6 +34,8 @@
 import org.springframework.mock.web.MockHttpServletResponse;
 
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
@@ -43,6 +45,7 @@
  * Tests for {@link ErrorPageFilter}.
  *
  * @author Dave Syer
+ * @author Andy Wilkinson
  */
 public class ErrorPageFilterTests {
 
@@ -61,6 +64,7 @@ public void notAnError() throws Exception {
 		assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getResponse(),
 				equalTo((ServletResponse) this.response));
 		assertTrue(this.response.isCommitted());
+		assertThat(this.response.getForwardedUrl(), is(nullValue()));
 	}
 
 	@Test
@@ -83,6 +87,7 @@ public void doFilter(ServletRequest request, ServletResponse response)
 		assertThat(wrapper.getStatus(), equalTo(401));
 		// The real response has to be 401 as well...
 		assertThat(this.response.getStatus(), equalTo(401));
+		assertThat(this.response.getForwardedUrl(), equalTo("/error"));
 	}
 
 	@Test
@@ -103,11 +108,12 @@ public void doFilter(ServletRequest request, ServletResponse response)
 				equalTo((ServletResponse) this.response));
 		assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus(),
 				equalTo(400));
+		assertThat(this.response.getForwardedUrl(), is(nullValue()));
 		assertTrue(this.response.isCommitted());
 	}
 
 	@Test
-	public void responseUncommitted() throws Exception {
+	public void responseUncommittedWithoutErrorPage() throws Exception {
 		this.chain = new MockFilterChain() {
 			@Override
 			public void doFilter(ServletRequest request, ServletResponse response)
@@ -122,6 +128,7 @@ public void doFilter(ServletRequest request, ServletResponse response)
 				equalTo((ServletResponse) this.response));
 		assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus(),
 				equalTo(400));
+		assertThat(this.response.getForwardedUrl(), is(nullValue()));
 		assertTrue(this.response.isCommitted());
 	}
 
@@ -159,6 +166,7 @@ public void doFilter(ServletRequest request, ServletResponse response)
 		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_MESSAGE),
 				equalTo((Object) "BAD"));
 		assertTrue(this.response.isCommitted());
+		assertThat(this.response.getForwardedUrl(), equalTo("/error"));
 	}
 
 	@Test
@@ -180,6 +188,26 @@ public void doFilter(ServletRequest request, ServletResponse response)
 		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_MESSAGE),
 				equalTo((Object) "BAD"));
 		assertTrue(this.response.isCommitted());
+		assertThat(this.response.getForwardedUrl(), equalTo("/400"));
+	}
+
+	@Test
+	public void statusErrorWithCommittedResponse() throws Exception {
+		this.filter.addErrorPages(new ErrorPage(HttpStatus.BAD_REQUEST, "/400"));
+		this.chain = new MockFilterChain() {
+			@Override
+			public void doFilter(ServletRequest request, ServletResponse response)
+					throws IOException, ServletException {
+				((HttpServletResponse) response).sendError(400, "BAD");
+				response.flushBuffer();
+				super.doFilter(request, response);
+			}
+		};
+		this.filter.doFilter(this.request, this.response, this.chain);
+		assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus(),
+				equalTo(400));
+		assertTrue(this.response.isCommitted());
+		assertThat(this.response.getForwardedUrl(), is(nullValue()));
 	}
 
 	@Test
@@ -203,6 +231,23 @@ public void doFilter(ServletRequest request, ServletResponse response)
 		assertThat(this.request.getAttribute(RequestDispatcher.ERROR_EXCEPTION_TYPE),
 				equalTo((Object) RuntimeException.class.getName()));
 		assertTrue(this.response.isCommitted());
+		assertThat(this.response.getForwardedUrl(), equalTo("/500"));
+	}
+
+	@Test
+	public void exceptionErrorWithCommittedResponse() throws Exception {
+		this.filter.addErrorPages(new ErrorPage(RuntimeException.class, "/500"));
+		this.chain = new MockFilterChain() {
+			@Override
+			public void doFilter(ServletRequest request, ServletResponse response)
+					throws IOException, ServletException {
+				super.doFilter(request, response);
+				response.flushBuffer();
+				throw new RuntimeException("BAD");
+			}
+		};
+		this.filter.doFilter(this.request, this.response, this.chain);
+		assertThat(this.response.getForwardedUrl(), is(nullValue()));
 	}
 
 	@Test