The message-decorator interface has nothing to do with the bug. The bug is that DUM is making modifications to SipMessages owned by the various subclasses of BaseUsage every time send() is called. Most of the subclasses of BaseUsage call DialogUsageManager::send() with the /exact same SipMessage every time/ (they create the SipMessage in their c'tor, and never let go of the reference, and they just increment the CSeq and call send() again with the same SharedPtr<SipMessage> when they need to send another request. This is why you see the branch param reset at DialogUsageManager.cxx:829; if it were not there, every request in a Dialog would have the same branch param on it.)
For instance, take DialogUsageManager.cxx:852; here we are adding Authentication headers. Fortunately for us, this call clears the existing auth headers before adding anything; if it did not clear the existing auth headers, we would see outgoing requests having an additional extra copy of all the auth headers for each request sent in the dialog, just like they have an extra copy of the decorator every time send is called.
Best regards, Byron Campen Byron, thanks for the clarification. I never use SipMessage::addOutoundDecorator() but rather Profile::setOutboundDecorator(SharedPtr<MessageDecorator> outboundDecorator) and let dum add the decorator itself. I guess this interface wouldn’t need to be changed as dum could create an auto_ptr-clone from this instance? Matthias From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx]
Sent: Donnerstag, 30. Oktober 2008 04:23
To: Matthias Moetje
Cc: 'Daniel Shi'; 'resiprocate-devel'; 'Jason Fischl'; 'Derek MacDonald'
Subject: Re: [reSIProcate] MessageDecorator bug? The reason we decided to move toward SipMessage::addOutboundDecorator() taking ownership is because MessageDecorator is now required to carry enough state to roll-back any changes it has made to the SipMessage; it makes sense for each SipMessage (or copy thereof) to have its own copy of the decorator. If you need a single place to keep state, you can use a subclass of MessageDecorator that has a reference to that single place. In any case, from what I can tell looking at the code, we were adding multiple copies of the decorator in DUM before the behavior was changed. The point is that DialogUsageManager::send() takes a SharedPtr<SipMessage> that is being held on to by some subclass of BaseUsage (for example, ClientPublication::mPublish), putting a decorator on it, and passing it down to the stack (which then clones the SipMessage). Meanwhile, many of the subclasses of BaseUsage hold onto that same SharedPtr<SipMessage>, with the decorator still attached, and just fiddle the CSeq and various other things, and later call DialogUsageManager::send() with the same message with the decorator already on it. DUM adds another copy, rinse, lather, repeat. What I think we need to do is write DialogUsageManager::send(auto_ptr<SipMessage>), and have DialogUsageManager::send(SharedPtr<SipMessage>) just turn around and clone the message into an auto_ptr, and call the auto_ptr version of the function. Also, having this function take an auto_ptr would mean that DUM could use SipStack::send(auto_ptr<SipMessage>), which would actually allow us to save cycles in many cases. I am using outbound decorators myself, but I haven't tried release 1.4. I don't understand why the cloning was introduced. I have a single outbound decorator which I'm holding a reference to. I guess that's the reason why it is a shared_ptr. When the STUN addresses change I know that I have this instance only where I need to change the public ip address. Not multiple instances that constantly clone themselves. I would vote to revert to the original behaviour. It works, it's tested and it's proven not to leak. Best regards, Freundliche Grüße, Matthias Moetje ______________________________________________ TERASENS GmbH Phone: +49.89.143370-0 Augustenstraße 24 Fax: +49.89.143370-22 80333 Munich e-mail: info@xxxxxxxxxxxxGERMANY Web: www.terasens.com______________________________________________ -----Original Message----- Sent: Mittwoch, 29. Oktober 2008 21:54
To: Daniel Shi
Cc: resiprocate-devel; Jason Fischl; Derek MacDonald
Subject: Re: [reSIProcate] MessageDecorator bug?
Looking at the old code, it looks like daniel set this stuff up with
SharedPtr a few years ago. Is there a reason we need to keep doing it this
way?
Best regards,
Byron Campen
Ugh. Yeah, this is completely broken.
The reason this is biting us is that DialogUsageManager is
adding the outbound decorators to long-lifetime SharedPtr<SipMessage>
that live in the various subclasses of DialogUsage. We either need to
have the subclasses set up the outbound decorators themselves, or stop
passing their outgoing traffic with a SharedPtr (ie, have DialogUsage
clone the SipMessage, put it in an auto_ptr, and fling _that_ at
DialogUsageManager). My vote is for the latter, since using SharedPtr
in this case doesn't make much sense anyway. We should be passing
auto_ptr instead.
Best regards,
Byron Campen
Hi,
I'm testing DUM using Resiprocate release 1.4 (SVN head) and have
noticed a strange behaviour about MessageDecorator invocation while
sending a SIP message.
To reproduce the issue I changed the sample application
basicRregister.cxx file (in "dum/test/directory") with some
modifications: I added my class SipMessageDecorator, invoked
setOutboundDecorator(), added logs and removed unregistration so that
the REGISTER message is always sent.
I set my MessageDecorator class in the main with
setOutboundDecorator() method, in this way:
SharedPtr<SipMessageDecorator> messageDecorator(new
SipMessageDecorator());
clientDum.getMasterProfile()->setOutboundDecorator(messageDecorator);
After sending the first REGISTER message, the method
decorateMessage()
in MessageDecorator class is called once, but the second time it is
called consecutively twice with the same message, at third REGISTER
three times (always same message) and so on.
I noticed that in DialogUsageManager the send() method adds a
MessageDecorator clone to mOutboundDecorators and the
SipMessage::callOutboundDecorator() method calls decorateMessage()
for each item in vector mOutboundDecorators, but this vector always
grows.
I attached to this mail the changed sample application that I tested
using repro and the produced log file.
Is there a bug or my mistake?
Thank you in advance.
Best regards
Andrea Chiappori
<
basicRegister
.cxx><test.log>_______________________________________________
resiprocate-devel mailing list
_______________________________________________
resiprocate-devel mailing list
|