Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Works as designed
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      • Session.addMessageHandler and @WebSocketMessage specify different rules (limits) to registered MessageHandlers. This should be unified if you want to be able to achieve same results via programmatic and annotated case.
      • MessageHandler and Session javadoc sometimes mention "listener" instead of "handler" when describing MessageHandler
      • Session.addMessageHandler does specify rules which are applicable to MessageHandler.Basic (at least it seams to), what about MessageHandler.Async (there is no specification of Async types)? It does not mention "decodable" type at all.
      • Threading issues needs to be resolved (still TBDs in javadoc)

        Activity

        Hide
        dannycoward added a comment -

        Making sure I'm understanding this, first bullet:-

        From WebSocketMessage:-
        "Each websocket

        • endpoint may only have one message handling method for each of the native websocket message formats: text, binary and pong. Methods
        • using this annotation are allowed to have
        • parameters of types described below, otherwise the container will generate an error at deployment time."

        from Session.addMessageHandler:-

        "Only one message handler per

        • native websocket message type may be added: a maximum of one for handling incoming text messages,
        • a maximum of one for handling incoming binary messages, and a maximum of one for handling incoming pong
        • messages. Adding more than one of any one type will result in an illegal state exception being thrown
        • by this method."

        ...perhaps its the language (or a different version), but the limits are the same ?

        second bullet: thanks, got them all. I think it best to say the removal may be blocked if the handler is in the middle of processing something ?

        third bullet: The language I used for message handlers is general: a message handler processing text messages could be a MessageHandler.Basic<String>, MessageHandler.Async<String>, or MessageHandler.Basic<Apple> where there is an apple decoder for text messages. so I think that covers it ?

        last bullet: see above on threading.

        Show
        dannycoward added a comment - Making sure I'm understanding this, first bullet:- From WebSocketMessage:- "Each websocket endpoint may only have one message handling method for each of the native websocket message formats: text, binary and pong. Methods using this annotation are allowed to have parameters of types described below, otherwise the container will generate an error at deployment time." from Session.addMessageHandler:- "Only one message handler per native websocket message type may be added: a maximum of one for handling incoming text messages, a maximum of one for handling incoming binary messages, and a maximum of one for handling incoming pong messages. Adding more than one of any one type will result in an illegal state exception being thrown by this method." ...perhaps its the language (or a different version), but the limits are the same ? second bullet: thanks, got them all. I think it best to say the removal may be blocked if the handler is in the middle of processing something ? third bullet: The language I used for message handlers is general: a message handler processing text messages could be a MessageHandler.Basic<String>, MessageHandler.Async<String>, or MessageHandler.Basic<Apple> where there is an apple decoder for text messages. so I think that covers it ? last bullet: see above on threading.
        Hide
        Pavel Bucek added a comment -

        ad 1) what I'd like to see is have this unified. I still think there is some difference between Session.addMessageHandler and @WebSocketMessage javadoc, but it might be just on my side. Anyway if Session.addMessageHandler javadoc would contain just simple statements about params + reference to @WebSocketMessage, it would be more readable (for me) because I wouldn't need to "parse" twice same info.

        ad 3) see 1).. basically I see some definitions/declarations - I might be just too strict when reading the documentation, but I kind of need to do that - if not anything else, I'm sure I will need to defend my implementation when it will be confronted with QA developer(s).

        Show
        Pavel Bucek added a comment - ad 1) what I'd like to see is have this unified. I still think there is some difference between Session.addMessageHandler and @WebSocketMessage javadoc, but it might be just on my side. Anyway if Session.addMessageHandler javadoc would contain just simple statements about params + reference to @WebSocketMessage, it would be more readable (for me) because I wouldn't need to "parse" twice same info. ad 3) see 1).. basically I see some definitions/declarations - I might be just too strict when reading the documentation, but I kind of need to do that - if not anything else, I'm sure I will need to defend my implementation when it will be confronted with QA developer(s).
        Hide
        dannycoward added a comment -

        OK fair enough, its good to tighten this up! I've cleaned this up so there should be no room for interpretation.

        MessageHandler.Basic and MessageHandler.Async explicitly define which types T are for text, binary and pong handling.

        Session.addHander() now links to that so you can get an unambiguous list.

        I kept the @WebSocketMessage type list separate, which might not be the most readable for you, but I did organize it into text, binary, pong message handling, so that should be clearer for those readers who are thinking of it in those terms.

        The threading issues are already captured in another one, so I'm going to close this one out.

        Show
        dannycoward added a comment - OK fair enough, its good to tighten this up! I've cleaned this up so there should be no room for interpretation. MessageHandler.Basic and MessageHandler.Async explicitly define which types T are for text, binary and pong handling. Session.addHander() now links to that so you can get an unambiguous list. I kept the @WebSocketMessage type list separate, which might not be the most readable for you, but I did organize it into text, binary, pong message handling, so that should be clearer for those readers who are thinking of it in those terms. The threading issues are already captured in another one, so I'm going to close this one out.
        Hide
        dannycoward added a comment -

        see last comment. I also made the specification document refer to the definition of parameters types and returns types on @WebSocketMessage instead of trying to maintain this in two places.

        Show
        dannycoward added a comment - see last comment. I also made the specification document refer to the definition of parameters types and returns types on @WebSocketMessage instead of trying to maintain this in two places.
        Hide
        Pavel Bucek added a comment -

        great, thanks!

        Show
        Pavel Bucek added a comment - great, thanks!
        Hide
        stepan.kopriva added a comment -

        According to the javadoc on @WebSocketMessage there can't be two methods in one class where one of the methods takes String as a parameter and the other one takes object parameter for which the endpoint has a text decoder.

        The same holds for two methods where each takes different object parameter for which the endpoint has a text decoder.

        Show
        stepan.kopriva added a comment - According to the javadoc on @WebSocketMessage there can't be two methods in one class where one of the methods takes String as a parameter and the other one takes object parameter for which the endpoint has a text decoder. The same holds for two methods where each takes different object parameter for which the endpoint has a text decoder.
        Hide
        dannycoward added a comment -

        Yes, I think this was the big simplification what we decided in the expert group. But let me go back and double check.

        Show
        dannycoward added a comment - Yes, I think this was the big simplification what we decided in the expert group. But let me go back and double check.
        Hide
        dannycoward added a comment -

        We have reconcluded that multiple message handlers won't work in generality. The basic problem is that there is no identifying data on an incoming message to tell you what kind of message it is, except string, binary or pong.

        There is a potential (small) relaxation however, which is to allow one MessageHandler.Basic and one MessageHandler.Async per native message type: text and binary.

        This can be mapped unambiguously, and might be moderately useful for some cases.

        So I will propose this to the expert group.

        Show
        dannycoward added a comment - We have reconcluded that multiple message handlers won't work in generality. The basic problem is that there is no identifying data on an incoming message to tell you what kind of message it is, except string, binary or pong. There is a potential (small) relaxation however, which is to allow one MessageHandler.Basic and one MessageHandler.Async per native message type: text and binary. This can be mapped unambiguously, and might be moderately useful for some cases. So I will propose this to the expert group.
        Hide
        dannycoward added a comment -

        There is some resistance to relaxing the rules to allow one whole and one partial message handler per connection, in part because it is more complicated to specify and explain, and in part because it only has limited usefulness. So I'm closing this, we'll keep it as it is.

        Show
        dannycoward added a comment - There is some resistance to relaxing the rules to allow one whole and one partial message handler per connection, in part because it is more complicated to specify and explain, and in part because it only has limited usefulness. So I'm closing this, we'll keep it as it is.

          People

          • Assignee:
            dannycoward
            Reporter:
            Pavel Bucek
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved: