[sip-comm-dev] Re: svn commit: r4884 - trunk/src/net/java/sip/communicator/impl/protocol/sip


#1

Hi Sébastien,

The introduced code in ProtocolProviderServiceSipImpl.getContactHeader(SipURI) makes two consecutive calls to the new method getContactAddressCustomParamValue() and it seems a single call would suffice. Moreover, the method being called isn't a trivial field getter. I'd personally stick with a single call because it should be both faster and should produce less garbage. What do you think?

Best regards,
Lubo

smazy@dev.java.net wrote:

···

Author: smazy
Date: 2009-01-08 00:29:57+0000
New Revision: 4884

Modified:
   trunk/src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java
   trunk/src/net/java/sip/communicator/impl/protocol/sip/SipStackSharing.java

Log:
SIP: adds a "registering_acc" param in some contact addresses

Sets a custom "registering_acc" param in the contact address of
registrar accounts and use it in the dispatching of incoming requests.

Modified: trunk/src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java
Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java?view=diff&rev=4884&p1=trunk/src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java&p2=trunk/src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java&r1=4883&r2=4884

--- trunk/src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java (original)
+++ trunk/src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java 2009-01-08 00:29:57+0000
@@ -1024,6 +1024,17 @@
              contactURI.setTransportParam(srcListeningPoint.getTransport());
             contactURI.setPort(srcListeningPoint.getPort());
+
+ // set a custom param to ease incoming requests dispatching in case
+ // we have several registrar accounts with the same username
+ if (getContactAddressCustomParamValue() != null)
+ {
+ contactURI.setParameter(
+ SipStackSharing.CONTACT_ADDRESS_CUSTOM_PARAM_NAME,
+ getContactAddressCustomParamValue()
+ );
+ }
+
             Address contactAddress = addressFactory.createAddress( contactURI );
              if (ourDisplayName != null)
@@ -1047,6 +1058,30 @@
     }
      /**
+ * Returns null for a registraless account, a value for the contact address
+ * custom parameter otherwise. This will help the dispatching of incoming
+ * requests between accounts with the same username. For address-of-record
+ * user@example.com, the returned value woud be example_com.
+ *
+ * @return null for a registraless account, a value for the
+ * "registering_acc" contact address parameter otherwise
+ */
+ public String getContactAddressCustomParamValue()
+ {
+ SipRegistrarConnection src = getRegistrarConnection();
+ if (src != null && !src.isRegistrarless())
+ {
+ // if we don't replace the dots in the hostname, we get
+ // "476 No Server Address in Contacts Allowed"
+ // from certain registrars (ippi.fr for instance)
+ String hostValue = ((SipURI) src.getAddressOfRecord().getURI())
+ .getHost().replace('.', '_');
+ return hostValue;
+ }
+ return null;
+ }
+
+ /**
      * Returns the AddressFactory used to create URLs ans Address objects.
      *
      * @return the AddressFactory used to create URLs ans Address objects.

Modified: trunk/src/net/java/sip/communicator/impl/protocol/sip/SipStackSharing.java
Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/protocol/sip/SipStackSharing.java?view=diff&rev=4884&p1=trunk/src/net/java/sip/communicator/impl/protocol/sip/SipStackSharing.java&p2=trunk/src/net/java/sip/communicator/impl/protocol/sip/SipStackSharing.java&r1=4883&r2=4884

--- trunk/src/net/java/sip/communicator/impl/protocol/sip/SipStackSharing.java (original)
+++ trunk/src/net/java/sip/communicator/impl/protocol/sip/SipStackSharing.java 2009-01-08 00:29:57+0000
@@ -35,6 +35,15 @@
     implements SipListener
{
     /**
+ * We set a custom parameter in the contact address for registrar accounts,
+ * so as to ease dispatching of incoming requests in case several accounts
+ * have the same username in their contact address, eg:
+ * sip:username@192.168.0.1:5060;transport=udp;registering_acc=example_com
+ */
+ public static final String CONTACT_ADDRESS_CUSTOM_PARAM_NAME
+ = "registering_acc";
+
+ /**
      * Logger for this class.
      */
     private static final Logger logger
@@ -607,13 +616,30 @@
                 return perfectMatch;
             }
- // past this point, our guess is not reliable
- // we try to find the "least worst" match based on parameters
- // like the To field
-
             // more than one account match
             if(candidates.size() > 1)
             {
+ // check if a custom param exists in the contact
+ // address (set for registrar accounts)
+ for (ProtocolProviderServiceSipImpl candidate : candidates)
+ {
+ String hostValue = ((SipURI) requestURI).getParameter(
+ SipStackSharing.CONTACT_ADDRESS_CUSTOM_PARAM_NAME);
+ if (hostValue == null)
+ continue;
+ if (hostValue.equals(candidate
+ .getContactAddressCustomParamValue()))
+ {
+ logger.trace("Will dispatch to \""
+ + candidate.getAccountID() + "\" because "
+ + "\" the custom param was set");
+ return candidate;
+ }
+ }
+
+ // Past this point, our guess is not reliable. We try to find
+ // the "least worst" match based on parameters like the To field
+
                 // check if the To header field host part
                 // matches any of our SIP hosts
                 for(ProtocolProviderServiceSipImpl candidate : candidates)
@@ -656,7 +682,7 @@
             // fallback on any account
             ProtocolProviderServiceSipImpl target =
                 currentListeners.iterator().next();
- logger.warn("Will randomly dispatch to \"" + target.getAccountID()
+ logger.info("Will randomly dispatch to \"" + target.getAccountID()
                 + "\" because the username in the Request-URI "
                 + "is unknown or empty");
             logger.trace("\n" + request);

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: commits-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#2

Hi Lubomir,

The introduced code in
ProtocolProviderServiceSipImpl.getContactHeader(SipURI) makes two
consecutive calls to the new method
getContactAddressCustomParamValue() and it seems a single call would
suffice. Moreover, the method being called isn't a trivial field
getter. I'd personally stick with a single call because it should be
both faster and should produce less garbage. What do you think?

You're right. We could also cache the result as it's always the same
(too bad one cannot create local static variables in Java). I checked
and we don't allow for a change in the AOR once an account is created.

May I commit this?

src/net/java/sip/communicator/impl/protocol/sip/ProtocolProviderServiceSipImpl.java
@@ -198,6 +198,11 @@ public class ProtocolProviderServiceSipImpl
     private SipStatusEnum sipStatusEnum;

     /**
+ * To be used by getContactAddressCustomParamValue() only (cache).
+ */
+ private String contactAddressCustomParamValue = null;

···

On Thu, Jan 08, 2009 at 11:01:37AM +0200, Lubomir Marinov wrote:
+
+ /**
      * Returns the AccountID that uniquely identifies the account represented by
      * this instance of the ProtocolProviderService.
      * @return the id of the account represented by this provider.
@@ -1068,6 +1073,9 @@ public class ProtocolProviderServiceSipImpl
      */
     public String getContactAddressCustomParamValue()
     {
+ if (this.contactAddressCustomParamValue != null)
+ return this.contactAddressCustomParamValue;
+
             SipRegistrarConnection src = getRegistrarConnection();
             if (src != null && !src.isRegistrarless())
             {

Cheers,

--
Sébastien Mazy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#3

On second thought, not a so good idea. We might as well allow a change
of the AOR in the future.

I commited your suggestion in r4887. Thanks!

Cheers,

···

On Thu, Jan 08, 2009 at 11:19:03AM +0100, Sébastien Mazy wrote:

You're right. We could also cache the result as it's always the same
(too bad one cannot create local static variables in Java). I checked
and we don't allow for a change in the AOR once an account is created.

--
Sébastien Mazy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net