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

Author: rstoyanchev

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}.
+ *
+ * <p>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 {
+
+}

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 @@
 
 /**
  * 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.
+ *
+ * <p>Registered at the end, after all other interceptors and
  * therefore invoked only if no other interceptor handles the timeout.
  *
  * <p>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 <T> Object handleTimeout(NativeWebRequest request, Callable<T> 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();
 	}
 
 }

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.
+ *
+ * <p>Registered at the end, after all other interceptors and
  * therefore invoked only if no other interceptor handles the timeout.
  *
  * <p>Note that according to RFC 7231, a 503 without a 'Retry-After' header is
@@ -37,11 +38,8 @@
 public class TimeoutDeferredResultProcessingInterceptor extends DeferredResultProcessingInterceptorAdapter {
 
 	@Override
-	public <T> boolean handleTimeout(NativeWebRequest request, DeferredResult<T> deferredResult) throws Exception {
-		HttpServletResponse servletResponse = request.getNativeResponse(HttpServletResponse.class);
-		if (!servletResponse.isCommitted()) {
-			servletResponse.sendError(HttpStatus.SERVICE_UNAVAILABLE.value());
-		}
+	public <T> boolean handleTimeout(NativeWebRequest request, DeferredResult<T> result) throws Exception {
+		result.setErrorResult(new AsyncRequestTimeoutException());
 		return false;
 	}
 

spring-web/src/test/java/org/springframework/web/context/request/async/WebAsyncManagerTimeoutTests.java

@@ -18,7 +18,6 @@
 
 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.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 void startCallableProcessingTimeoutAndComplete() throws Exception {
 		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 void startDeferredResultProcessingTimeoutAndComplete() throws Exception {
 		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);

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java

@@ -44,6 +44,7 @@
 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;
@@ -114,7 +115,8 @@ public abstract class ResponseEntityExceptionHandler {
 			MethodArgumentNotValidException.class,
 			MissingServletRequestPartException.class,
 			BindException.class,
-			NoHandlerFoundException.class
+			NoHandlerFoundException.class,
+			AsyncRequestTimeoutException.class
 		})
 	public final ResponseEntity<Object> handleException(Exception ex, WebRequest request) {
 		HttpHeaders headers = new HttpHeaders();
@@ -178,6 +180,11 @@ else if (ex instanceof NoHandlerFoundException) {
 			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;
@@ -449,4 +456,20 @@ protected ResponseEntity<Object> handleNoHandlerFoundException(
 		return handleExceptionInternal(ex, null, headers, status, request);
 	}
 
+	/**
+	 * Customize the response for NoHandlerFoundException.
+	 * <p>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<Object> handleAsyncRequestTimeoutException(
+			AsyncRequestTimeoutException ex, HttpHeaders headers, HttpStatus status, WebRequest request) {
+
+		return handleExceptionInternal(ex, null, headers, status, request);
+	}
+
 }

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java

@@ -44,6 +44,7 @@
 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;
@@ -159,6 +160,10 @@ else if (ex instanceof BindException) {
 			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()) {
@@ -478,6 +483,25 @@ protected ModelAndView handleNoHandlerFoundException(NoHandlerFoundException ex,
 		return new ModelAndView();
 	}
 
+	/**
+	 * Handle the case where an async request timed out.
+	 * <p>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

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.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;
@@ -205,6 +206,11 @@ public void noHandlerFoundException() {
 		testException(ex);
 	}
 
+	@Test
+	public void asyncRequestTimeoutException() {
+		testException(new AsyncRequestTimeoutException());
+	}
+
 	@Test
 	public void controllerAdvice() throws Exception {
 		StaticWebApplicationContext cxt = new StaticWebApplicationContext();

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolverTests.java

@@ -40,6 +40,7 @@
 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;
@@ -216,6 +217,15 @@ public void handleConversionNotSupportedException() throws Exception {
 		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) {