atom feed4 messages in net.java.dev.jain-sip.usersRe: SipServerTransaction and MessageC...
FromSent OnAttachments
Frederic TardifAug 16, 2012 12:06 pm 
M. RanganathanAug 16, 2012 1:33 pm 
Jean DeruelleAug 17, 2012 1:37 am 
M. RanganathanAug 17, 2012 6:58 am 
Subject:Re: SipServerTransaction and MessageChannel interaction?
From:Jean Deruelle (jean@gmail.com)
Date:Aug 17, 2012 1:37:16 am
List:net.java.dev.jain-sip.users

I think if this can be refactored and fully tested in a dedicated branch like we did for performance optimizations, it makes sense to me. Better design leads to easier refactoring down the path, better optimizations and potentially more contributions. If we start to be fearful of doing refactoring then it means the project is on a dead path to me. The purpose of the testsuite and TCK is to make sure everything is covered (even if granted some corner cases might not be covered) and new code works and is compliant.

Jean

On Thu, Aug 16, 2012 at 10:33 PM, M. Ranganathan <mra@gmail.com> wrote:

Bon afternoon,

JAIN-SIP is an organic piece of code. You are looking closely at its warts. It is the way it looks because it just evolved that way:

- First there was a String parser. - The String parser begat a pipelined message parser - The parsers begat a message channel whereby they could send messages - Then came transactions which were kind of bolted on to message channels and adopt the same pattern as message channels so that the layer above them could be oblivious to whether the he channel was stateful or not. At least that was the theory. i.e. the transaction is a type of message channel that has a certain state machine. ( As stated in the FAQ JAIN-SIP follows a "fractal design pattern" :-) )

There is of course much improvement that can be done but I'd suggest pounding the weak spots with test cases so that if something breaks, we can fix it rather than extensive re-factoring for the sake of software hygiene.

I think though that we are safe because the transaction is the unit of event serialization. A message channel belongs to only one transaction.

Thank you for a careful analysis.

Best regards,

Ranga

(The most compelling thing about a frog is that it jumps and not that it looks pretty. )

On Thu, Aug 16, 2012 at 3:06 PM, Frederic Tardif <fred@gmail.com> wrote:

Bonjour,

As shown below, I found out the issue I was chasing (JSIP-339) regarding wrong traces logging when sending retransmitted responses though a SipServerTransaction using an unreliable transport (UdpMessageChannel).

I had quite an headache to weave my way through the implementation of the SipTransaction's and I would appreciate if someone could enlighten me with the design intended behind. Here are a couple of caveat I found out:

A SipTransaction owns an encapsulated MessageChannel.

The MessageChannel can either be associated on construction or set later on. The MessageChannel kept as a member in a TX only makes sense for reliable transport !!!

If the MessageChannel is UDP, the interior of the UdpMessageChannel will change as further unrelated requests goes. Consequently, the encapsulated messageChannel member will not reflect anymore the original connection used to define the transaction. This means all public SipTransaction methods (and even internal usage) of messageChannel are bug prone when dealing with a udp transport.

Moreover, why a SipTransaction extends MessageChannel (A extends B but also owns a B.... )??? ObjectOriented-wise, it smells bad...

This is especially error prone since a SipTransaction must expose methods like getPeerAddress() from MessageChannel. It actually implements it by doing encapsulatedMessageChannel.getPeerAddress(). But as stated above, using the encapsulated messageChannel like this will only work if the underlying transport is reliable, otherwise the result will be random according to concurrent udp traffic!!!!

For the present bug, it was only a logging issue, but when checking at all the other classes coupled with a SipTransaction using either getMessageChannel() or any other inherited MessageChannel related method, I am really worried about other ripple effects.

Unless I am missing something, there is an ObjectOriented design inconsistency in the current implementation. I don't know exactly what is the best strategy to address this but I was finding important to raise the flag.

Frederic

---------- Forwarded message ---------- From: <ftar@java.net> Date: Thu, Aug 16, 2012 at 2:11 PM Subject: [jsip~svn:2145] fix for issue : http://java.net/jira/browse/JSIP-339 To: cv@jsip.java.net

Project: jsip Repository: svn Revision: 2145 Author: ftardif Date: 2012-08-16 18:11:35 UTC Link:

Log Message:

------------ fix for issue : http://java.net/jira/browse/JSIP-339 The code was not using the right messageChannel when using unreliable transport causing to log retransmited responses using peerAddress and peerPort of an already reused UdpMessageChannel.

Revisions:

---------- 2145

Modified Paths:

--------------- trunk/src/gov/nist/javax/sip/stack/SIPServerTransaction.java

Diffs:

------ Index: trunk/src/gov/nist/javax/sip/stack/SIPServerTransaction.java =================================================================== --- trunk/src/gov/nist/javax/sip/stack/SIPServerTransaction.java (revision 2144) +++ trunk/src/gov/nist/javax/sip/stack/SIPServerTransaction.java (revision 2145) @@ -1249,17 +1249,12 @@ try { SIPResponse lastReparsedResponse = (SIPResponse)

sipStack.getMessageParserFactory().createMessageParser(sipStack).parseSIPMessage(lastResponseAsBytes,

true, false, null);

- lastReparsedResponse.setRemoteAddress( - this.getPeerInetAddress()); - lastReparsedResponse.setRemotePort - (this.getPeerPort()); - lastReparsedResponse.setLocalPort( - getMessageChannel().getPort()); - lastReparsedResponse.setLocalAddress( - getMessageChannel() - .getMessageProcessor().getIpAddress()); +

lastReparsedResponse.setRemoteAddress(messageChannel.getPeerInetAddress());

+ lastReparsedResponse.setRemotePort(messageChannel.getPeerPort()); + lastReparsedResponse.setLocalPort(messageChannel.getPort()); +

lastReparsedResponse.setLocalAddress(messageChannel.getMessageProcessor().getIpAddress());

- getMessageChannel().logMessage(lastReparsedResponse, this.getPeerInetAddress(), this.getPeerPort(), System.currentTimeMillis()); + messageChannel.logMessage(lastReparsedResponse, messageChannel.getPeerInetAddress(), messageChannel.getPeerPort(), System.currentTimeMillis()); } catch (ParseException e) { if (logger.isLoggingEnabled(LogWriter.TRACE_DEBUG)) { logger.logDebug("couldn't reparse last response " + new String(lastResponseAsBytes));