Skip to content

Commit 6cc512b

Browse files
committed
Ensure async Callables are in sync with the call stack
After this change each call stack level pushes and pops an async Callable to ensure the AsyncExecutionChain is in sync with the call stack. Before this change, a controller returning a "forward:" prefixed string caused the AsyncExecutionChain to contain a extra Callables that did not match the actual call stack. Issue: SPR-9611
1 parent 33a3681 commit 6cc512b

27 files changed

+233
-232
lines changed

spring-orm/src/main/java/org/springframework/orm/hibernate3/support/OpenSessionInViewFilter.java

+4-5
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ protected void doFilterInternal(
187187
SessionHolder sessionHolder = new SessionHolder(session);
188188
TransactionSynchronizationManager.bindResource(sessionFactory, sessionHolder);
189189

190-
chain.addDelegatingCallable(getAsyncCallable(request, sessionFactory, sessionHolder));
190+
chain.push(getAsyncCallable(request, sessionFactory, sessionHolder));
191191
}
192192
}
193193
else {
@@ -204,21 +204,20 @@ protected void doFilterInternal(
204204
try {
205205
filterChain.doFilter(request, response);
206206
}
207-
208207
finally {
209208
if (!participate) {
210209
if (isSingleSession()) {
211210
// single session mode
212211
SessionHolder sessionHolder =
213212
(SessionHolder) TransactionSynchronizationManager.unbindResource(sessionFactory);
214-
if (chain.isAsyncStarted()) {
213+
if (!chain.pop()) {
215214
return;
216215
}
217216
logger.debug("Closing single Hibernate Session in OpenSessionInViewFilter");
218217
closeSession(sessionHolder.getSession(), sessionFactory);
219218
}
220219
else {
221-
if (chain.isAsyncStarted()) {
220+
if (!chain.pop()) {
222221
throw new IllegalStateException("Deferred close is not supported with async requests.");
223222
}
224223
// deferred close mode
@@ -303,7 +302,7 @@ private AbstractDelegatingCallable getAsyncCallable(final HttpServletRequest req
303302
public Object call() throws Exception {
304303
TransactionSynchronizationManager.bindResource(sessionFactory, sessionHolder);
305304
try {
306-
getNextCallable().call();
305+
getNext().call();
307306
}
308307
finally {
309308
SessionHolder sessionHolder =

spring-orm/src/main/java/org/springframework/orm/hibernate3/support/OpenSessionInViewInterceptor.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ public AbstractDelegatingCallable getAsyncCallable(WebRequest request) {
181181
return new AbstractDelegatingCallable() {
182182
public Object call() throws Exception {
183183
TransactionSynchronizationManager.bindResource(getSessionFactory(), sessionHolder);
184-
getNextCallable().call();
184+
getNext().call();
185185
return null;
186186
}
187187
};

spring-orm/src/main/java/org/springframework/orm/hibernate4/support/OpenSessionInViewFilter.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ protected void doFilterInternal(
119119
SessionHolder sessionHolder = new SessionHolder(session);
120120
TransactionSynchronizationManager.bindResource(sessionFactory, sessionHolder);
121121

122-
chain.addDelegatingCallable(getAsyncCallable(request, sessionFactory, sessionHolder));
122+
chain.push(getAsyncCallable(request, sessionFactory, sessionHolder));
123123
}
124124

125125
try {
@@ -130,7 +130,7 @@ protected void doFilterInternal(
130130
if (!participate) {
131131
SessionHolder sessionHolder =
132132
(SessionHolder) TransactionSynchronizationManager.unbindResource(sessionFactory);
133-
if (chain.isAsyncStarted()) {
133+
if (!chain.pop()) {
134134
return;
135135
}
136136
logger.debug("Closing Hibernate Session in OpenSessionInViewFilter");
@@ -198,7 +198,7 @@ private AbstractDelegatingCallable getAsyncCallable(final HttpServletRequest req
198198
public Object call() throws Exception {
199199
TransactionSynchronizationManager.bindResource(sessionFactory, sessionHolder);
200200
try {
201-
getNextCallable().call();
201+
getNext().call();
202202
}
203203
finally {
204204
SessionHolder sessionHolder =

spring-orm/src/main/java/org/springframework/orm/hibernate4/support/OpenSessionInViewInterceptor.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ public AbstractDelegatingCallable getAsyncCallable(WebRequest request) {
137137
return new AbstractDelegatingCallable() {
138138
public Object call() throws Exception {
139139
TransactionSynchronizationManager.bindResource(getSessionFactory(), sessionHolder);
140-
getNextCallable().call();
140+
getNext().call();
141141
return null;
142142
}
143143
};

spring-orm/src/test/java/org/springframework/orm/hibernate3/support/OpenSessionInViewTests.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ public void testOpenSessionInViewInterceptorAsyncScenario() throws Exception {
176176
verify(sf);
177177
verify(session);
178178

179-
asyncCallable.setNextCallable(new Callable<Object>() {
179+
asyncCallable.setNext(new Callable<Object>() {
180180
public Object call() {
181181
return null;
182182
}
@@ -484,7 +484,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo
484484
verify(asyncWebRequest);
485485

486486
chain.setTaskExecutor(new SyncTaskExecutor());
487-
chain.setCallable(new Callable<Object>() {
487+
chain.setLastCallable(new Callable<Object>() {
488488
public Object call() {
489489
assertTrue(TransactionSynchronizationManager.hasResource(sf));
490490
return null;
@@ -503,7 +503,7 @@ public Object call() {
503503
replay(sf);
504504
replay(session);
505505

506-
chain.startCallableChainProcessing();
506+
chain.startCallableProcessing();
507507
assertFalse(TransactionSynchronizationManager.hasResource(sf));
508508

509509
verify(sf);

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

+9-18
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,11 @@
1919
import java.util.concurrent.Callable;
2020

2121
/**
22-
* A base class for a Callable that can be used in a chain of Callable instances.
23-
*
24-
* <p>Typical use for async request processing scenarios involves:
25-
* <ul>
26-
* <li>Create an instance of this type and register it via
27-
* {@link AsyncExecutionChain#addDelegatingCallable(AbstractDelegatingCallable)}
28-
* (internally the nodes of the chain will be linked so no need to set up "next").
29-
* <li>Provide an implementation of {@link Callable#call()} that contains the
30-
* logic needed to complete request processing outside the main processing thread.
31-
* <li>In the implementation, delegate to the next Callable to obtain
32-
* its result, e.g. ModelAndView, and then do some post-processing, e.g. view
33-
* resolution. In some cases both pre- and post-processing might be
34-
* appropriate, e.g. setting up {@link ThreadLocal} storage.
35-
* </ul>
22+
* A base class for a Callable used to form a chain of Callable instances.
23+
* Instances of this class are typically registered via
24+
* {@link AsyncExecutionChain#push(AbstractDelegatingCallable)} in which case
25+
* there is no need to set the next Callable. Implementations can simply use
26+
* {@link #getNext()} to delegate to the next Callable and assume it will be set.
3627
*
3728
* @author Rossen Stoyanchev
3829
* @since 3.2
@@ -43,12 +34,12 @@ public abstract class AbstractDelegatingCallable implements Callable<Object> {
4334

4435
private Callable<Object> next;
4536

46-
public void setNextCallable(Callable<Object> nextCallable) {
47-
this.next = nextCallable;
37+
protected Callable<Object> getNext() {
38+
return this.next;
4839
}
4940

50-
protected Callable<Object> getNextCallable() {
51-
return this.next;
41+
public void setNext(Callable<Object> callable) {
42+
this.next = callable;
5243
}
5344

5445
}

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

+76-73
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@
1616

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

19-
import java.util.ArrayList;
20-
import java.util.List;
19+
import java.util.ArrayDeque;
20+
import java.util.Deque;
2121
import java.util.concurrent.Callable;
2222

2323
import javax.servlet.ServletRequest;
@@ -31,18 +31,20 @@
3131

3232
/**
3333
* The central class for managing async request processing, mainly intended as
34-
* an SPI and typically not by non-framework classes.
34+
* an SPI and not typically used directly by application classes.
3535
*
36-
* <p>An async execution chain consists of a sequence of Callable instances and
37-
* represents the work required to complete request processing in a separate
38-
* thread. To construct the chain, each layer in the call stack of a normal
39-
* request (e.g. filter, servlet) may contribute an
40-
* {@link AbstractDelegatingCallable} when a request is being processed.
41-
* For example the DispatcherServlet might contribute a Callable that
42-
* performs view resolution while a HandlerAdapter might contribute a Callable
43-
* that returns the ModelAndView, etc. The last Callable is the one that
44-
* actually produces an application-specific value, for example the Callable
45-
* returned by an {@code @RequestMapping} method.
36+
* <p>An async execution chain consists of a sequence of Callable instances that
37+
* represent the work required to complete request processing in a separate thread.
38+
* To construct the chain, each level of the call stack pushes an
39+
* {@link AbstractDelegatingCallable} during the course of a normal request and
40+
* pops (removes) it on the way out. If async processing has not started, the pop
41+
* operation succeeds and the processing continues as normal, or otherwise if async
42+
* processing has begun, the main processing thread must be exited.
43+
*
44+
* <p>For example the DispatcherServlet might contribute a Callable that completes
45+
* view resolution or the HandlerAdapter might contribute a Callable that prepares a
46+
* ModelAndView while the last Callable in the chain is usually associated with the
47+
* application, e.g. the return value of an {@code @RequestMapping} method.
4648
*
4749
* @author Rossen Stoyanchev
4850
* @since 3.2
@@ -51,13 +53,13 @@ public final class AsyncExecutionChain {
5153

5254
public static final String CALLABLE_CHAIN_ATTRIBUTE = AsyncExecutionChain.class.getName() + ".CALLABLE_CHAIN";
5355

54-
private final List<AbstractDelegatingCallable> delegatingCallables = new ArrayList<AbstractDelegatingCallable>();
56+
private final Deque<AbstractDelegatingCallable> callables = new ArrayDeque<AbstractDelegatingCallable>();
5557

56-
private Callable<Object> callable;
58+
private Callable<Object> lastCallable;
5759

5860
private AsyncWebRequest asyncWebRequest;
5961

60-
private AsyncTaskExecutor taskExecutor = new SimpleAsyncTaskExecutor("AsyncExecutionChain");
62+
private AsyncTaskExecutor taskExecutor = new SimpleAsyncTaskExecutor("MvcAsync");
6163

6264
/**
6365
* Private constructor
@@ -68,7 +70,7 @@ private AsyncExecutionChain() {
6870

6971
/**
7072
* Obtain the AsyncExecutionChain for the current request.
71-
* Or if not found, create an instance and associate it with the request.
73+
* Or if not found, create it and associate it with the request.
7274
*/
7375
public static AsyncExecutionChain getForCurrentRequest(ServletRequest request) {
7476
AsyncExecutionChain chain = (AsyncExecutionChain) request.getAttribute(CALLABLE_CHAIN_ATTRIBUTE);
@@ -81,7 +83,7 @@ public static AsyncExecutionChain getForCurrentRequest(ServletRequest request) {
8183

8284
/**
8385
* Obtain the AsyncExecutionChain for the current request.
84-
* Or if not found, create an instance and associate it with the request.
86+
* Or if not found, create it and associate it with the request.
8587
*/
8688
public static AsyncExecutionChain getForCurrentRequest(WebRequest request) {
8789
int scope = RequestAttributes.SCOPE_REQUEST;
@@ -94,105 +96,106 @@ public static AsyncExecutionChain getForCurrentRequest(WebRequest request) {
9496
}
9597

9698
/**
97-
* Provide an instance of an AsyncWebRequest.
98-
* This property must be set before async request processing can begin.
99+
* Provide an instance of an AsyncWebRequest -- required for async processing.
99100
*/
100101
public void setAsyncWebRequest(AsyncWebRequest asyncRequest) {
102+
Assert.state(!isAsyncStarted(), "Cannot set AsyncWebRequest after the start of async processing.");
101103
this.asyncWebRequest = asyncRequest;
102104
}
103105

104106
/**
105-
* Provide an AsyncTaskExecutor to use when
106-
* {@link #startCallableChainProcessing()} is invoked, for example when a
107-
* controller method returns a Callable.
108-
* <p>By default a {@link SimpleAsyncTaskExecutor} instance is used.
107+
* Provide an AsyncTaskExecutor for use with {@link #startCallableProcessing()}.
108+
* <p>By default a {@link SimpleAsyncTaskExecutor} instance is used. Applications are
109+
* advised to provide a TaskExecutor configured for production use.
110+
* @see org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter#setAsyncTaskExecutor
109111
*/
110112
public void setTaskExecutor(AsyncTaskExecutor taskExecutor) {
111113
this.taskExecutor = taskExecutor;
112114
}
113115

114116
/**
115-
* Whether async request processing has started through one of:
116-
* <ul>
117-
* <li>{@link #startCallableChainProcessing()}
118-
* <li>{@link #startDeferredResultProcessing(DeferredResult)}
119-
* </ul>
117+
* Push an async Callable for the current stack level. This method should be
118+
* invoked before delegating to the next level of the stack where async
119+
* processing may start.
120120
*/
121-
public boolean isAsyncStarted() {
122-
return ((this.asyncWebRequest != null) && this.asyncWebRequest.isAsyncStarted());
121+
public void push(AbstractDelegatingCallable callable) {
122+
Assert.notNull(callable, "Async Callable is required");
123+
this.callables.addFirst(callable);
123124
}
124125

125126
/**
126-
* Add a Callable with logic required to complete request processing in a
127-
* separate thread. See {@link AbstractDelegatingCallable} for details.
127+
* Pop the Callable of the current stack level. Ensure this method is invoked
128+
* after delegation to the next level of the stack where async processing may
129+
* start. The pop operation succeeds if async processing did not start.
130+
* @return {@code true} if the Callable was removed, or {@code false}
131+
* otherwise (i.e. async started).
128132
*/
129-
public void addDelegatingCallable(AbstractDelegatingCallable callable) {
130-
Assert.notNull(callable, "Callable required");
131-
this.delegatingCallables.add(callable);
133+
public boolean pop() {
134+
if (isAsyncStarted()) {
135+
return false;
136+
}
137+
else {
138+
this.callables.removeFirst();
139+
return true;
140+
}
132141
}
133142

134143
/**
135-
* Add the last Callable, for example the one returned by the controller.
136-
* This property must be set prior to invoking
137-
* {@link #startCallableChainProcessing()}.
144+
* Whether async request processing has started.
138145
*/
139-
public AsyncExecutionChain setCallable(Callable<Object> callable) {
140-
Assert.notNull(callable, "Callable required");
141-
this.callable = callable;
142-
return this;
146+
public boolean isAsyncStarted() {
147+
return ((this.asyncWebRequest != null) && this.asyncWebRequest.isAsyncStarted());
143148
}
144149

145150
/**
146-
* Start the async execution chain by submitting an
147-
* {@link AsyncExecutionChainRunnable} instance to the TaskExecutor provided via
148-
* {@link #setTaskExecutor(AsyncTaskExecutor)} and returning immediately.
149-
* @see AsyncExecutionChainRunnable
151+
* Set the last Callable, e.g. the one returned by the controller.
150152
*/
151-
public void startCallableChainProcessing() {
152-
startAsync();
153-
this.taskExecutor.execute(new AsyncExecutionChainRunnable(this.asyncWebRequest, buildChain()));
153+
public AsyncExecutionChain setLastCallable(Callable<Object> callable) {
154+
Assert.notNull(callable, "Callable required");
155+
this.lastCallable = callable;
156+
return this;
154157
}
155158

156-
private void startAsync() {
157-
Assert.state(this.asyncWebRequest != null, "An AsyncWebRequest is required to start async processing");
159+
/**
160+
* Start async processing and execute the async chain with an AsyncTaskExecutor.
161+
* This method returns immediately.
162+
*/
163+
public void startCallableProcessing() {
164+
Assert.state(this.asyncWebRequest != null, "AsyncWebRequest was not set");
158165
this.asyncWebRequest.startAsync();
166+
this.taskExecutor.execute(new AsyncExecutionChainRunnable(this.asyncWebRequest, buildChain()));
159167
}
160168

161169
private Callable<Object> buildChain() {
162-
Assert.state(this.callable != null, "The last callable is required to build the async chain");
163-
this.delegatingCallables.add(new StaleAsyncRequestCheckingCallable(asyncWebRequest));
164-
Callable<Object> result = this.callable;
165-
for (int i = this.delegatingCallables.size() - 1; i >= 0; i--) {
166-
AbstractDelegatingCallable callable = this.delegatingCallables.get(i);
167-
callable.setNextCallable(result);
168-
result = callable;
170+
Assert.state(this.lastCallable != null, "The last Callable was not set");
171+
AbstractDelegatingCallable head = new StaleAsyncRequestCheckingCallable(this.asyncWebRequest);
172+
head.setNext(this.lastCallable);
173+
for (AbstractDelegatingCallable callable : this.callables) {
174+
callable.setNext(head);
175+
head = callable;
169176
}
170-
return result;
177+
return head;
171178
}
172179

173180
/**
174-
* Mark the start of async request processing accepting the provided
175-
* DeferredResult and initializing it such that if
176-
* {@link DeferredResult#set(Object)} is called (from another thread),
177-
* the set Object value will be processed with the execution chain by
178-
* invoking {@link AsyncExecutionChainRunnable}.
179-
* <p>The resulting processing from this method is identical to
180-
* {@link #startCallableChainProcessing()}. The main difference is in
181-
* the threading model, i.e. whether a TaskExecutor is used.
182-
* @see DeferredResult
181+
* Start async processing and initialize the given DeferredResult so when
182+
* its value is set, the async chain is executed with an AsyncTaskExecutor.
183183
*/
184184
public void startDeferredResultProcessing(final DeferredResult<?> deferredResult) {
185185
Assert.notNull(deferredResult, "DeferredResult is required");
186-
startAsync();
186+
Assert.state(this.asyncWebRequest != null, "AsyncWebRequest was not set");
187+
this.asyncWebRequest.startAsync();
188+
187189
deferredResult.init(new DeferredResultHandler() {
188190
public void handle(Object result) {
189191
if (asyncWebRequest.isAsyncCompleted()) {
190-
throw new StaleAsyncWebRequestException("Async request processing already completed");
192+
throw new StaleAsyncWebRequestException("Too late to set DeferredResult: " + result);
191193
}
192-
setCallable(new PassThroughCallable(result));
193-
new AsyncExecutionChainRunnable(asyncWebRequest, buildChain()).run();
194+
setLastCallable(new PassThroughCallable(result));
195+
taskExecutor.execute(new AsyncExecutionChainRunnable(asyncWebRequest, buildChain()));
194196
}
195197
});
198+
196199
this.asyncWebRequest.setTimeoutHandler(deferredResult.getTimeoutHandler());
197200
}
198201

0 commit comments

Comments
 (0)