From dd5fe6852261db533306328f40e6fbe841946fdb Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 1 Mar 2024 20:41:27 +0000 Subject: [PATCH] Dispatch again after disconnected client error Dispatching was prevented for disconnected client errors after recent reports like #32042 when running on Tomcat, and the async request was completed from the onError notification. This has the side effect of not allowing exception resolvers to take final action even if the response is not writeable. After all updates for this issue, it appears the dispatch no longer causes issues. Tomcat actually does not do the dispatch, but it doesn't seem to cause any issues, and on other servers like Jetty where the dispatch works, applications can have a chance to handle the exception. This change removes the disconnected client checks and allows dispatching again. After the change DisconnectedClientHelper is no longer needed in the 5.3.x branch. See gh-32342 --- .../async/StandardServletAsyncWebRequest.java | 8 -- .../request/async/WebAsyncManager.java | 17 ---- .../web/util/DisconnectedClientHelper.java | 98 ------------------- .../DefaultHandlerExceptionResolver.java | 25 ++++- .../ResponseEntityExceptionHandlerTests.java | 5 +- 5 files changed, 28 insertions(+), 125 deletions(-) delete mode 100644 spring-web/src/main/java/org/springframework/web/util/DisconnectedClientHelper.java diff --git a/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java index 278556c4255..c2799b2f516 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java @@ -34,7 +34,6 @@ import javax.servlet.http.HttpServletResponseWrapper; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.web.context.request.ServletWebRequest; -import org.springframework.web.util.DisconnectedClientHelper; /** * A Servlet implementation of {@link AsyncWebRequest}. @@ -184,13 +183,6 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements transitionToErrorState(); Throwable ex = event.getThrowable(); this.exceptionHandlers.forEach(consumer -> consumer.accept(ex)); - - // We skip ASYNC dispatches for "disconnected client" errors, - // but can only complete from a Servlet container thread - - if (DisconnectedClientHelper.isClientDisconnectedException(ex) && this.state != State.COMPLETED) { - this.asyncContext.complete(); - } } finally { this.stateLock.unlock(); diff --git a/spring-web/src/main/java/org/springframework/web/context/request/async/WebAsyncManager.java b/spring-web/src/main/java/org/springframework/web/context/request/async/WebAsyncManager.java index 159e06da33b..acd1280ce41 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/async/WebAsyncManager.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/async/WebAsyncManager.java @@ -36,7 +36,6 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.web.context.request.RequestAttributes; import org.springframework.web.context.request.async.DeferredResult.DeferredResultHandler; -import org.springframework.web.util.DisconnectedClientHelper; /** * The central class for managing asynchronous request processing, mainly intended @@ -70,16 +69,6 @@ public final class WebAsyncManager { private static final Log logger = LogFactory.getLog(WebAsyncManager.class); - /** - * Log category to use for network failure after a client has gone away. - * @see DisconnectedClientHelper - */ - private static final String DISCONNECTED_CLIENT_LOG_CATEGORY = - "org.springframework.web.server.DisconnectedClient"; - - private static final DisconnectedClientHelper disconnectedClientHelper = - new DisconnectedClientHelper(DISCONNECTED_CLIENT_LOG_CATEGORY); - private static final CallableProcessingInterceptor timeoutCallableInterceptor = new TimeoutCallableProcessingInterceptor(); @@ -418,12 +407,6 @@ public final class WebAsyncManager { logger.debug("Async result set to: " + result + " for " + formatUri(this.asyncWebRequest)); } - if (result instanceof Exception) { - if (disconnectedClientHelper.checkAndLogClientDisconnectedException((Exception) result)) { - return; - } - } - if (this.asyncWebRequest.isAsyncComplete()) { if (logger.isDebugEnabled()) { logger.debug("Async request already completed for " + formatUri(this.asyncWebRequest)); diff --git a/spring-web/src/main/java/org/springframework/web/util/DisconnectedClientHelper.java b/spring-web/src/main/java/org/springframework/web/util/DisconnectedClientHelper.java deleted file mode 100644 index f3d2197b114..00000000000 --- a/spring-web/src/main/java/org/springframework/web/util/DisconnectedClientHelper.java +++ /dev/null @@ -1,98 +0,0 @@ -/* - * Copyright 2002-2024 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.web.util; - -import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; - -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; - -import org.springframework.core.NestedExceptionUtils; -import org.springframework.util.Assert; - -/** - * Utility methods to assist with identifying and logging exceptions that indicate - * the client has gone away. Such exceptions fill logs with unnecessary stack - * traces. The utility methods help to log a single line message at DEBUG level, - * and a full stacktrace at TRACE level. - * - * @author Rossen Stoyanchev - * @since 5.3.33 - */ -public class DisconnectedClientHelper { - - private static final Set EXCEPTION_PHRASES = - new HashSet<>(Arrays.asList("broken pipe", "connection reset by peer")); - - private static final Set EXCEPTION_TYPE_NAMES = - new HashSet<>(Arrays.asList("AbortedException", "ClientAbortException", - "EOFException", "EofException", "AsyncRequestNotUsableException")); - - private final Log logger; - - - public DisconnectedClientHelper(String logCategory) { - Assert.notNull(logCategory, "'logCategory' is required"); - this.logger = LogFactory.getLog(logCategory); - } - - - /** - * Check via {@link #isClientDisconnectedException} if the exception - * indicates the remote client disconnected, and if so log a single line - * message when DEBUG is on, and a full stacktrace when TRACE is on for - * the configured logger. - */ - public boolean checkAndLogClientDisconnectedException(Throwable ex) { - if (isClientDisconnectedException(ex)) { - if (logger.isTraceEnabled()) { - logger.trace("Looks like the client has gone away", ex); - } - else if (logger.isDebugEnabled()) { - logger.debug("Looks like the client has gone away: " + ex + - " (For a full stack trace, set the log category '" + logger + "' to TRACE level.)"); - } - return true; - } - return false; - } - - /** - * Whether the given exception indicates the client has gone away. - * Known cases covered: - *
    - *
  • ClientAbortException or EOFException for Tomcat - *
  • EofException for Jetty - *
  • IOException "Broken pipe" or "connection reset by peer" - *
- */ - public static boolean isClientDisconnectedException(Throwable ex) { - String message = NestedExceptionUtils.getMostSpecificCause(ex).getMessage(); - if (message != null) { - String text = message.toLowerCase(); - for (String phrase : EXCEPTION_PHRASES) { - if (text.contains(phrase)) { - return true; - } - } - } - return EXCEPTION_TYPE_NAMES.contains(ex.getClass().getSimpleName()); - } - -} diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java index 72f539b8335..54dd293a887 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -46,6 +46,7 @@ import org.springframework.web.bind.ServletRequestBindingException; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestPart; +import org.springframework.web.context.request.async.AsyncRequestNotUsableException; import org.springframework.web.context.request.async.AsyncRequestTimeoutException; import org.springframework.web.multipart.MultipartFile; import org.springframework.web.multipart.support.MissingServletRequestPartException; @@ -228,6 +229,10 @@ public class DefaultHandlerExceptionResolver extends AbstractHandlerExceptionRes return handleAsyncRequestTimeoutException( (AsyncRequestTimeoutException) ex, request, response, handler); } + else if (ex instanceof AsyncRequestNotUsableException) { + return handleAsyncRequestNotUsableException( + (AsyncRequestNotUsableException) ex, request, response, handler); + } } catch (Exception handlerEx) { if (logger.isWarnEnabled()) { @@ -541,6 +546,24 @@ public class DefaultHandlerExceptionResolver extends AbstractHandlerExceptionRes return new ModelAndView(); } + /** + * Handle the case of an I/O failure from the ServletOutputStream. + *

By default, do nothing since the response is not usable. + * @param ex the {@link AsyncRequestTimeoutException} to be handled + * @param request current HTTP request + * @param response current HTTP response + * @param handler the executed handler, or {@code null} if none chosen + * at the time of the exception (for example, if multipart resolution failed) + * @return an empty ModelAndView indicating the exception was handled + * @throws IOException potentially thrown from {@link HttpServletResponse#sendError} + * @since 5.3.33 + */ + protected ModelAndView handleAsyncRequestNotUsableException(AsyncRequestNotUsableException ex, + HttpServletRequest request, HttpServletResponse response, @Nullable Object handler) { + + return new ModelAndView(); + } + /** * Invoked to send a server error. Sets the status to 500 and also sets the * request attribute "javax.servlet.error.exception" to the Exception. diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java index e5fa5c68473..29d2a54f65c 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -92,6 +92,9 @@ public class ResponseEntityExceptionHandlerTests { Class[] paramTypes = method.getParameterTypes(); if (method.getName().startsWith("handle") && (paramTypes.length == 4)) { String name = paramTypes[0].getSimpleName(); + if (name.equals("AsyncRequestNotUsableException")) { + continue; + } assertThat(exceptionTypes.contains(paramTypes[0])).as("@ExceptionHandler is missing " + name).isTrue(); } }