From 765b47246a4893a4855f4e8e26ad41de491fa175 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 14 Sep 2016 21:34:30 -0400 Subject: [PATCH] Improve async request timeout handling Rather than setting the status to 503 directly from the timeout interceptor which no longer seems to work reliably with Servlet containers like Jetty even performing an additional ERROR dispatch back to the original URL, we know rather set the DeferredResult to an AsyncTimeoutException, which results in a dispatch and standard handling within Spring MVC. This should be a more reliable way of dealing with timeouts. Issue: SPR-14669 --- .../async/AsyncRequestTimeoutException.java | 34 +++++++++++++++++++ .../TimeoutCallableProcessingInterceptor.java | 14 ++++---- ...utDeferredResultProcessingInterceptor.java | 18 +++++----- .../async/WebAsyncManagerTimeoutTests.java | 20 +++++------ .../ResponseEntityExceptionHandler.java | 25 +++++++++++++- .../DefaultHandlerExceptionResolver.java | 24 +++++++++++++ .../ResponseEntityExceptionHandlerTests.java | 8 ++++- .../DefaultHandlerExceptionResolverTests.java | 10 ++++++ 8 files changed, 124 insertions(+), 29 deletions(-) create mode 100644 spring-web/src/main/java/org/springframework/web/context/request/async/AsyncRequestTimeoutException.java diff --git a/spring-web/src/main/java/org/springframework/web/context/request/async/AsyncRequestTimeoutException.java b/spring-web/src/main/java/org/springframework/web/context/request/async/AsyncRequestTimeoutException.java new file mode 100644 index 00000000000..f079caf53b0 --- /dev/null +++ b/spring-web/src/main/java/org/springframework/web/context/request/async/AsyncRequestTimeoutException.java @@ -0,0 +1,34 @@ +/* + * Copyright 2002-2016 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 + * + * http://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.context.request.async; + +/** + * Exception to be thrown when an async request times out. + * Alternatively an applications can register a + * {@link DeferredResultProcessingInterceptor} or a + * {@link CallableProcessingInterceptor} to handle the timeout through + * the MVC Java config or the MVC XML namespace or directly through properties + * of the {@code RequestMappingHandlerAdapter}. + * + *

By default the exception will be handled as a 503 error. + * + * @author Rossen Stoyanchev + * @since 4.2.8 + */ +@SuppressWarnings("serial") +public class AsyncRequestTimeoutException extends Exception { + +} diff --git a/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutCallableProcessingInterceptor.java b/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutCallableProcessingInterceptor.java index a336d64513f..c26e476bb9d 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutCallableProcessingInterceptor.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutCallableProcessingInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2016 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. @@ -24,7 +24,11 @@ import org.springframework.web.context.request.NativeWebRequest; /** * Sends a 503 (SERVICE_UNAVAILABLE) in case of a timeout if the response is not - * already committed. Registered at the end, after all other interceptors and + * already committed. As of 4.2.8 this is done indirectly by setting the result + * to an {@link AsyncRequestTimeoutException} which is then handled by + * Spring MVC's default exception handling as a 503 error. + * + *

Registered at the end, after all other interceptors and * therefore invoked only if no other interceptor handles the timeout. * *

Note that according to RFC 7231, a 503 without a 'Retry-After' header is @@ -39,11 +43,7 @@ public class TimeoutCallableProcessingInterceptor extends CallableProcessingInte @Override public Object handleTimeout(NativeWebRequest request, Callable task) throws Exception { - HttpServletResponse servletResponse = request.getNativeResponse(HttpServletResponse.class); - if (!servletResponse.isCommitted()) { - servletResponse.sendError(HttpStatus.SERVICE_UNAVAILABLE.value()); - } - return CallableProcessingInterceptor.RESPONSE_HANDLED; + return new AsyncRequestTimeoutException(); } } diff --git a/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutDeferredResultProcessingInterceptor.java b/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutDeferredResultProcessingInterceptor.java index 5cb855d9497..80d1172fa51 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutDeferredResultProcessingInterceptor.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutDeferredResultProcessingInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2016 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. @@ -16,14 +16,15 @@ package org.springframework.web.context.request.async; -import javax.servlet.http.HttpServletResponse; - -import org.springframework.http.HttpStatus; import org.springframework.web.context.request.NativeWebRequest; /** * Sends a 503 (SERVICE_UNAVAILABLE) in case of a timeout if the response is not - * already committed. Registered at the end, after all other interceptors and + * already committed. As of 4.2.8 this is done indirectly by returning + * {@link AsyncRequestTimeoutException} as the result of processing which is + * then handled by Spring MVC's default exception handling as a 503 error. + * + *

Registered at the end, after all other interceptors and * therefore invoked only if no other interceptor handles the timeout. * *

Note that according to RFC 7231, a 503 without a 'Retry-After' header is @@ -37,11 +38,8 @@ import org.springframework.web.context.request.NativeWebRequest; public class TimeoutDeferredResultProcessingInterceptor extends DeferredResultProcessingInterceptorAdapter { @Override - public boolean handleTimeout(NativeWebRequest request, DeferredResult deferredResult) throws Exception { - HttpServletResponse servletResponse = request.getNativeResponse(HttpServletResponse.class); - if (!servletResponse.isCommitted()) { - servletResponse.sendError(HttpStatus.SERVICE_UNAVAILABLE.value()); - } + public boolean handleTimeout(NativeWebRequest request, DeferredResult result) throws Exception { + result.setErrorResult(new AsyncRequestTimeoutException()); return false; } diff --git a/spring-web/src/test/java/org/springframework/web/context/request/async/WebAsyncManagerTimeoutTests.java b/spring-web/src/test/java/org/springframework/web/context/request/async/WebAsyncManagerTimeoutTests.java index 30b1a13a201..b5fac91e686 100644 --- a/spring-web/src/test/java/org/springframework/web/context/request/async/WebAsyncManagerTimeoutTests.java +++ b/spring-web/src/test/java/org/springframework/web/context/request/async/WebAsyncManagerTimeoutTests.java @@ -18,7 +18,6 @@ package org.springframework.web.context.request.async; import java.util.concurrent.Callable; import javax.servlet.AsyncEvent; -import javax.servlet.DispatcherType; import org.junit.Before; import org.junit.Test; @@ -29,9 +28,12 @@ import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; import org.springframework.web.context.request.NativeWebRequest; -import static org.junit.Assert.*; -import static org.mockito.BDDMockito.*; -import static org.springframework.web.context.request.async.CallableProcessingInterceptor.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.BDDMockito.given; +import static org.mockito.BDDMockito.mock; +import static org.mockito.BDDMockito.verify; +import static org.springframework.web.context.request.async.CallableProcessingInterceptor.RESULT_NONE; /** * {@link WebAsyncManager} tests where container-triggered timeout/completion @@ -79,9 +81,8 @@ public class WebAsyncManagerTimeoutTests { this.asyncWebRequest.onTimeout(ASYNC_EVENT); this.asyncWebRequest.onComplete(ASYNC_EVENT); - assertFalse(this.asyncManager.hasConcurrentResult()); - assertEquals(DispatcherType.REQUEST, this.servletRequest.getDispatcherType()); - assertEquals(503, this.servletResponse.getStatus()); + assertTrue(this.asyncManager.hasConcurrentResult()); + assertEquals(AsyncRequestTimeoutException.class, this.asyncManager.getConcurrentResult().getClass()); verify(interceptor).beforeConcurrentHandling(this.asyncWebRequest, callable); verify(interceptor).afterCompletion(this.asyncWebRequest, callable); @@ -163,9 +164,8 @@ public class WebAsyncManagerTimeoutTests { this.asyncWebRequest.onTimeout(ASYNC_EVENT); this.asyncWebRequest.onComplete(ASYNC_EVENT); - assertFalse(this.asyncManager.hasConcurrentResult()); - assertEquals(DispatcherType.REQUEST, this.servletRequest.getDispatcherType()); - assertEquals(503, this.servletResponse.getStatus()); + assertTrue(this.asyncManager.hasConcurrentResult()); + assertEquals(AsyncRequestTimeoutException.class, this.asyncManager.getConcurrentResult().getClass()); verify(interceptor).beforeConcurrentHandling(this.asyncWebRequest, deferredResult); verify(interceptor).preProcess(this.asyncWebRequest, deferredResult); diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java index 5ad128f9ae5..5885a28aac5 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java @@ -44,6 +44,7 @@ import org.springframework.web.bind.ServletRequestBindingException; import org.springframework.web.bind.annotation.ControllerAdvice; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.context.request.WebRequest; +import org.springframework.web.context.request.async.AsyncRequestTimeoutException; import org.springframework.web.multipart.support.MissingServletRequestPartException; import org.springframework.web.servlet.NoHandlerFoundException; import org.springframework.web.util.WebUtils; @@ -112,7 +113,8 @@ public abstract class ResponseEntityExceptionHandler { MethodArgumentNotValidException.class, MissingServletRequestPartException.class, BindException.class, - NoHandlerFoundException.class + NoHandlerFoundException.class, + AsyncRequestTimeoutException.class }) public final ResponseEntity handleException(Exception ex, WebRequest request) { HttpHeaders headers = new HttpHeaders(); @@ -172,6 +174,11 @@ public abstract class ResponseEntityExceptionHandler { HttpStatus status = HttpStatus.NOT_FOUND; return handleNoHandlerFoundException((NoHandlerFoundException) ex, headers, status, request); } + else if (ex instanceof AsyncRequestTimeoutException) { + HttpStatus status = HttpStatus.SERVICE_UNAVAILABLE; + return handleAsyncRequestTimeoutException( + (AsyncRequestTimeoutException) ex, headers, status, request); + } else { logger.warn("Unknown exception type: " + ex.getClass().getName()); HttpStatus status = HttpStatus.INTERNAL_SERVER_ERROR; @@ -424,4 +431,20 @@ public abstract class ResponseEntityExceptionHandler { return handleExceptionInternal(ex, null, headers, status, request); } + /** + * Customize the response for NoHandlerFoundException. + *

This method delegates to {@link #handleExceptionInternal}. + * @param ex the exception + * @param headers the headers to be written to the response + * @param status the selected response status + * @param request the current request + * @return a {@code ResponseEntity} instance + * @since 4.2.8 + */ + protected ResponseEntity handleAsyncRequestTimeoutException( + AsyncRequestTimeoutException ex, HttpHeaders headers, HttpStatus status, WebRequest request) { + + return handleExceptionInternal(ex, null, headers, status, request); + } + } 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 ac065d583ee..eac87f285e2 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 @@ -44,6 +44,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.AsyncRequestTimeoutException; import org.springframework.web.multipart.MultipartFile; import org.springframework.web.multipart.support.MissingServletRequestPartException; import org.springframework.web.servlet.ModelAndView; @@ -153,6 +154,10 @@ public class DefaultHandlerExceptionResolver extends AbstractHandlerExceptionRes else if (ex instanceof NoHandlerFoundException) { return handleNoHandlerFoundException((NoHandlerFoundException) ex, request, response, handler); } + else if (ex instanceof AsyncRequestTimeoutException) { + return handleAsyncRequestTimeoutException( + (AsyncRequestTimeoutException) ex, request, response, handler); + } } catch (Exception handlerException) { if (logger.isWarnEnabled()) { @@ -449,6 +454,25 @@ public class DefaultHandlerExceptionResolver extends AbstractHandlerExceptionRes return new ModelAndView(); } + /** + * Handle the case where an async request timed out. + *

The default implementation sends an HTTP 503 error. + * @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 response.sendError() + * @since 4.2.8 + */ + protected ModelAndView handleAsyncRequestTimeoutException(AsyncRequestTimeoutException ex, + HttpServletRequest request, HttpServletResponse response, Object handler) throws IOException { + + response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); + return new ModelAndView(); + } + /** * Invoked to send a server error. Sets the status to 500 and also sets the 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 a698447a58a..8fe943cd9dc 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-2015 the original author or authors. + * Copyright 2002-2016 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. @@ -50,6 +50,7 @@ import org.springframework.web.bind.annotation.ControllerAdvice; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.context.request.WebRequest; +import org.springframework.web.context.request.async.AsyncRequestTimeoutException; import org.springframework.web.context.support.StaticWebApplicationContext; import org.springframework.web.multipart.support.MissingServletRequestPartException; import org.springframework.web.servlet.NoHandlerFoundException; @@ -197,6 +198,11 @@ public class ResponseEntityExceptionHandlerTests { testException(ex); } + @Test + public void asyncRequestTimeoutException() { + testException(new AsyncRequestTimeoutException()); + } + @Test public void controllerAdvice() throws Exception { StaticWebApplicationContext cxt = new StaticWebApplicationContext(); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolverTests.java index 0b4d3af29a7..c2ab52fd887 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolverTests.java @@ -40,6 +40,7 @@ import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.MissingPathVariableException; import org.springframework.web.bind.MissingServletRequestParameterException; import org.springframework.web.bind.ServletRequestBindingException; +import org.springframework.web.context.request.async.AsyncRequestTimeoutException; import org.springframework.web.multipart.support.MissingServletRequestPartException; import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.NoHandlerFoundException; @@ -206,6 +207,15 @@ public class DefaultHandlerExceptionResolverTests { assertSame(ex, request.getAttribute("javax.servlet.error.exception")); } + @Test // SPR-14669 + public void handleAsyncRequestTimeoutException() throws Exception { + Exception ex = new AsyncRequestTimeoutException(); + ModelAndView mav = exceptionResolver.resolveException(request, response, null, ex); + assertNotNull("No ModelAndView returned", mav); + assertTrue("No Empty ModelAndView returned", mav.isEmpty()); + assertEquals("Invalid status code", 503, response.getStatus()); + } + @SuppressWarnings("unused") public void handle(String arg) {