Bug 59261 - Request getAsyncContext should throw IllegalStateException if async is not started
Summary: Request getAsyncContext should throw IllegalStateException if async is not st...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.33
Hardware: PC Mac OS X 10.1
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-01 15:05 UTC by Rob Winch
Modified: 2016-04-13 19:22 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Winch 2016-04-01 15:05:15 UTC
If ServletRequest.getAsyncContext() IllegalStateException is invoked and has not been put into asynchronous mode an . From the javadoc [1]:

> Throws: IllegalStateException - if this request has not been put into
> asynchronous mode, i.e., if neither startAsync() nor 
> startAsync(ServletRequest,ServletResponse) has been called

For implementations of HttpServletRequestWrapper that override this method, the fact that result can be null can cause problems [2]. It appears there are parts of tomcat that check if getAsyncContext() is null rather than checking isAsycStarted(). For example, ApplicationDispatcher checks if getAsyncContext() is null.


[1] http://docs.oracle.com/javaee/6/api/javax/servlet/ServletRequest.html#getAsyncContext()
[2] https://github.com/spring-cloud/spring-cloud-netflix/issues/868
Comment 1 Rob Winch 2016-04-01 15:06:39 UTC
I messed up the description some. Sorry about that. It should read:

If ServletRequest.getAsyncContext() is invoked and has not been put into asynchronous mode an IllegalStateException should be thrown.
Comment 2 Remy Maucherat 2016-04-01 15:32:24 UTC
Maybe that is what the specification says, but using null in this situation is considerably better than using an ISE which should be reserved for some invalid/meaningless situations.

Bad design ! [being part of that EG, I include myself in that since I missed it then ...]
Comment 3 Remy Maucherat 2016-04-05 20:58:55 UTC
Right now the spec is that getAsyncContext() should throw an ISE if isAsyncStarted returns false (which is not the same as asyncContext != null). This is an extremely risky change, some other places do check if asyncContext is null because it may have been there but is no longer started, so it cannot be swapped with isAsyncStarted.
Comment 4 Mark Thomas 2016-04-06 22:02:02 UTC
On the plus side, the async code is reasonably well covered by the unit tests.

The down side, as Rémy points out is that we'll need to carefully review all the calls to getAsyncContext(). We may also need a getAsyncContextInternal() method (or something along those lines) so we can get the async context in those cases where we need it even if isAsyncStarted() is false.

I guess working on this in 9.0.x, and ironing out the wrinkles before slowly back-porting is the way to go.
Comment 5 Mark Thomas 2016-04-06 22:14:25 UTC
Reading the Javadoc, the test is 'has one of the startAsync() methods been called' which is not quite the same as isAsyncStarted() == false. The spec document isn't much better. It uses the phrase 'has not been put in asynchronous mode'.

The key question is does the spec really mean '...has not been put in...' or was the intent to mean '...is not currently in...'. Rémy, do you have any insight on this?
Comment 6 Remy Maucherat 2016-04-06 22:33:57 UTC
isAsyncStarted is correct, and it's not the same as asyncContext != null, I checked GF before writing comment 3. However; Tomcat needs to know if a startAsync has been called earlier for its cleanup operations.

I was also thinking about a getAsyncContextInternal method, but it needs to be accessed on the internal Request class, which is harder.
Comment 7 Mark Thomas 2016-04-07 12:53:52 UTC
isAsyncStarted() makes sense and if we code to that and it is later relaxed we won't need to change anything. I think I am most of the way to a working patch for this.

We also need to raise this with the Servlet EG for clarification.
Comment 8 Mark Thomas 2016-04-07 16:21:04 UTC
Fixed in trunk for 9.0.0.M5 and the unit tests still all pass.
Comment 9 Mark Thomas 2016-04-07 19:48:08 UTC
I've back-ported this to 8.5.x for 8.5.1 and 8.0.x for 8.0.34.

I'll hold off on back-porting this to 7.0.x until after the 7.0.69 tag.
Comment 10 Mark Thomas 2016-04-13 19:22:12 UTC
Fix applied to 7.0.x for 7.0.70 onwards.