Re: [reSIProcate] Patch: Adds support to queue NITs

["Scott Godin" <slgodin@xxxxxxxxxxxx>]

Ah – I think I see what’s happening.

 

The code in the destructor is just wrong:

 

QueuedNIT *qn;

while (qn=mNITQueue.front())

{

    mNITQueue.pop();

    delete qn;

}

 

Should be re-written to be:

 

while (mNITQueue.size())

{

    delete mNITQueue.front();

    mNITQueue.pop();

}

 

mNITQueue.front() will return an undefined value if queue is empty.

 

The auto_ptr solution is a reasonable solution as well.

 

Scott

 

From: Kobi Eshun [mailto:kobi@xxxxxxxxxxxxxx]
Sent: Monday, February 04, 2008 1:32 AM
To: Scott Godin
Cc: resiprocate-devel@xxxxxxxxxxxxxxx
Subject: Re: [reSIProcate] Patch: Adds support to queue NITs

 

You're right about the copying -- I see that the InviteSession copy constructors are in fact disabled.

 

But there's definitely something rattling around in there. Here's a stack trace from a reliable crash from a test application. (Ignore the apparently different reSIProcate header/source paths -- they're a coherent set.) The app establishes a call with a UAS, fires off a bunch of in-dialog NITs, and then tears down the call. It crashes while destroying the InviteSession.

 

Replacing the raw pointers in the NIT queue with SharedPtr's fixes the problem, but I don't understand why. Any ideas?

--

kobi

 

 

Program received signal:  "EXC_BAD_ACCESS".

[Switching to process 17993 thread 0xb90f]

(gdb) where

#0  0x003a0d81 in resip::shared_count::~shared_count (this=0x2e747273) at /Users/ss-kobi/source/client/mp2/resiprocate-1.2/rutil/SharedCount.hxx:247

#1  0x003a0f34 in resip::SharedPtr<resip::SipMessage>::~SharedPtr (this=0x2e74726f) at /Users/ss-kobi/source/client/mp2/resiprocate-1.2/rutil/SharedPtr.hxx:73

#2  0x004212bd in resip::InviteSession::QueuedNIT::~QueuedNIT (this=0x2e74726f) at ../resip/dum/InviteSession.hxx:352

#3  0x001fae2a in resip::InviteSession::~InviteSession (this=0xe352200) at /Users/ss-kobi/source/resiprocate-1.2.2/resip/dum/InviteSession.cxx:86

#4  0x0044c091 in resip::ClientInviteSession::~ClientInviteSession (this=0xe352200) at ../resip/dum/ClientInviteSession.hxx:13

#5  0x002486d5 in resip::DestroyUsage::destroy (this=0xdcdf9d0) at /Users/ss-kobi/source/resiprocate-1.2.2/resip/dum/DestroyUsage.cxx:87

#6  0x001afcd5 in resip::DialogUsageManager::internalProcess (this=0xe197200, msg=@0xb02d97e4) at /Users/ss-kobi/source/resiprocate-1.2.2/resip/dum/DialogUsageManager.cxx:1123

#7  0x001b03f8 in resip::DialogUsageManager::process (this=0xe197200, mutex=0x0) at /Users/ss-kobi/source/resiprocate-1.2.2/resip/dum/DialogUsageManager.cxx:1388

#8  0x0011adf4 in SipEP::run (this=0x286f8cb0) at /Users/ss-kobi/source/client/xcode/../QvixApp/AppSrc/pstn/SipEP.cxx:3011

#9  0x00114341 in SipEP::staticSDLThread (arg=0x286f8cb0) at /Users/ss-kobi/source/client/xcode/../QvixApp/AppSrc/pstn/SipEP.cxx:817

#10 0x3002d649 in SDL_RunThread ()

#11 0x3002f1e8 in RunThread ()

#12 0x90024227 in _pthread_body ()

Current language:  auto; currently c++

 

 

 

On Feb 1, 2008, at 10:06 PM, Scott Godin wrote:

When is an InviteSession ever copied?  I don’t think this is a problem.

 

From: Kobi Eshun [mailto:kobi@xxxxxxxxxxxxxx] 
Sent: February-01-08 6:50 PM
To: Scott Godin
Cc: resiprocate-devel@xxxxxxxxxxxxxxx
Subject: Re: [reSIProcate] Patch: Adds support to queue NITs

 

Hi Scott,

 

I found an issue with the implementation: the elements of mNITQueue are not correctly duplicated on an InviteSession copy as currently implemented (my bad, not yours).

 

I changed it to be a queue of shared rather than raw pointers.

 

--

kobi

 

On Feb 1, 2008, at 9:07 AM, Scott Godin wrote:

Thanks Kobi! I think this patch is a good idea - I've had to do the same thing at the app level in many of the applications I've created.

I've taken a look at the patch and have made some modifications to my local copy - I'll give others a few days to digest and make comments, then I'll commit it. Here are the changes I've made so far:
1. Removed use of the resip Fifo to store the queued NITs and replaced this with an STL queue - the FIFO classes uses Mutex's that are just unneeded overhead.
2. In an attempt to make things a little cleaner. I renamed the checkNITQueue fn to nitComplete - this fn now sets the state to completed and checks the queue. This new fn is called in dispatchInfo, dispatchMessage on responses and on refer responses.
3. As a consequence of 2 - removed the checkNITQueue call from Dialog.cxx, when receiving a response to INFO or MESSAGE NITs.

I've attached a new version of the patch.

Thanks,
Scott

> -----Original Message-----
> From: resiprocate-devel-bounces@xxxxxxxxxxxxxxx [mailto:resiprocate-
> devel-bounces@xxxxxxxxxxxxxxx] On Behalf Of Kobi Eshun
> Sent: Thursday, January 31, 2008 7:56 PM
> To: resiprocate-devel@xxxxxxxxxxxxxxx
> Subject: [reSIProcate] Patch: Adds support to queue NITs
>
> Hi,
>
> Please consider the attached patch against HEAD. It implements queueing
> for any outgoing REFER, INFO, or MESSAGE requests associated with an
> InviteSession. If a non-INVITE transaction is already in progress when
> refer(), info() or message() is invoked, the resulting SIP request is
> pushed into a FIFO. Queued requestes are shipped out one at a time
> without overlap as final responses are processed.
>
> For INFO and MESSAGE, the new behavior is semantically equivalent to
> before (queueing notwithstanding).
>
> For REFER, the previous code locked out a new request until the
> onReferAccepted() callback fired on the first NOTIFY. The patch changes
> the end of that lockout to coincide with the final response for the
> REFER itself. The new lockout period should be shorter, and proved
> easier to code. Does anyone see a problem with this?
>
> The new queue should also work with the DUM command API, but I did not
> test it.
>
> Cheers,
> --
> kobi
>

<resiprocate_queued_NITs-scott.diff>