[SIPSERVLET_SPEC-39] Javadocs unclear for SipServletMessage.getRemoteAddr and getRemotePort Created: 04/Mar/14  Updated: 28/Nov/14  Resolved: 28/Nov/14

Status: Resolved
Project: sipservlet-spec
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0-pfd

Type: Improvement Priority: Minor
Reporter: keith-lewis Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 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.



 Comments   
Comment by keith-lewis [ 04/Mar/14 ]

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

Comment by binod [ 14/Aug/14 ]

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.

Comment by binod [ 07/Oct/14 ]

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

Comment by keith-lewis [ 07/Oct/14 ]

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.

Comment by binod [ 08/Oct/14 ]

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.

Comment by keith-lewis [ 08/Oct/14 ]

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.

Comment by binod [ 08/Oct/14 ]

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?

Comment by keith-lewis [ 13/Oct/14 ]

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.

Comment by binod [ 15/Oct/14 ]

<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?

Comment by keith-lewis [ 16/Oct/14 ]

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.

Comment by keith-lewis [ 16/Oct/14 ]

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

Comment by binod [ 28/Nov/14 ]

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.

Generated at Sat Apr 25 22:58:39 UTC 2015 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.