servlet-spec
  1. servlet-spec
  2. SERVLET_SPEC-6

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

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      The specification is unclear on what should happen here. Clearly, it isn't going to work but how it fails and when it fails is undefined.

      My preference is for declaring that those methods throw IllegalStateException in those circumstances.

        Activity

        Hide
        gregwilkins added a comment -

        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.

        Show
        gregwilkins added a comment - 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.
        Hide
        markt_asf added a comment -

        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.

        Show
        markt_asf added a comment - 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.
        Hide
        Shing Wai Chan added a comment -

        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.

        Show
        Shing Wai Chan added a comment - 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.
        Hide
        Shing Wai Chan added a comment -

        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.

        Show
        Shing Wai Chan added a comment - 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.
        Hide
        gregwilkins added a comment -

        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.

        Show
        gregwilkins added a comment - 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.
        Hide
        Shing Wai Chan added a comment -

        Need further investigation on request issuee.

        Show
        Shing Wai Chan added a comment - Need further investigation on request issuee.
        Hide
        markt_asf added a comment -

        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.

        Show
        markt_asf added a comment - 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.
        Hide
        gregwilkins added a comment -

        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.

        Show
        gregwilkins added a comment - 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.
        Hide
        Shing Wai Chan added a comment -

        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.

        Show
        Shing Wai Chan added a comment - 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.
        Hide
        Rajiv Mordani added a comment -

        @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.

        Show
        Rajiv Mordani added a comment - @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.
        Hide
        rstoyanchev added a comment -

        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.

        Show
        rstoyanchev added a comment - 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.
        Hide
        gregwilkins added a comment -

        @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.

        Show
        gregwilkins added a comment - @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.
        Hide
        markt_asf added a comment -

        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.

        Show
        markt_asf added a comment - 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.
        Hide
        markt_asf added a comment -

        Update title

        Show
        markt_asf added a comment - Update title
        Hide
        Rajiv Mordani added a comment -

        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.

        Show
        Rajiv Mordani added a comment - 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.
        Hide
        Shing Wai Chan added a comment -

        Per discussion in expert group discussion, 12/11/12, 1/9/13, 2/4/13, ISE will be thrown in this case.

        Show
        Shing Wai Chan added a comment - Per discussion in expert group discussion, 12/11/12, 1/9/13, 2/4/13, ISE will be thrown in this case.

          People

          • Assignee:
            Shing Wai Chan
            Reporter:
            markt_asf
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: