From 43071b9375dc4a7519f8997a49e9bfdc59b9aa59 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 30 May 2018 13:34:15 -0700 Subject: [PATCH 1/2] Polish formatting --- .../web/support/ErrorPageFilterTests.java | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java b/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java index ed691645e32..15eb5d471c3 100644 --- a/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java @@ -83,6 +83,7 @@ public class ErrorPageFilterTests { @Test public void notAnErrorButNotOK() throws Exception { this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { @@ -90,6 +91,7 @@ public class ErrorPageFilterTests { super.doFilter(request, response); response.flushBuffer(); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponse) this.chain.getResponse()).getStatus()) @@ -103,12 +105,14 @@ public class ErrorPageFilterTests { public void unauthorizedWithErrorPath() throws Exception { this.filter.addErrorPages(new ErrorPage("/error")); this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { ((HttpServletResponse) response).sendError(401, "UNAUTHORIZED"); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.chain.getRequest()).isEqualTo(this.request); @@ -127,12 +131,14 @@ public class ErrorPageFilterTests { this.filter.addErrorPages(new ErrorPage("/error")); this.response.setCommitted(true); this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { ((HttpServletResponse) response).sendError(400, "BAD"); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.chain.getRequest()).isEqualTo(this.request); @@ -147,12 +153,14 @@ public class ErrorPageFilterTests { @Test public void responseUncommittedWithoutErrorPage() throws Exception { this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { ((HttpServletResponse) response).sendError(400, "BAD"); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.chain.getRequest()).isEqualTo(this.request); @@ -167,6 +175,7 @@ public class ErrorPageFilterTests { @Test public void oncePerRequest() throws Exception { this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { @@ -174,6 +183,7 @@ public class ErrorPageFilterTests { assertThat(request.getAttribute("FILTER.FILTERED")).isNotNull(); super.doFilter(request, response); } + }; this.filter.init(new MockFilterConfig("FILTER")); this.filter.doFilter(this.request, this.response, this.chain); @@ -183,12 +193,14 @@ public class ErrorPageFilterTests { public void globalError() throws Exception { this.filter.addErrorPages(new ErrorPage("/error")); this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { ((HttpServletResponse) response).sendError(400, "BAD"); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus()) @@ -207,12 +219,14 @@ public class ErrorPageFilterTests { public void statusError() 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"); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus()) @@ -231,6 +245,7 @@ public class ErrorPageFilterTests { 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 { @@ -238,6 +253,7 @@ public class ErrorPageFilterTests { response.flushBuffer(); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus()) @@ -250,12 +266,14 @@ public class ErrorPageFilterTests { public void exceptionError() 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); throw new RuntimeException("BAD"); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus()) @@ -282,6 +300,7 @@ public class ErrorPageFilterTests { 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 { @@ -289,6 +308,7 @@ public class ErrorPageFilterTests { response.flushBuffer(); throw new RuntimeException("BAD"); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.response.getForwardedUrl()).isNull(); @@ -297,12 +317,14 @@ public class ErrorPageFilterTests { @Test public void statusCode() throws Exception { this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { assertThat(((HttpServletResponse) response).getStatus()).isEqualTo(200); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus()) @@ -313,12 +335,14 @@ public class ErrorPageFilterTests { public void subClassExceptionError() 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); throw new IllegalStateException("BAD"); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus()) @@ -356,12 +380,14 @@ public class ErrorPageFilterTests { this.filter.addErrorPages(new ErrorPage("/error")); this.request.setAsyncStarted(true); this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { super.doFilter(request, response); throw new RuntimeException("BAD"); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.chain.getRequest()).isEqualTo(this.request); @@ -376,12 +402,14 @@ public class ErrorPageFilterTests { this.filter.addErrorPages(new ErrorPage("/error")); this.request.setAsyncStarted(true); this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { super.doFilter(request, response); ((HttpServletResponse) response).sendError(400, "BAD"); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.chain.getRequest()).isEqualTo(this.request); @@ -406,12 +434,14 @@ public class ErrorPageFilterTests { this.filter.addErrorPages(new ErrorPage("/error")); setUpAsyncDispatch(); this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { super.doFilter(request, response); throw new RuntimeException("BAD"); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.chain.getRequest()).isEqualTo(this.request); @@ -426,12 +456,14 @@ public class ErrorPageFilterTests { this.filter.addErrorPages(new ErrorPage("/error")); setUpAsyncDispatch(); this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { super.doFilter(request, response); ((HttpServletResponse) response).sendError(400, "BAD"); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.chain.getRequest()).isEqualTo(this.request); @@ -493,12 +525,14 @@ public class ErrorPageFilterTests { public void nestedServletExceptionIsUnwrapped() 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); throw new NestedServletException("Wrapper", new RuntimeException("BAD")); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(((HttpServletResponseWrapper) this.chain.getResponse()).getStatus()) @@ -525,6 +559,7 @@ public class ErrorPageFilterTests { public void whenErrorIsSentAndWriterIsFlushedErrorIsSentToTheClient() throws Exception { this.chain = new MockFilterChain() { + @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { @@ -532,6 +567,7 @@ public class ErrorPageFilterTests { response.getWriter().flush(); super.doFilter(request, response); } + }; this.filter.doFilter(this.request, this.response, this.chain); assertThat(this.response.getStatus()).isEqualTo(400); From 00b76490dc72fc21ff437c57c26c9583967c17bb Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 30 May 2018 14:00:55 -0700 Subject: [PATCH 2/2] Remove error logging on ClientAbortException Update `ErrorPageFilter` so that a Tomcat `ClientAbortException` no longer causes "Cannot forward to error page for request" logging for committed responses. Since a `ClientAbortException` indicates that the client is no longer available it's of no consequence that the request has been committed and the forward will fail. Closes gh-13221 --- .../boot/web/support/ErrorPageFilter.java | 38 ++++++++++++++++++- .../web/support/ErrorPageFilterTests.java | 20 ++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java b/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java index d4c9102fd15..41826d1bdbd 100644 --- a/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java +++ b/spring-boot/src/main/java/org/springframework/boot/web/support/ErrorPageFilter.java @@ -18,8 +18,12 @@ package org.springframework.boot.web.support; import java.io.IOException; import java.io.PrintWriter; +import java.util.Collection; +import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -40,6 +44,7 @@ import org.springframework.boot.web.servlet.ErrorPageRegistrar; import org.springframework.boot.web.servlet.ErrorPageRegistry; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; +import org.springframework.util.ClassUtils; import org.springframework.web.filter.OncePerRequestFilter; import org.springframework.web.util.NestedServletException; @@ -77,6 +82,14 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { private static final String ERROR_STATUS_CODE = "javax.servlet.error.status_code"; + private static final Set> CLIENT_ABORT_EXCEPTIONS; + static { + Set> clientAbortExceptions = new HashSet<>(); + addClassIfPresent(clientAbortExceptions, + "org.apache.catalina.connector.ClientAbortException"); + CLIENT_ABORT_EXCEPTIONS = Collections.unmodifiableSet(clientAbortExceptions); + } + private String global; private final Map statuses = new HashMap(); @@ -164,7 +177,6 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { handleCommittedResponse(request, ex); return; } - forwardToErrorPage(errorPath, request, wrapped, ex); } @@ -200,6 +212,9 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { } private void handleCommittedResponse(HttpServletRequest request, Throwable ex) { + if (isClientAbortException(ex)) { + return; + } String message = "Cannot forward to error page for request " + getDescription(request) + " as the response has already been" + " committed. As a result, the response may have the wrong status" @@ -216,6 +231,18 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { } } + private boolean isClientAbortException(Throwable ex) { + if (ex == null) { + return false; + } + for (Class candidate : CLIENT_ABORT_EXCEPTIONS) { + if (candidate.isInstance(ex)) { + return true; + } + } + return isClientAbortException(ex.getCause()); + } + private String getErrorPath(Map map, Integer status) { if (map.containsKey(status)) { return map.get(status); @@ -276,6 +303,15 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { public void destroy() { } + private static void addClassIfPresent(Collection> collection, + String className) { + try { + collection.add(ClassUtils.forName(className, null)); + } + catch (Throwable ex) { + } + } + private static class ErrorWrapperResponse extends HttpServletResponseWrapper { private int status; diff --git a/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java b/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java index 15eb5d471c3..ed60e84ca8c 100644 --- a/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/web/support/ErrorPageFilterTests.java @@ -28,6 +28,7 @@ import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponseWrapper; +import org.apache.catalina.connector.ClientAbortException; import org.junit.Rule; import org.junit.Test; @@ -150,6 +151,25 @@ public class ErrorPageFilterTests { assertThat(this.response.isCommitted()).isTrue(); } + @Test + public void responseCommittedWhenFromClientAbortException() throws Exception { + this.filter.addErrorPages(new ErrorPage("/error")); + this.response.setCommitted(true); + this.chain = new MockFilterChain() { + + @Override + public void doFilter(ServletRequest request, ServletResponse response) + throws IOException, ServletException { + super.doFilter(request, response); + throw new ClientAbortException(); + } + + }; + this.filter.doFilter(this.request, this.response, this.chain); + assertThat(this.response.isCommitted()).isTrue(); + assertThat(this.output.toString()).doesNotContain("Cannot forward"); + } + @Test public void responseUncommittedWithoutErrorPage() throws Exception { this.chain = new MockFilterChain() {