websocket-spec
  1. websocket-spec
  2. WEBSOCKET_SPEC-174

Add more detail to javax.websocket.Session JavaDoc about which methods do not throw IllegalStateException

    Details

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

      Description

      Current JavaDoc says this:

      Once the session is closed, it is no longer valid for use by applications. Calling any of its methods once the session has been closed will result in an IllegalStateException being thrown. Developers should retrieve any information from the session during the Endpoint.onClose(javax.websocket.Session, javax.websocket.CloseReason) method.

      I believe this needs more clarification. I submit the following:

      • close() and close(CloseReason), in accordance with the java.io.Closeable contract, must NOT throw an exception if the Session is already closed, but instead calling these after Session close should be a no-op. Since this is specified already in Closeable, it may or may not be necessary to specify this in Session, but I suggest it doesn't hurt, and makes it 100% unambiguous.
      • isOpen() should NOT throw an exception, since it is used to indicate whether the Session is open (and, thus, whether a subsequent call to some other method is unlikely to throw an IllegalStateException).
      • getId() should NOT throw an exception, since the String it returns cannot be acted upon, and the ID could be useful debugging/tracking/logging/troubleshooting information post-close.

        Activity

        Nick Williams created issue -
        Hide
        Nick Williams added a comment - - edited

        Now that I think about it, even more clarification is needed. Consider this: Whether I call Session#close(...) or the remote endpoint closes the Session, it is not valid to send messages on the remote endpoint in Endpoint#onClose(...). But the JavaDoc implicitly suggests that Endpoint#onClose(...) can call any Session method without throwing an IllegalStateException. I do not believe this is correct. For the most part, Session methods shouldn't throw IllegalStateException until AFTER Endpoint#onClose(...) returns. However, I believe the following methods should throw IllegalStateExceptions if called from Endpoint#onClose(...):

        • addMessageHandler(MessageHandler)
        • removeMessageHandler(MessageHandler)
        • Any set* methods.
        • getAsyncRemote()
        • getBasicRemote()

        This can simply be handled by (internally) assuming the session has three states: OPEN, CLOSING and CLOSED. It's OPEN until close(...) is called locally or remotely, then it's CLOSING until Endpoint#onClose(...) returns, then it's CLOSED. The methods mentioned in this comment throw an IllegalStateException if the session state != OPEN, the rest of the methods throw an IllegalStateException if the session state == CLOSED, except for the four exceptions noted above, which never throw IllegalStateException.

        Thoughts?

        Show
        Nick Williams added a comment - - edited Now that I think about it, even more clarification is needed. Consider this: Whether I call Session#close(...) or the remote endpoint closes the Session , it is not valid to send messages on the remote endpoint in Endpoint#onClose(...) . But the JavaDoc implicitly suggests that Endpoint#onClose(...) can call any Session method without throwing an IllegalStateException . I do not believe this is correct. For the most part, Session methods shouldn't throw IllegalStateException until AFTER Endpoint#onClose(...) returns. However, I believe the following methods should throw IllegalStateExceptions if called from Endpoint#onClose(...) : addMessageHandler(MessageHandler) removeMessageHandler(MessageHandler) Any set* methods. getAsyncRemote() getBasicRemote() This can simply be handled by (internally) assuming the session has three states: OPEN , CLOSING and CLOSED . It's OPEN until close(...) is called locally or remotely, then it's CLOSING until Endpoint#onClose(...) returns, then it's CLOSED . The methods mentioned in this comment throw an IllegalStateException if the session state != OPEN , the rest of the methods throw an IllegalStateException if the session state == CLOSED , except for the four exceptions noted above, which never throw IllegalStateException . Thoughts?
        Hide
        dannycoward added a comment -

        I think you're right. I've clarified that during on close developers can access the session information (id, user data etc), but should not try to modify any of the data, nor try to send messages.

        I think that covers the cases that could cause issues.

        I think CLOSING is a good way to model it internally, although there are even two sorts of closing: one is closing initiated locally, and one is closing after receipt of a close frame from the client.

        Show
        dannycoward added a comment - I think you're right. I've clarified that during on close developers can access the session information (id, user data etc), but should not try to modify any of the data, nor try to send messages. I think that covers the cases that could cause issues. I think CLOSING is a good way to model it internally, although there are even two sorts of closing: one is closing initiated locally, and one is closing after receipt of a close frame from the client.
        dannycoward made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee dannycoward [ dannycoward ]
        Tags final
        Resolution Fixed [ 1 ]
        Hide
        Nick Williams added a comment -

        I think we need slightly more clarification still. See this bug:

        https://issues.apache.org/bugzilla/show_bug.cgi?id=54746

        I must be able to call certain methods to obtain information from a session (getId, getRequestParameterMap, getRequestURI, getUserPrincipal, getUserProperties, getContainer, getPathParameters, getQueryString) even after a session is closed. This is especially important if onError is called after a session is closed (as is the case with this bug). The data provided by these methods is crucial to log an error and help track down its cause.

        I propose that these eight methods be made to NEVER throw an IllegalStateException. No benefit is gained from throwing an IllegalStateException in these methods after the Session is closed, and no danger is created by not throwing an IllegalStateException. These methods are by their very nature idempotent.

        Show
        Nick Williams added a comment - I think we need slightly more clarification still. See this bug: https://issues.apache.org/bugzilla/show_bug.cgi?id=54746 I must be able to call certain methods to obtain information from a session ( getId , getRequestParameterMap , getRequestURI , getUserPrincipal , getUserProperties , getContainer , getPathParameters , getQueryString ) even after a session is closed. This is especially important if onError is called after a session is closed (as is the case with this bug). The data provided by these methods is crucial to log an error and help track down its cause. I propose that these eight methods be made to NEVER throw an IllegalStateException . No benefit is gained from throwing an IllegalStateException in these methods after the Session is closed, and no danger is created by not throwing an IllegalStateException . These methods are by their very nature idempotent.
        Hide
        Nick Williams added a comment -

        I would love to get this further clarified for Java EE 8, as per the comment above.

        Show
        Nick Williams added a comment - I would love to get this further clarified for Java EE 8, as per the comment above.

          People

          • Assignee:
            dannycoward
            Reporter:
            Nick Williams
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 15 minutes
              15m
              Remaining:
              Remaining Estimate - 15 minutes
              15m
              Logged:
              Time Spent - Not Specified
              Not Specified