You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2016/04/01 17:05:15 UTC
[Bug 59261] New: Request getAsyncContext should throw
IllegalStateException if async is not started
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
Bug ID: 59261
Summary: Request getAsyncContext should throw
IllegalStateException if async is not started
Product: Tomcat 8
Version: 8.0.33
Hardware: PC
OS: Mac OS X 10.1
Status: NEW
Severity: normal
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: rwinch@gmail.com
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
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #3 from Remy Maucherat <re...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #5 from Mark Thomas <ma...@apache.org> ---
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?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #1 from Rob Winch <rw...@gmail.com> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #6 from Remy Maucherat <re...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #7 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #2 from Remy Maucherat <re...@apache.org> ---
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 ...]
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #9 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #8 from Mark Thomas <ma...@apache.org> ---
Fixed in trunk for 9.0.0.M5 and the unit tests still all pass.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
--- Comment #4 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59261] Request getAsyncContext should throw
IllegalStateException if async is not started
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59261
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #10 from Mark Thomas <ma...@apache.org> ---
Fix applied to 7.0.x for 7.0.70 onwards.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org