Commit 00b76490 authored by Phillip Webb's avatar Phillip Webb

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
parent 43071b93
...@@ -18,8 +18,12 @@ package org.springframework.boot.web.support; ...@@ -18,8 +18,12 @@ package org.springframework.boot.web.support;
import java.io.IOException; import java.io.IOException;
import java.io.PrintWriter; import java.io.PrintWriter;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap; import java.util.HashMap;
import java.util.HashSet;
import java.util.Map; import java.util.Map;
import java.util.Set;
import javax.servlet.Filter; import javax.servlet.Filter;
import javax.servlet.FilterChain; import javax.servlet.FilterChain;
...@@ -40,6 +44,7 @@ import org.springframework.boot.web.servlet.ErrorPageRegistrar; ...@@ -40,6 +44,7 @@ import org.springframework.boot.web.servlet.ErrorPageRegistrar;
import org.springframework.boot.web.servlet.ErrorPageRegistry; import org.springframework.boot.web.servlet.ErrorPageRegistry;
import org.springframework.core.Ordered; import org.springframework.core.Ordered;
import org.springframework.core.annotation.Order; import org.springframework.core.annotation.Order;
import org.springframework.util.ClassUtils;
import org.springframework.web.filter.OncePerRequestFilter; import org.springframework.web.filter.OncePerRequestFilter;
import org.springframework.web.util.NestedServletException; import org.springframework.web.util.NestedServletException;
...@@ -77,6 +82,14 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { ...@@ -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 String ERROR_STATUS_CODE = "javax.servlet.error.status_code";
private static final Set<Class<?>> CLIENT_ABORT_EXCEPTIONS;
static {
Set<Class<?>> clientAbortExceptions = new HashSet<>();
addClassIfPresent(clientAbortExceptions,
"org.apache.catalina.connector.ClientAbortException");
CLIENT_ABORT_EXCEPTIONS = Collections.unmodifiableSet(clientAbortExceptions);
}
private String global; private String global;
private final Map<Integer, String> statuses = new HashMap<Integer, String>(); private final Map<Integer, String> statuses = new HashMap<Integer, String>();
...@@ -164,7 +177,6 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { ...@@ -164,7 +177,6 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry {
handleCommittedResponse(request, ex); handleCommittedResponse(request, ex);
return; return;
} }
forwardToErrorPage(errorPath, request, wrapped, ex); forwardToErrorPage(errorPath, request, wrapped, ex);
} }
...@@ -200,6 +212,9 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { ...@@ -200,6 +212,9 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry {
} }
private void handleCommittedResponse(HttpServletRequest request, Throwable ex) { private void handleCommittedResponse(HttpServletRequest request, Throwable ex) {
if (isClientAbortException(ex)) {
return;
}
String message = "Cannot forward to error page for request " String message = "Cannot forward to error page for request "
+ getDescription(request) + " as the response has already been" + getDescription(request) + " as the response has already been"
+ " committed. As a result, the response may have the wrong status" + " committed. As a result, the response may have the wrong status"
...@@ -216,6 +231,18 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { ...@@ -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<Integer, String> map, Integer status) { private String getErrorPath(Map<Integer, String> map, Integer status) {
if (map.containsKey(status)) { if (map.containsKey(status)) {
return map.get(status); return map.get(status);
...@@ -276,6 +303,15 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry { ...@@ -276,6 +303,15 @@ public class ErrorPageFilter implements Filter, ErrorPageRegistry {
public void destroy() { public void destroy() {
} }
private static void addClassIfPresent(Collection<Class<?>> collection,
String className) {
try {
collection.add(ClassUtils.forName(className, null));
}
catch (Throwable ex) {
}
}
private static class ErrorWrapperResponse extends HttpServletResponseWrapper { private static class ErrorWrapperResponse extends HttpServletResponseWrapper {
private int status; private int status;
......
...@@ -28,6 +28,7 @@ import javax.servlet.ServletResponse; ...@@ -28,6 +28,7 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpServletResponseWrapper; import javax.servlet.http.HttpServletResponseWrapper;
import org.apache.catalina.connector.ClientAbortException;
import org.junit.Rule; import org.junit.Rule;
import org.junit.Test; import org.junit.Test;
...@@ -150,6 +151,25 @@ public class ErrorPageFilterTests { ...@@ -150,6 +151,25 @@ public class ErrorPageFilterTests {
assertThat(this.response.isCommitted()).isTrue(); 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 @Test
public void responseUncommittedWithoutErrorPage() throws Exception { public void responseUncommittedWithoutErrorPage() throws Exception {
this.chain = new MockFilterChain() { this.chain = new MockFilterChain() {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment