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

        Update title

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved: