Undefined behaviour for AsyncContext#getRequest() and getResponse() after complete()/dispatch() about servlet

[glassfishrobot]

• Issue Imported From: https://github.com/javaee/servlet-spec/issues/6
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @shingwaichan
• Original Issue Closed By:@glassfishrobot

[glassfishrobot]

@glassfishrobot Commented
Reported by markt_asf

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
I believe the request/response pair should be valid and not recycled until the lifecycle has completed - either with a call to onError or onComplete.

So During those calls, getRequest and getResponse should still return the appropriate object. The use case for this might be to retrieve an attribute from the request or to look at isCommitted or getStatus methods of the request to see what the response was going to be if it had not errored.

[glassfishrobot]

@glassfishrobot Commented
markt_asf said:
I'd be fine with that approach. The question remains, what happens after the lifecycle has completed. My preference is still an ISE in that case.

[glassfishrobot]

@glassfishrobot Commented
@shingwaichan said:
According to javadoc of AsyncContext, when there is async timeout, the
following may be invoked:
a. invoke AsyncListener#onTimeout
b. AsyncContext#complete, AsyncListener#onComplete
c. AsyncContext#dispatch

One should throw IllegalStateException for AsyncContext#getRequest() / getResponse()
if AsyncContext#complete or AsyncContext#dispatch are called.
This will solve the following timeout scenario, too.

[glassfishrobot]

@glassfishrobot Commented
@shingwaichan said:
Sending src/main/java/javax/servlet/AsyncContext.java
Transmitting file data .
Committed revision 51990.

Sending servletobjects.fm
Sending status.fm
Transmitting file data ..
Committed revision 5.

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
Because the servlet specification allows (even encourages) object reuse/recycling, it is impossible to require a ISE to be thrown after the lifecycle completes. By definition of recycle, the object has been recycled and is now carrying the state of another request in another lifecycle, and it may well be in a state where such a call is applicable.

All we can say is that after a call to onComplete has returned, then the results of all calls to the AsyncContext API via an existing reference are undefined.

Now given current garbage collectors, it may no longer be good idea not to encourage such object reuse - but many containers already do so, thus it would be a big break to change that.

[glassfishrobot]

@glassfishrobot Commented
@shingwaichan said:
Need further investigation on request issuee.

[glassfishrobot]

@glassfishrobot Commented
markt_asf said:
It should be possible to use a thin, "throw away" wrapper around the AsyncContext that tracked state and threw the Exception leaving the underlying AsyncContext implementation to be safely recycled. Whether this complexity is worth the benefit of changing behaviour from "undertermined" to "throws ISE" is TBD.

My own view is that the complexity is worth the benefit. These sorts of re-use bugs can be really tricky to track down and this change would help considerably.

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
markt_asf,

I agree that the benefits of reusing objects are probably at best marginal now. A think wrapper may help, but it is probably just as much work to implement as removing the recycling of objects.

For Jetty we definitely plan to evaluate if our current recycling of request,response,asyncContext are still worth it - but it is not a change that we can make lightly.

Also, the state of the art with garbage collection may swing the other way someday, and recycling objects may come back in vogue. Thus I think because the spec originally allowed/encouraged recycling, then it should not change to prohibit recycling - even if containers move away from doing so.

Perhaps we can go for some verbage along the the lines of - all method calls on the AsyncContext are undefined after a call to onComplete. However implementations are encouraged to throw ISE if at all possible.

[glassfishrobot]

@glassfishrobot Commented
@shingwaichan said:
After timeout or commit, we would like to clarify the use of those previously acquired request or response objects are undefined.
The question is AysncContext.getRequest()/getResponse().
I prefer to have a deterministic behavior here.
As mentioned by Mark, this is feasible in the implementation, I would prefer to throw ISE in this case.

[glassfishrobot]

@glassfishrobot Commented
mode said:
@greg - I generally don't like to keep things undefined in the spec. I think we should say that an ISE is thrown. It may introduce some complexity for implementations but there are a handful of these . I agree with Mark that these the re-use bugs can be really tricky and hence from the developers point of view it will be very clear to define in the spec that we throw an ISE and require all containers to implement it that way rather than "encourage" container vendors to throw the ISE.

[glassfishrobot]

@glassfishrobot Commented
rstoyanchev said:

After timeout or commit, we would like to clarify the use of those previously acquired request or response objects are undefined.

As discussed above, this should not be done after a timeout but only after the lifecycle has completed - i.e. with a call to onError or onComplete. The subject of this issue should be corrected to reflect that as well.

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
@rajiv It think we need to be consistent in the specification. If we say that AsyncContext objects throw ISE if used after the request lifecycle is complete, then I think we also need to do the same for the request and response instances themselves. I think it is too confusing to have some objects be reusable and others not.

Besides if AsyncContext.getRequest() throws an ISE, but Request itself is not usable after the completion of the request life cycle, then we have a race condition as an async thread might call getRequest() a nano second before the end of the request life cycle and then perform illegal operations on the request object it got in return.

So either we have to explicitly prohibit all recycling of objects and say that request/response/contexts are all invalid after the completion; OR we continue as we are saying that it is the applications responsibility to not call methods on any of these objects after the lifecycle has completed.

I do not think a half way house of some objects being recyclable while other are not is viable nor easy to understand.

If this was a green field spec, I think that requiring fresh objects on every cycle would be best. But as we have lots of existing code, then I now favour the status quo and that we continue to allow recycling of these objects.

[glassfishrobot]

@glassfishrobot Commented
markt_asf said:
Bugs triggered by retaining references to the Request/Response objects are usually more obvious than those involving with AsycnContext. The multi-threaded nature of async adds complexity that does make it easier to get things wrong - hence my focus on AsycnContext.

I'm not against expanding this change to encompass Request/Response and Greg's consistency argument is a valid one but I would also be happy with just changing AsyncContext. I don't think the status quo is a reasonable option.

[glassfishrobot]

@glassfishrobot Commented
markt_asf said:
Update title

[glassfishrobot]

@glassfishrobot Commented
mode said:
I'd like to close out on this issue one way or another. Let me start a thread on the EG to make sure that everyone is in fact tracking the issue on issue tracker and see what their opinions are.

[glassfishrobot]

@glassfishrobot Commented
@shingwaichan said:
Per discussion in expert group discussion, 12/11/12, 1/9/13, 2/4/13, ISE will be thrown in this case.

[glassfishrobot]

@glassfishrobot Commented
This issue was imported from java.net JIRA SERVLET_SPEC-6

[glassfishrobot]

@glassfishrobot Commented
Marked as fixed on Monday, February 11th 2013, 10:27:26 am