Set response to 503 for async requests that time out

Issue: SPR-10002

spring-web/src/main/java/org/springframework/web/context/request/async/CallableInterceptorChain.java

@@ -15,8 +15,6 @@
  */
 package org.springframework.web.context.request.async;
 
-import java.util.ArrayList;
-import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.Callable;
 
@@ -63,10 +61,13 @@ class CallableInterceptorChain {
 	}
 
 	public Object triggerAfterTimeout(NativeWebRequest request, Callable<?> task) {
-		for (int i = this.interceptors.size()-1; i >= 0; i--) {
+		for (CallableProcessingInterceptor interceptor : this.interceptors) {
 			try {
-				Object result = this.interceptors.get(i).afterTimeout(request, task);
+				Object result = interceptor.handleTimeout(request, task);
-				if (result != CallableProcessingInterceptor.RESULT_NONE) {
+				if (result == CallableProcessingInterceptor.RESPONSE_HANDLED) {
+					break;
+				}
+				else if (result != CallableProcessingInterceptor.RESULT_NONE) {
 					return result;
 				}
 			}

spring-web/src/main/java/org/springframework/web/context/request/async/CallableProcessingInterceptor.java

@@ -35,7 +35,7 @@ import org.springframework.web.context.request.NativeWebRequest;
  * the Exception instance as the concurrent result. Such exceptions will then
  * be processed through the {@code HandlerExceptionResolver} mechanism.
  *
- * <p>The {@link #afterTimeout(NativeWebRequest, Callable) afterTimeout} method
+ * <p>The {@link #handleTimeout(NativeWebRequest, Callable) afterTimeout} method
  * can select a value to be used to resume processing.
  *
  * @author Rossen Stoyanchev
@@ -43,7 +43,10 @@ import org.springframework.web.context.request.NativeWebRequest;
  */
 public interface CallableProcessingInterceptor {
 
-	public static final Object RESULT_NONE = new Object();
+	static final Object RESULT_NONE = new Object();
+
+	static final Object RESPONSE_HANDLED = new Object();
+
 
 	/**
 	 * Invoked <em>after</em> the start of concurrent handling in the async
@@ -79,11 +82,11 @@ public interface CallableProcessingInterceptor {
 	 * @param request the current request
 	 * @param task the task for the current async request
 	 * @return a concurrent result value; if the value is anything other than
-	 * {@link #RESULT_NONE}, concurrent processing is resumed and subsequent
+	 * {@link #RESULT_NONE} or {@link #RESPONSE_HANDLED}, concurrent processing
-	 * interceptors are not invoked
+	 * is resumed and subsequent interceptors are not invoked
 	 * @throws Exception in case of errors
 	 */
-	<T> Object afterTimeout(NativeWebRequest request, Callable<T> task) throws Exception;
+	<T> Object handleTimeout(NativeWebRequest request, Callable<T> task) throws Exception;
 
 	/**
 	 * Invoked from a container thread when async processing completes for any

spring-web/src/main/java/org/springframework/web/context/request/async/CallableProcessingInterceptorAdapter.java

@@ -44,7 +44,7 @@ public abstract class CallableProcessingInterceptorAdapter implements CallablePr
 	 * This implementation always returns
 	 * {@link CallableProcessingInterceptor#RESULT_NONE RESULT_NONE}.
 	 */
-	public <T> Object afterTimeout(NativeWebRequest request, Callable<T> task) throws Exception {
+	public <T> Object handleTimeout(NativeWebRequest request, Callable<T> task) throws Exception {
 		return RESULT_NONE;
 	}
 

spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResult.java

@@ -183,18 +183,19 @@ public final class DeferredResult<T> {
 		return new DeferredResultProcessingInterceptorAdapter() {
 
 			@Override
-			public <S> void afterTimeout(NativeWebRequest request, DeferredResult<S> deferredResult) {
+			public <S> boolean handleTimeout(NativeWebRequest request, DeferredResult<S> deferredResult) {
 				if (timeoutCallback != null) {
 					timeoutCallback.run();
 				}
 				if (DeferredResult.this.timeoutResult != RESULT_NONE) {
 					setResultInternal(timeoutResult);
 				}
+				return true;
 			}
 
 			@Override
 			public <S> void afterCompletion(NativeWebRequest request, DeferredResult<S> deferredResult) {
-				synchronized (this) {
+				synchronized (DeferredResult.this) {
 					expired = true;
 				}
 				if (completionCallback != null) {

spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResultInterceptorChain.java

@@ -60,11 +60,13 @@ class DeferredResultInterceptorChain {
 	}
 
 	public void triggerAfterTimeout(NativeWebRequest request, DeferredResult<?> deferredResult) throws Exception {
-		for (int i = this.preProcessingIndex; i >= 0; i--) {
+		for (DeferredResultProcessingInterceptor interceptor : this.interceptors) {
 			if (deferredResult.isSetOrExpired()) {
 				return;
 			}
-			this.interceptors.get(i).afterTimeout(request, deferredResult);
+			if (!interceptor.handleTimeout(request, deferredResult)){
+				break;
+			}
 		}
 	}
 

spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResultProcessingInterceptor.java

@@ -32,7 +32,7 @@ import org.springframework.web.context.request.NativeWebRequest;
  * the Exception instance as the concurrent result. Such exceptions will then
  * be processed through the {@code HandlerExceptionResolver} mechanism.
  *
- * <p>The {@link #afterTimeout(NativeWebRequest, DeferredResult) afterTimeout}
+ * <p>The {@link #handleTimeout(NativeWebRequest, DeferredResult) afterTimeout}
  * method can set the {@code DeferredResult} in order to resume processing.
  *
  * @author Rossen Stoyanchev
@@ -81,9 +81,11 @@ public interface DeferredResultProcessingInterceptor {
 	 * @param deferredResult the DeferredResult for the current request; if the
 	 * {@code DeferredResult} is set, then concurrent processing is resumed and
 	 * subsequent interceptors are not invoked
+	 * @return {@code true} if processing should continue, or {@code false} if
+	 * other interceptors should not be invoked
 	 * @throws Exception in case of errors
 	 */
-	<T> void afterTimeout(NativeWebRequest request, DeferredResult<T> deferredResult) throws Exception;
+	<T> boolean handleTimeout(NativeWebRequest request, DeferredResult<T> deferredResult) throws Exception;
 
 	/**
 	 * Invoked from a container thread when an async request completed for any

spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResultProcessingInterceptorAdapter.java

@@ -40,9 +40,10 @@ public abstract class DeferredResultProcessingInterceptorAdapter implements Defe
 	}
 
 	/**
-	 * This implementation is empty.
+	 * This implementation returns {@code true} by default.
 	 */
-	public <T> void afterTimeout(NativeWebRequest request, DeferredResult<T> deferredResult) throws Exception {
+	public <T> boolean handleTimeout(NativeWebRequest request, DeferredResult<T> deferredResult) throws Exception {
+		return true;
 	}
 
 	/**

spring-web/src/main/java/org/springframework/web/context/request/async/MvcAsyncTask.java

@@ -156,7 +156,7 @@ public class MvcAsyncTask<V> {
 		return new CallableProcessingInterceptorAdapter() {
 
 			@Override
-			public <T> Object afterTimeout(NativeWebRequest request, Callable<T> task) throws Exception {
+			public <T> Object handleTimeout(NativeWebRequest request, Callable<T> task) throws Exception {
 				return (timeoutCallback != null) ? timeoutCallback.call() : CallableProcessingInterceptor.RESULT_NONE;
 			}
 

spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutCallableProcessingInterceptor.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright 2002-2012 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;
+
+import java.util.concurrent.Callable;
+
+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
+ * therefore invoked only if no other interceptor handles the timeout.
+ *
+ * <p>Note that according to RFC 2616, a 503 without a 'Retry-After' header is
+ * interpreted as a 500 error and the client should not retry. Applications
+ * can install their own interceptor to handle a timeout and add a 'Retry-After'
+ * header if necessary.
+ *
+ * @author Rossen Stoyanchev
+ * @since 3.2
+ */
+public class TimeoutCallableProcessingInterceptor extends CallableProcessingInterceptorAdapter {
+
+	@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;
+	}
+
+}

spring-web/src/main/java/org/springframework/web/context/request/async/TimeoutDeferredResultProcessingInterceptor.java

@@ -0,0 +1,47 @@
+/*
+ * Copyright 2002-2012 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;
+
+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
+ * therefore invoked only if no other interceptor handles the timeout.
+ *
+ * <p>Note that according to RFC 2616, a 503 without a 'Retry-After' header is
+ * interpreted as a 500 error and the client should not retry. Applications
+ * can install their own interceptor to handle a timeout and add a 'Retry-After'
+ * header if necessary.
+ *
+ * @author Rossen Stoyanchev
+ * @since 3.2
+ */
+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());
+		}
+		return false;
+	}
+
+}

spring-web/src/main/java/org/springframework/web/context/request/async/WebAsyncManager.java

@@ -63,6 +63,12 @@ public final class WebAsyncManager {
 
 	private static final UrlPathHelper urlPathHelper = new UrlPathHelper();
 
+	private static final CallableProcessingInterceptor timeoutCallableInterceptor =
+			new TimeoutCallableProcessingInterceptor();
+
+	private static final DeferredResultProcessingInterceptor timeoutDeferredResultInterceptor =
+			new TimeoutDeferredResultProcessingInterceptor();
+
 
 	private AsyncWebRequest asyncWebRequest;
 
@@ -266,8 +272,6 @@ public final class WebAsyncManager {
 		Assert.notNull(mvcAsyncTask, "MvcAsyncTask must not be null");
 		Assert.state(this.asyncWebRequest != null, "AsyncWebRequest must not be null");
 
-		final Callable<?> callable = mvcAsyncTask.getCallable();
-
 		Long timeout = mvcAsyncTask.getTimeout();
 		if (timeout != null) {
 			this.asyncWebRequest.setTimeout(timeout);
@@ -281,7 +285,9 @@ public final class WebAsyncManager {
 		List<CallableProcessingInterceptor> interceptors = new ArrayList<CallableProcessingInterceptor>();
 		interceptors.add(mvcAsyncTask.getInterceptor());
 		interceptors.addAll(this.callableInterceptors.values());
+		interceptors.add(timeoutCallableInterceptor);
 
+		final Callable<?> callable = mvcAsyncTask.getCallable();
 		final CallableInterceptorChain interceptorChain = new CallableInterceptorChain(interceptors);
 
 		this.asyncWebRequest.addTimeoutHandler(new Runnable() {
@@ -368,6 +374,7 @@ public final class WebAsyncManager {
 		List<DeferredResultProcessingInterceptor> interceptors = new ArrayList<DeferredResultProcessingInterceptor>();
 		interceptors.add(deferredResult.getInterceptor());
 		interceptors.addAll(this.deferredResultInterceptors.values());
+		interceptors.add(timeoutDeferredResultInterceptor);
 
 		final DeferredResultInterceptorChain interceptorChain = new DeferredResultInterceptorChain(interceptors);
 

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

@@ -113,7 +113,7 @@ public class DeferredResultTests {
 			}
 		});
 
-		result.getInterceptor().afterTimeout(null, null);
+		result.getInterceptor().handleTimeout(null, null);
 
 		assertEquals("timeout event", sb.toString());
 		assertFalse("Should not be able to set result a second time", result.setResult("hello"));

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

@@ -56,12 +56,14 @@ public class WebAsyncManagerTimeoutTests {
 
 	private MockHttpServletRequest servletRequest;
 
+	private MockHttpServletResponse servletResponse;
 
 	@Before
 	public void setUp() {
 		this.servletRequest = new MockHttpServletRequest("GET", "/test");
 		this.servletRequest.setAsyncSupported(true);
-		this.asyncWebRequest = new StandardServletAsyncWebRequest(servletRequest, new MockHttpServletResponse());
+		this.servletResponse = new MockHttpServletResponse();
+		this.asyncWebRequest = new StandardServletAsyncWebRequest(servletRequest, servletResponse);
 
 		AsyncTaskExecutor executor = createMock(AsyncTaskExecutor.class);
 		expect(executor.submit((Runnable) notNull())).andReturn(null);
@@ -78,7 +80,7 @@ public class WebAsyncManagerTimeoutTests {
 		StubCallable callable = new StubCallable();
 
 		CallableProcessingInterceptor interceptor = createStrictMock(CallableProcessingInterceptor.class);
-		expect(interceptor.afterTimeout(this.asyncWebRequest, callable)).andReturn(RESULT_NONE);
+		expect(interceptor.handleTimeout(this.asyncWebRequest, callable)).andReturn(RESULT_NONE);
 		interceptor.afterCompletion(this.asyncWebRequest, callable);
 		replay(interceptor);
 
@@ -90,6 +92,7 @@ public class WebAsyncManagerTimeoutTests {
 
 		assertFalse(this.asyncManager.hasConcurrentResult());
 		assertEquals(DispatcherType.REQUEST, this.servletRequest.getDispatcherType());
+		assertEquals(503, this.servletResponse.getStatus());
 
 		verify(interceptor);
 	}
@@ -120,7 +123,7 @@ public class WebAsyncManagerTimeoutTests {
 		StubCallable callable = new StubCallable();
 
 		CallableProcessingInterceptor interceptor = createStrictMock(CallableProcessingInterceptor.class);
-		expect(interceptor.afterTimeout(this.asyncWebRequest, callable)).andReturn(22);
+		expect(interceptor.handleTimeout(this.asyncWebRequest, callable)).andReturn(22);
 		replay(interceptor);
 
 		this.asyncManager.registerCallableInterceptor("timeoutInterceptor", interceptor);
@@ -142,7 +145,7 @@ public class WebAsyncManagerTimeoutTests {
 		Exception exception = new Exception();
 
 		CallableProcessingInterceptor interceptor = createStrictMock(CallableProcessingInterceptor.class);
-		expect(interceptor.afterTimeout(this.asyncWebRequest, callable)).andThrow(exception);
+		expect(interceptor.handleTimeout(this.asyncWebRequest, callable)).andThrow(exception);
 		replay(interceptor);
 
 		this.asyncManager.registerCallableInterceptor("timeoutInterceptor", interceptor);
@@ -164,7 +167,7 @@ public class WebAsyncManagerTimeoutTests {
 
 		DeferredResultProcessingInterceptor interceptor = createStrictMock(DeferredResultProcessingInterceptor.class);
 		interceptor.preProcess(this.asyncWebRequest, deferredResult);
-		interceptor.afterTimeout(this.asyncWebRequest, deferredResult);
+		expect(interceptor.handleTimeout(this.asyncWebRequest, deferredResult)).andReturn(true);
 		interceptor.afterCompletion(this.asyncWebRequest, deferredResult);
 		replay(interceptor);
 
@@ -176,6 +179,7 @@ public class WebAsyncManagerTimeoutTests {
 
 		assertFalse(this.asyncManager.hasConcurrentResult());
 		assertEquals(DispatcherType.REQUEST, this.servletRequest.getDispatcherType());
+		assertEquals(503, this.servletResponse.getStatus());
 
 		verify(interceptor);
 	}
@@ -220,8 +224,9 @@ public class WebAsyncManagerTimeoutTests {
 		DeferredResult<Integer> deferredResult = new DeferredResult<Integer>();
 
 		DeferredResultProcessingInterceptor interceptor = new DeferredResultProcessingInterceptorAdapter() {
-			public <T> void afterTimeout(NativeWebRequest request, DeferredResult<T> result) throws Exception {
+			public <T> boolean handleTimeout(NativeWebRequest request, DeferredResult<T> result) throws Exception {
 				result.setErrorResult(23);
+				return true;
 			}
 		};
 
@@ -243,7 +248,7 @@ public class WebAsyncManagerTimeoutTests {
 		final Exception exception = new Exception();
 
 		DeferredResultProcessingInterceptor interceptor = new DeferredResultProcessingInterceptorAdapter() {
-			public <T> void afterTimeout(NativeWebRequest request, DeferredResult<T> result) throws Exception {
+			public <T> boolean handleTimeout(NativeWebRequest request, DeferredResult<T> result) throws Exception {
 				throw exception;
 			}
 		};