[reSIProcate] MessageDecorator bug?
Byron Campen
bcampen at estacado.net
Thu Oct 30 21:16:32 CDT 2008
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 at estacado.net]
> 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.
>
> Best regards,
> Byron Campen
>
> 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 at terasens.com
> GERMANY Web: www.terasens.com
> ______________________________________________
>
>
>
>
> -----Original Message-----
> From: resiprocate-devel-bounces at resiprocate.org [mailto:resiprocate-devel-
> bounces at resiprocate.org] On Behalf Of Byron Campen
> 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 at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20081030/bc4bd100/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2482 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20081030/bc4bd100/attachment.bin>
More information about the resiprocate-devel
mailing list