From 72ef1d6554635b9d223c44803f01592d09276c74 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Thu, 25 Sep 2014 18:07:27 +0100 Subject: [PATCH] =?UTF-8?q?Update=20ErrorPageFilter=20so=20it=20won?= =?UTF-8?q?=E2=80=99t=20try=20to=20forward=20a=20committed=20response?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../main/asciidoc/spring-boot-features.adoc | 37 ++++++----- .../boot/context/web/ErrorPageFilter.java | 63 +++++++++++++++---- .../context/web/ErrorPageFilterTests.java | 47 +++++++++++++- 3 files changed, 119 insertions(+), 28 deletions(-) diff --git a/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc b/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc index 0457c26446..91396fa184 100644 --- a/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc +++ b/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 <> 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]] diff --git a/spring-boot/src/main/java/org/springframework/boot/context/web/ErrorPageFilter.java b/spring-boot/src/main/java/org/springframework/boot/context/web/ErrorPageFilter.java index 69ebc5a9b3..ab1e253309 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/web/ErrorPageFilter.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/web/ErrorPageFilter.java @@ -52,6 +52,7 @@ import org.springframework.web.filter.OncePerRequestFilter; * * @author Dave Syer * @author Phillip Webb + * @author Andy Wilkinson */ @Component @Order(Ordered.HIGHEST_PRECEDENCE) @@ -124,6 +125,12 @@ class ErrorPageFilter extends AbstractConfigurableEmbeddedServletContainer imple 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 @@ class ErrorPageFilter extends AbstractConfigurableEmbeddedServletContainer imple response.setStatus(status); setErrorAttributes(request, status, message); request.getRequestDispatcher(errorPath).forward(request, response); + } private void handleException(HttpServletRequest request, @@ -143,33 +151,49 @@ class ErrorPageFilter extends AbstractConfigurableEmbeddedServletContainer imple 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 map, Integer status) { @@ -243,6 +267,8 @@ class ErrorPageFilter extends AbstractConfigurableEmbeddedServletContainer imple private String message; + private boolean errorToSend; + public ErrorWrapperResponse(HttpServletResponse response) { super(response); this.status = response.getStatus(); @@ -257,6 +283,8 @@ class ErrorPageFilter extends AbstractConfigurableEmbeddedServletContainer imple public void sendError(int status, String message) throws IOException { this.status = status; this.message = message; + + this.errorToSend = true; } @Override @@ -264,6 +292,15 @@ class ErrorPageFilter extends AbstractConfigurableEmbeddedServletContainer imple 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; } diff --git a/spring-boot/src/test/java/org/springframework/boot/context/web/ErrorPageFilterTests.java b/spring-boot/src/test/java/org/springframework/boot/context/web/ErrorPageFilterTests.java index 968df7f39f..c6dee4244d 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/web/ErrorPageFilterTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/web/ErrorPageFilterTests.java @@ -34,6 +34,8 @@ import org.springframework.mock.web.MockHttpServletRequest; 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 @@ import static org.junit.Assert.assertTrue; * Tests for {@link ErrorPageFilter}. * * @author Dave Syer + * @author Andy Wilkinson */ public class ErrorPageFilterTests { @@ -61,6 +64,7 @@ public class ErrorPageFilterTests { 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 class ErrorPageFilterTests { 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 class ErrorPageFilterTests { 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 class ErrorPageFilterTests { 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 class ErrorPageFilterTests { 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 class ErrorPageFilterTests { 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 class ErrorPageFilterTests { 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