sipservlet-spec
  1. sipservlet-spec
  2. SIPSERVLET_SPEC-39

Javadocs unclear for SipServletMessage.getRemoteAddr and getRemotePort

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0-pfd
    • Labels:
      None

      Description

      The "Returns" part of the Javadoc for getRemoteAddr, getRemotePort is confusing since it asserts that the result will be null for local messages which is in conflict with the description of the method immediately above. In the case of getRemoteAddr 15.7 of the spec states

      MUST return address of the remote SIP interface if the request was received from an external entity but if the request was internally routed (from one application to the next on the same container) then it MUST return the address of the container's SIP interface

      So the value null must NOT be used for an internally routed message.

      A similar problem exists for getRemotePort.

      The only scenario in which an incoming message should have a null value for the remoteAddr (or -1 for remotePort) is a container generated error response. In this case the message has not been "sent" and so it seems reasonable that information about the local and remote address/port and the transport would be null values (or -1 for the port). In fact this may be the only way for the application to distinguish a container generated response from those received from an app or externally. It would be helpful if the Javadoc page explicitly mentioned this scenario.

        Activity

        keith-lewis created issue -
        Hide
        keith-lewis added a comment -

        There is also a question as to what these method should return for a message which has been created by an application but not yet sent.

        We suggest that in 2.0 the spec should mandate the following behaviour :

        The container should initialise these fields in the message to null values when the object is created. When the message is sent they should be set as indicated in section 15.7 of the 1.1 spec

        Show
        keith-lewis added a comment - There is also a question as to what these method should return for a message which has been created by an application but not yet sent. We suggest that in 2.0 the spec should mandate the following behaviour : The container should initialise these fields in the message to null values when the object is created. When the message is sent they should be set as indicated in section 15.7 of the 1.1 spec
        Hide
        binod added a comment -

        The apis in question exist from 1.0 onwards, when application composition was not there. The original idea was to provide connection details of the messages that are coming to the container from an external entity. So, for all other messages (those created by application or container within the container) these apis return null or -1 as applicable. The spec used "locally generated" term to describe those messages that are created within the container.

        During 1.1, when application chaining was introduced, getRemoteAddr and getRemotePort apis were enhanced to return the containers address/port when a message is received by an application in the chain. 1.1 spec also introduced apis (getInitialXXX) to return original remote address and remote port for the incoming message to all application.

        As we discussed yesterday, that is the interpretation I have about these apis.

        Show
        binod added a comment - The apis in question exist from 1.0 onwards, when application composition was not there. The original idea was to provide connection details of the messages that are coming to the container from an external entity. So, for all other messages (those created by application or container within the container) these apis return null or -1 as applicable. The spec used "locally generated" term to describe those messages that are created within the container. During 1.1, when application chaining was introduced, getRemoteAddr and getRemotePort apis were enhanced to return the containers address/port when a message is received by an application in the chain. 1.1 spec also introduced apis (getInitialXXX) to return original remote address and remote port for the incoming message to all application. As we discussed yesterday, that is the interpretation I have about these apis.
        Hide
        binod added a comment - - edited

        Hopefully updating the javadoc to say that "locally generated" mean "locally generated within the container" would clarify the javadoc.

        Show
        binod added a comment - - edited Hopefully updating the javadoc to say that "locally generated" mean "locally generated within the container" would clarify the javadoc.
        Hide
        keith-lewis added a comment - - edited

        I don't think that adding "within the container" helps.

        The text as it is seems OK for getLocalAddr, getLocalPort, getTransport, getInitialRemoteAddr, getInitialRemotePort and getInitialTransport. For all of these the value seen for an incoming message will be non-null for a message received from an external entity and null (or -1) for a message generated locally. Going back to the spec I can now see that the Javadoc is not misleading for values, I included them in the description of this JIRA due to a mis-reading of the spec.

        The only problem is for getRemoteAddr and getRemotePort. For getRemoteAddr it would be more helpful to say.

        Returns: the IP address of the sender of this message, or null for a container generated error response

        the text for getRemotePort would be similar. For requests there are no scenarios where an incoming message has a null (-1) value.

        The other change I suggest (for all the transport properties) is that we should add text to say "this method should only be called for an incoming message, in other cases its result is undefined". In 6.5 the spec says "These methods have meaning for incoming messages only". The Javadoc should not imply that there is a well defined value for these properties in the case of an outgoing message since the spec does not require this.

        Show
        keith-lewis added a comment - - edited I don't think that adding "within the container" helps. The text as it is seems OK for getLocalAddr, getLocalPort, getTransport, getInitialRemoteAddr, getInitialRemotePort and getInitialTransport. For all of these the value seen for an incoming message will be non-null for a message received from an external entity and null (or -1) for a message generated locally. Going back to the spec I can now see that the Javadoc is not misleading for values, I included them in the description of this JIRA due to a mis-reading of the spec. The only problem is for getRemoteAddr and getRemotePort. For getRemoteAddr it would be more helpful to say. Returns: the IP address of the sender of this message, or null for a container generated error response the text for getRemotePort would be similar. For requests there are no scenarios where an incoming message has a null (-1) value. The other change I suggest (for all the transport properties) is that we should add text to say "this method should only be called for an incoming message, in other cases its result is undefined". In 6.5 the spec says "These methods have meaning for incoming messages only". The Javadoc should not imply that there is a well defined value for these properties in the case of an outgoing message since the spec does not require this.
        Hide
        binod added a comment -

        Why do we need a different treatment for getRemoteAddr/getRemotePort. Please take a look at https://sipservlet-spec.java.net/javadoc/1.0/final/javax/servlet/sip/SipServletMessage.html#getRemoteAddr%28%29 for the description before the application composition was introduced in 1.1. The text remains exactly the same for transport related interfaces.

        Show
        binod added a comment - Why do we need a different treatment for getRemoteAddr/getRemotePort. Please take a look at https://sipservlet-spec.java.net/javadoc/1.0/final/javax/servlet/sip/SipServletMessage.html#getRemoteAddr%28%29 for the description before the application composition was introduced in 1.1. The text remains exactly the same for transport related interfaces.
        Hide
        keith-lewis added a comment -

        As Binod says in his comment of 14th August - the functionality for getRemoteAddr and getRemotePort was changed between 1.0 and 1.1
        There was no change to the Javadoc in 1.1 for these methods - I think that these should have changed and that this was an omission on the part of the 1.1 authors.
        We should use the opportunity to correct this in 2.0 and bring the Javadoc in line with the spec.

        Show
        keith-lewis added a comment - As Binod says in his comment of 14th August - the functionality for getRemoteAddr and getRemotePort was changed between 1.0 and 1.1 There was no change to the Javadoc in 1.1 for these methods - I think that these should have changed and that this was an omission on the part of the 1.1 authors. We should use the opportunity to correct this in 2.0 and bring the Javadoc in line with the spec.
        Hide
        binod added a comment -

        All of my comments indicate that that with 1.1 the javadoc has been modified to take care of application composition. To reconfirm,

        Here is the 1.0 javadoc for getRemoteAddr
        ---------------------------------------------------
        <snip>

        Returns the IP address of the sender of this message.

        Returns:
        a String containing the IP address of the sender of this message, or null if it was locally generated

        </snip>

        1.1 javadoc for getRemoteAddr
        -----------------------------------------
        <snip>
        Returns the IP address of the next upstream/downstream hop from which this message was received. Applications can determine the actual IP address of the UA that originated the message from the message Via header fields.
        If the message was internally routed (from one application to the next within the same container), then this method returns the address of the container's SIP interface.
        Returns:
        a String containing the IP address of the sender of this message, or null if it was locally generated
        </snip>

        So, with application composition, it is explained that "after" a message is internally routed, it returns the address of containers SIP interface (which is the sender).

        So, I still dont understand the suggestion that return value should be null only for "container generated error response". For example, if there is just one application in the server and no chaining is involved and application create a request within the container (eg: in an http servlet), what should getRemoteAddr return?

        Show
        binod added a comment - All of my comments indicate that that with 1.1 the javadoc has been modified to take care of application composition. To reconfirm, Here is the 1.0 javadoc for getRemoteAddr --------------------------------------------------- <snip> Returns the IP address of the sender of this message. Returns: a String containing the IP address of the sender of this message, or null if it was locally generated </snip> 1.1 javadoc for getRemoteAddr ----------------------------------------- <snip> Returns the IP address of the next upstream/downstream hop from which this message was received. Applications can determine the actual IP address of the UA that originated the message from the message Via header fields. If the message was internally routed (from one application to the next within the same container), then this method returns the address of the container's SIP interface. Returns: a String containing the IP address of the sender of this message, or null if it was locally generated </snip> So, with application composition, it is explained that "after" a message is internally routed, it returns the address of containers SIP interface (which is the sender). So, I still dont understand the suggestion that return value should be null only for "container generated error response". For example, if there is just one application in the server and no chaining is involved and application create a request within the container (eg: in an http servlet), what should getRemoteAddr return?
        Hide
        keith-lewis added a comment -

        The extracts Binod has included above illustrate that although the main body of the entry for getRemoteAddr was changed between 1.0 and 1.1 the "Returns" part was not changed. I believe that it should have been changed since the statement as it stands is not true - internally generated messages which are passed along an application composition chain will NOT result in a null result at the receiving application. Many readers of the Javadocs will use the main body of the entry to resolve the conflict but others will be left confused.

        The same is problem exists for getRemotePort.

        I am also suggesting that the Javadoc text should focus on what is returned for incoming messages since the spec states that these fields are only meaningful for incoming messages (see 6.5). For a request arriving at an application there are only 2 possibilities (a) the previous hop was an external entity (b) the previous hop was an internal application. In neither case will the remoteAddr be null. For a response there is a 3rd possibility - it can be a container generated error response; this is the only case which would result in a null value.

        The spec does not define the result when a UAC application calls getRemoteAddr on a message that it has generated since this is not an incoming message.

        Show
        keith-lewis added a comment - The extracts Binod has included above illustrate that although the main body of the entry for getRemoteAddr was changed between 1.0 and 1.1 the "Returns" part was not changed. I believe that it should have been changed since the statement as it stands is not true - internally generated messages which are passed along an application composition chain will NOT result in a null result at the receiving application. Many readers of the Javadocs will use the main body of the entry to resolve the conflict but others will be left confused. The same is problem exists for getRemotePort. I am also suggesting that the Javadoc text should focus on what is returned for incoming messages since the spec states that these fields are only meaningful for incoming messages (see 6.5). For a request arriving at an application there are only 2 possibilities (a) the previous hop was an external entity (b) the previous hop was an internal application. In neither case will the remoteAddr be null. For a response there is a 3rd possibility - it can be a container generated error response; this is the only case which would result in a null value. The spec does not define the result when a UAC application calls getRemoteAddr on a message that it has generated since this is not an incoming message.
        Hide
        binod added a comment -

        <snip>
        These methods have meaning for incoming messages only, in which case they return the IP address/port
        number on which the message was received locally ( getLocalAddr, getLocalPort ), or the IP
        address/port number of the sender of the message ( getRemoteAddr, getRemotePort,
        getInitialRemoteAddr, getInitialRemotePort ), as well as the transport protocol used, e.g.
        UDP, TCP, TLS, or SCTP.
        </snip>

        Spec 6.5 states the above. My interpretation is that, if the message is not "incoming" (either from the external entity or from the previous hop in the application chain) then these methods are not useful and hence the methods would return null or -1 for any message that is not incoming. Restricting the return value of null or -1 for only container generated responses would be a backward incompatible change. Repeating my earlier question: If we make this suggested change and if there is just one application in the server and no chaining is involved and application create a request within the container (eg: in an http servlet), what should getRemoteAddr return?

        Show
        binod added a comment - <snip> These methods have meaning for incoming messages only, in which case they return the IP address/port number on which the message was received locally ( getLocalAddr, getLocalPort ), or the IP address/port number of the sender of the message ( getRemoteAddr, getRemotePort, getInitialRemoteAddr, getInitialRemotePort ), as well as the transport protocol used, e.g. UDP, TCP, TLS, or SCTP. </snip> Spec 6.5 states the above. My interpretation is that, if the message is not "incoming" (either from the external entity or from the previous hop in the application chain) then these methods are not useful and hence the methods would return null or -1 for any message that is not incoming. Restricting the return value of null or -1 for only container generated responses would be a backward incompatible change. Repeating my earlier question: If we make this suggested change and if there is just one application in the server and no chaining is involved and application create a request within the container (eg: in an http servlet), what should getRemoteAddr return?
        keith-lewis made changes -
        Field Original Value New Value
        Summary Javadocs unclear for SipServletMessage.getRemoteAddr and related methods Javadocs unclear for SipServletMessage.getRemoteAddr and getRemotePort
        keith-lewis made changes -
        Description The term "locally generated" is used in the Javadocs but not defined. See getLocalAddr, getLocalPort, getRemoteAddr, getRemotePort, getTransport, getInitialRemoteAddr, getInitialRemotePort, getInitialTransport. In the case of getRemoteAddr 15.7 of the spec states

        MUST return address of the remote SIP interface if the request was received from an external entity but if the request was internally routed (from one application to the next on the same container) then it MUST return the address of the container's SIP interface

        So the value null must NOT be used for an internally routed message, hence the term locally generated is NOT equivalent to locally routed.

        One interpretation which make sense is to take the term "locally generated" to apply to container generated responses passed to an application. In this case the message has not been "sent" and so it seems reasonable that information about the local and remote address/port and the transport would be null values (or -1 for the port). In fact this may be the only way for the application to distinguish a container generated response from those received from an app or externally.
        The "Returns" part of the Javadoc for getRemoteAddr, getRemotePort is confusing since it asserts that the result will be null for local messages which is in conflict with the description of the method immediately above. In the case of getRemoteAddr 15.7 of the spec states

        MUST return address of the remote SIP interface if the request was received from an external entity but if the request was internally routed (from one application to the next on the same container) then it MUST return the address of the container's SIP interface

        So the value null must NOT be used for an internally routed message.

        A similar problem exists for getRemotePort.

        The only scenario in which an incoming message should have a null value for the remoteAddr (or -1 for remotePort) is a container generated error response. In this case the message has not been "sent" and so it seems reasonable that information about the local and remote address/port and the transport would be null values (or -1 for the port). In fact this may be the only way for the application to distinguish a container generated response from those received from an app or externally. It would be helpful if the Javadoc page explicitly mentioned this scenario.
        Hide
        keith-lewis added a comment -

        I am not saying that containers should change their behaviour for an outgoing UAC message.
        If they used to return null they can continue to do so.
        I am saying that the spec does not currently mandate a particular value for the remoteAddr and remotePort properties in that scenario so app developers should not rely on it.

        If you think that we should mandate that null must be returned for outgoing messages then we should alter section 6.5 of the spec.
        Containers would need to clone the message before setting the remoteAddr and remotePort properties to guarantee that the fields as seen by the UAC application keep their null (-1) value.
        Not sure that we want to make that change but it does have some benefits.

        Show
        keith-lewis added a comment - I am not saying that containers should change their behaviour for an outgoing UAC message. If they used to return null they can continue to do so. I am saying that the spec does not currently mandate a particular value for the remoteAddr and remotePort properties in that scenario so app developers should not rely on it. If you think that we should mandate that null must be returned for outgoing messages then we should alter section 6.5 of the spec. Containers would need to clone the message before setting the remoteAddr and remotePort properties to guarantee that the fields as seen by the UAC application keep their null (-1) value. Not sure that we want to make that change but it does have some benefits.
        Hide
        keith-lewis added a comment -

        I have changed the Description of this JIRA to make clear that the only issue is with remoteAddr and remotePort.

        Show
        keith-lewis added a comment - I have changed the Description of this JIRA to make clear that the only issue is with remoteAddr and remotePort.
        Hide
        binod added a comment -

        Here is the updated text for section 6.5.

        These methods return the IP address/port number of the SIP interface on which the message was received (getLocalAddr, getLocalPort), the IP address/port number of the sender of the message (getRemoteAddr, getRemotePort, getInitialRemoteAddr, getInitialRemotePort), as well as the transport protocol used, e.g., UDP, TCP, TLS, or SCTP (getTransport). Note that these methods have meaning for only incoming SIP messages. For all other messages,such as application created outgoing messages and container generated responses, these methods return null (or -1 for ports). See section 18.6 Transport Information for the impact of application composition on these methods

        Javadoc also is updated to reflect this.

        Show
        binod added a comment - Here is the updated text for section 6.5. These methods return the IP address/port number of the SIP interface on which the message was received (getLocalAddr, getLocalPort), the IP address/port number of the sender of the message (getRemoteAddr, getRemotePort, getInitialRemoteAddr, getInitialRemotePort), as well as the transport protocol used, e.g., UDP, TCP, TLS, or SCTP (getTransport). Note that these methods have meaning for only incoming SIP messages. For all other messages,such as application created outgoing messages and container generated responses, these methods return null (or -1 for ports). See section 18.6 Transport Information for the impact of application composition on these methods Javadoc also is updated to reflect this.
        binod made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        binod made changes -
        Fix Version/s 2.0-pfd [ 17068 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            keith-lewis
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: