Skip to content

Commit f37efb4

Browse files
committed
Polish Servlet async request processing
* Clarify semantics and behavior of AsyncWebRequest methods in most cases making a best effort and not raising an exception if async processing has completed for example due to a timeout. The startAsync() method is still protected with various checks and will raise ISE under a number of conditions. * Return 503 (service unavailable) when requests time out. * Logging improvements. Issue: SPR-8517
1 parent 699de7e commit f37efb4

File tree

7 files changed

+75
-67
lines changed

7 files changed

+75
-67
lines changed

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
*/
3636
public class AsyncExecutionChainRunnable implements Runnable {
3737

38-
private static final Log logger = LogFactory.getLog(AsyncWebRequest.class);
38+
private static final Log logger = LogFactory.getLog(AsyncExecutionChainRunnable.class);
3939

4040
private final AsyncWebRequest asyncWebRequest;
4141

@@ -62,8 +62,7 @@ public AsyncExecutionChainRunnable(AsyncWebRequest asyncWebRequest, Callable<?>
6262
*/
6363
public void run() {
6464
try {
65-
logger.debug("Starting async execution chain");
66-
callable.call();
65+
this.callable.call();
6766
}
6867
catch (StaleAsyncWebRequestException ex) {
6968
logger.trace("Could not complete async request", ex);
@@ -73,8 +72,8 @@ public void run() {
7372
this.asyncWebRequest.sendError(HttpStatus.INTERNAL_SERVER_ERROR, ex.getMessage());
7473
}
7574
finally {
76-
logger.debug("Exiting async execution chain");
77-
asyncWebRequest.complete();
75+
logger.debug("Completing async request processing");
76+
this.asyncWebRequest.complete();
7877
}
7978
}
8079

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

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,54 +21,52 @@
2121

2222

2323
/**
24-
* Extends {@link NativeWebRequest} with methods for starting, completing, and
25-
* configuring async request processing. Abstract underlying mechanisms such as
26-
* the Servlet 3.0 AsyncContext.
24+
* Extend {@link NativeWebRequest} with methods for async request processing.
2725
*
2826
* @author Rossen Stoyanchev
2927
* @since 3.2
3028
*/
3129
public interface AsyncWebRequest extends NativeWebRequest {
3230

3331
/**
34-
* Set the timeout for asynchronous request processing. When the timeout
35-
* begins depends on the underlying technology. With the Servlet 3 async
36-
* support the timeout begins after the main processing thread has exited
37-
* and has been returned to the container pool.
32+
* Set the timeout for asynchronous request processing in milliseconds.
33+
* In Servlet 3 async request processing, the timeout begins when the
34+
* main processing thread has exited.
3835
*/
3936
void setTimeout(Long timeout);
4037

4138
/**
42-
* Marks the start of async request processing for example. Ensures the
43-
* request remains open to be completed in a separate thread.
39+
* Mark the start of async request processing for example ensuring the
40+
* request remains open in order to be completed in a separate thread.
41+
* @throws IllegalStateException if async processing has started, if it is
42+
* not supported, or if it has completed.
4443
*/
4544
void startAsync();
4645

4746
/**
48-
* Return {@code true} if async processing has started following a call to
49-
* {@link #startAsync()} and before it has completed.
47+
* Whether async processing is in progress and has not yet completed.
5048
*/
5149
boolean isAsyncStarted();
5250

5351
/**
54-
* Complete async request processing finalizing the underlying request.
52+
* Complete async request processing making a best effort but without any
53+
* effect if async request processing has already completed for any reason
54+
* including a timeout.
5555
*/
5656
void complete();
5757

5858
/**
59-
* Send an error to the client.
59+
* Whether async processing has completed either normally via calls to
60+
* {@link #complete()} or for other reasons such as a timeout likely
61+
* detected in a separate thread during async request processing.
6062
*/
61-
void sendError(HttpStatus status, String message);
63+
boolean isAsyncCompleted();
6264

6365
/**
64-
* Return {@code true} if async processing completed either normally or for
65-
* any other reason such as a timeout or an error. Note that a timeout or a
66-
* (client) error may occur in a separate thread while async processing is
67-
* still in progress in its own thread. Hence the underlying async request
68-
* may become stale and this method may return {@code true} even if
69-
* {@link #complete()} was never actually called.
70-
* @see StaleAsyncWebRequestException
66+
* Send an error to the client making a best effort to do so but without any
67+
* effect if async request processing has already completed, for example due
68+
* to a timeout.
7169
*/
72-
boolean isAsyncCompleted();
70+
void sendError(HttpStatus status, String message);
7371

7472
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public Object call() throws Exception {
4242
Object result = getNextCallable().call();
4343
if (this.asyncWebRequest.isAsyncCompleted()) {
4444
throw new StaleAsyncWebRequestException(
45-
"Async request no longer available due to a timed out or a (client) error");
45+
"Async request no longer available due to a timeout or a (client) error");
4646
}
4747
return result;
4848
}

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

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,10 @@
3232
/**
3333
* A Servlet 3.0 implementation of {@link AsyncWebRequest}.
3434
*
35-
* <p>The servlet processing an async request as well as all filters involved
36-
* must async support enabled. This can be done in Java using the Servlet API
37-
* or by adding an {@code <async-support>true</async-support>} element to
38-
* servlet and filter declarations in web.xml
35+
* <p>The servlet and all filters involved in an async request must have async
36+
* support enabled using the Servlet API or by adding an
37+
* {@code <async-support>true</async-support>} element to servlet and filter
38+
* declarations in web.xml
3939
*
4040
* @author Rossen Stoyanchev
4141
* @since 3.2
@@ -57,7 +57,6 @@ public void setTimeout(Long timeout) {
5757
}
5858

5959
public boolean isAsyncStarted() {
60-
assertNotStale();
6160
return ((this.asyncContext != null) && getRequest().isAsyncStarted());
6261
}
6362

@@ -81,12 +80,17 @@ public void startAsync() {
8180
}
8281

8382
public void complete() {
84-
assertNotStale();
8583
if (!isAsyncCompleted()) {
8684
this.asyncContext.complete();
85+
completeInternal();
8786
}
8887
}
8988

89+
private void completeInternal() {
90+
this.asyncContext = null;
91+
this.asyncCompleted.set(true);
92+
}
93+
9094
public void sendError(HttpStatus status, String message) {
9195
try {
9296
if (!isAsyncCompleted()) {
@@ -107,18 +111,19 @@ private void assertNotStale() {
107111
// ---------------------------------------------------------------------
108112

109113
public void onTimeout(AsyncEvent event) throws IOException {
110-
this.asyncCompleted.set(true);
114+
completeInternal();
115+
getResponse().sendError(HttpStatus.SERVICE_UNAVAILABLE.value());
111116
}
112117

113118
public void onError(AsyncEvent event) throws IOException {
114-
this.asyncCompleted.set(true);
119+
completeInternal();
115120
}
116121

117122
public void onStartAsync(AsyncEvent event) throws IOException {
118123
}
119124

120125
public void onComplete(AsyncEvent event) throws IOException {
121-
this.asyncCompleted.set(true);
126+
completeInternal();
122127
}
123128

124129
}

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

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,15 @@
1616

1717
package org.springframework.web.context.request.async;
1818

19+
1920
import static org.easymock.EasyMock.expect;
2021
import static org.easymock.EasyMock.replay;
2122
import static org.easymock.EasyMock.reset;
2223
import static org.easymock.EasyMock.verify;
24+
import static org.hamcrest.Matchers.containsString;
2325
import static org.junit.Assert.assertEquals;
26+
import static org.junit.Assert.assertFalse;
27+
import static org.junit.Assert.assertThat;
2428
import static org.junit.Assert.assertTrue;
2529
import static org.junit.Assert.fail;
2630

@@ -57,33 +61,29 @@ public void setup() {
5761

5862
@Test
5963
public void isAsyncStarted() throws Exception {
60-
assertEquals(false, this.asyncRequest.isAsyncStarted());
64+
replay(this.request);
65+
assertEquals("Should be \"false\" before startAsync()", false, this.asyncRequest.isAsyncStarted());
66+
verify(this.request);
6167

6268
startAsync();
6369

6470
reset(this.request);
6571
expect(this.request.isAsyncStarted()).andReturn(true);
6672
replay(this.request);
6773

68-
assertTrue(this.asyncRequest.isAsyncStarted());
69-
}
74+
assertTrue("Should be \"true\" true startAsync()", this.asyncRequest.isAsyncStarted());
75+
verify(this.request);
7076

71-
@Test
72-
public void isAsyncStarted_stale() throws Exception {
7377
this.asyncRequest.onComplete(new AsyncEvent(null));
74-
try {
75-
this.asyncRequest.isAsyncStarted();
76-
fail("expected exception");
77-
}
78-
catch (IllegalStateException ex) {
79-
assertStaleRequestMessage(ex);
80-
}
78+
79+
assertFalse("Should be \"false\" after complete()", this.asyncRequest.isAsyncStarted());
8180
}
8281

8382
@Test
8483
public void startAsync() throws Exception {
8584
AsyncContext asyncContext = EasyMock.createMock(AsyncContext.class);
8685

86+
reset(this.request);
8787
expect(this.request.isAsyncSupported()).andReturn(true);
8888
expect(this.request.startAsync(this.request, this.response)).andStubReturn(asyncContext);
8989
replay(this.request);
@@ -97,6 +97,19 @@ public void startAsync() throws Exception {
9797
verify(this.request);
9898
}
9999

100+
@Test
101+
public void startAsync_notSupported() throws Exception {
102+
expect(this.request.isAsyncSupported()).andReturn(false);
103+
replay(this.request);
104+
try {
105+
this.asyncRequest.startAsync();
106+
fail("expected exception");
107+
}
108+
catch (IllegalStateException ex) {
109+
assertThat(ex.getMessage(), containsString("Async support must be enabled"));
110+
}
111+
}
112+
100113
@Test
101114
public void startAsync_alreadyStarted() throws Exception {
102115
startAsync();
@@ -128,20 +141,17 @@ public void startAsync_stale() throws Exception {
128141
fail("expected exception");
129142
}
130143
catch (IllegalStateException ex) {
131-
assertStaleRequestMessage(ex);
144+
assertEquals("Cannot use async request after completion", ex.getMessage());
132145
}
133146
}
134147

135148
@Test
136149
public void complete_stale() throws Exception {
137150
this.asyncRequest.onComplete(new AsyncEvent(null));
138-
try {
139-
this.asyncRequest.complete();
140-
fail("expected exception");
141-
}
142-
catch (IllegalStateException ex) {
143-
assertStaleRequestMessage(ex);
144-
}
151+
this.asyncRequest.complete();
152+
153+
assertFalse(this.asyncRequest.isAsyncStarted());
154+
assertTrue(this.asyncRequest.isAsyncCompleted());
145155
}
146156

147157
@Test
@@ -151,15 +161,10 @@ public void sendError() throws Exception {
151161
}
152162

153163
@Test
154-
public void sendError_requestAlreadyCompleted() throws Exception {
164+
public void sendError_stale() throws Exception {
155165
this.asyncRequest.onComplete(new AsyncEvent(null));
156166
this.asyncRequest.sendError(HttpStatus.INTERNAL_SERVER_ERROR, "error");
157167
assertEquals(200, this.response.getStatus());
158168
}
159169

160-
161-
private void assertStaleRequestMessage(IllegalStateException ex) {
162-
assertEquals("Cannot use async request after completion", ex.getMessage());
163-
}
164-
165170
}

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -398,10 +398,10 @@ public void setAsyncTaskExecutor(AsyncTaskExecutor taskExecutor) {
398398
}
399399

400400
/**
401-
* Set the timeout for asynchronous request processing. When the timeout
402-
* begins depends on the underlying async technology. With the Servlet 3
403-
* async support the timeout begins after the main processing thread has
404-
* exited and has been returned to the container pool.
401+
* Set the timeout for asynchronous request processing in milliseconds.
402+
* When the timeout begins depends on the underlying async technology.
403+
* With the Servlet 3 async support the timeout begins after the main
404+
* processing thread has exited and has been returned to the container pool.
405405
* <p>If a value is not provided, the default timeout of the underlying
406406
* async technology is used (10 seconds on Tomcat with Servlet 3 async).
407407
*/

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ private AbstractDelegatingCallable geAsyncCallable(final ServletWebRequest webRe
126126

127127
return new AbstractDelegatingCallable() {
128128
public Object call() throws Exception {
129+
mavContainer.setRequestHandled(false);
129130
new CallableHandlerMethod(getNextCallable()).invokeAndHandle(webRequest, mavContainer, providedArgs);
130131
return null;
131132
}

0 commit comments

Comments
 (0)