reSIProcate

Mailing Lists

Source Code

Google Search:



< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

Re: [reSIProcate] More problems with `assert'



Derek MacDonald wrote:
The assert for line 1166 is complaining that there is a feature message that 
does not have a feature chain.  What is happening is if the
ServerAuthManager receives an CANCEL it reports that the feature is done,
even though it has an oustanding auth request. When the UserAuthInfo message 
arrives it triggers the assert as the chain has cleaned itself up.

We can do two things here:
1. Leave the assert in, and have the contract of features remain that they 
will post stray messages; features cannot call themselves done until they
have received any outstanding feature messages.
2. Have dum delete, or call a destroy method, on any DumFeatureMessage that 
does not have a feature chain.
There is also a side issue on the use of assert: perhaps it would be useful to have a configure option at compile time to decide if we want to use assert() in a strict manner, or we would prefer to use `lazy' alternative code that just logs an error and tries to carry on where genuinely feasible. I realise this may sometimes lead to memory leaks and other unpredictable behaviour, but the application developer, using profiling tools and syslog records, will often be able to pinpoint the cause of such problems anyway. Where `carry on' was not feasible, assert() would still be used regardless of whether the stack had been compiled with the `lazy' option.
Choice 2 will make writing features easier, but encourage sloppyness which could lead to bugs. However, if the default implementation of the destroy 
method is assert(0) features that don't want the weak_ptr type behaviour
will still have the assert


On 11/6/06, Daniel Pocock <daniel@xxxxxxxxxxxxxxxxxxxxx> wrote:





Jason Fischl wrote:

> I agree with Scott. If the assert is caused by a bug in the
> application (or
> even by a bug in the higher layers of dum) then we shouldn't just
> comment it
> out. We need to fix it. I suspect that the code compiled with asserts
> disabled will continue. So until we have more information, I recommend
> leaving the assert in. Any ideas what we would have to do to replicate
> the
> problem in a test? Are these the steps?
>
> 1) Send INVITE, get 407
> 2) request auth info async
> 3) Send CANCEL before response to requested auth info
>
> Jason


Definitely case (a), below.  I had actually done this test while you
were writing the email.

I just put Thread::sleep(10 seconds); at the beginning of the credential
lookup thread.  I then proceeded to make a test call, and got 100 trying
back from the stack.  After another 2 seconds I hungup (CANCEL).  8
seconds later, when the credentials arrived, assert is invoked.

>
>
>
> On 11/6/06, Scott Godin <slgodin@xxxxxxxxxxxx> wrote:
>
>>
>> Well if the problem is caused by b) then the assert is doing its job - 
>> detecting bugs in the application/stack.  : )
>>
>> If the assert can be generated by some race condition that is
>> unavoidable to applications, then I agree it should be modified to a
>> Warning or Error Log.
>>
>> Scott
>>
>> > -----Original Message-----
>> > From: resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx
>> > [mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of
>> > Daniel Pocock
>> > Sent: Monday, November 06, 2006 9:07 AM
>> > To: resiprocate-devel@xxxxxxxxxxxxxxxxxxx
>> > Subject: Re: [reSIProcate] More problems with `assert'
>> >
>> >
>> >
>> > Scott Godin wrote:
>> >
>> > >I think it would be good to understand why this is happening first - >> > >this could be a real bug. Any idea what message type is making here 
>> > and
>> > >why?
>> > >
>> > >Scott
>> > >
>> > >
>> > >
>> > The messages are UserAuthInfo
>> >
>> > This type of message is more likely to sneak through due to problems 
>> in
>> > the higher level application, although it could be related to dialogs 
>> > that fail while an asynchronous credential lookup is in progress in
>> > another thread.
>> >
>> > In my case, the stack is in a B2BUA.  The B2BUA processes
>> > `requestCredential' in an asynchronous manner, so UserAuthInfo is
>> > posted
>> > back from a different thread some time after the dialog has
commenced.
>> >
>> > I believe that either of these explanations is possible:
>> >
>> > a) the caller has sent a CANCEL before the UserAuthInfo message is
>> > posted to DUM - for instance, if the RADIUS server is a bit slow,
>> > UserAuthInfo might be delayed
>> >
>> > b) the application, for whatever reason, has posted two copies of the 
>> > UserAuthInfo message - although I have thoroughly checked for this,
it
>> > is a possibility and we probably shouldn't crash the whole stack,
>> > logging an error should be sufficient
>> >
>> > >>-----Original Message-----
>> > >>From: resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx
>> > >>[mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of 
>> > >>Daniel Pocock
>> > >>Sent: Sunday, November 05, 2006 6:37 PM
>> > >>To: resiprocate-devel@xxxxxxxxxxxxxxxxxxx
>> > >>Subject: [reSIProcate] More problems with `assert'
>> > >>
>> > >>
>> > >>
>> > >>I occasionally see this piece of code causing trouble, assert at
>> > >>approximately line 1166 of DialogUsageManager.cxx (see code below). 
>> > >>
>> > >>Is it satisfactory to change the `assert' to something less
>> > >>catastrophic, like this for example:
>> > >>
>> > >>#include <typeinfo>
>> > >>...
>> > >>if(dynamic_cast<SipMessage*>(msg.get()) == 0)
>> > >>{
>> > >>   ErrorLog(<< "discarding message of type " <<
>> > >>typeid(*(msg.get())).name() <<", transaction gone away perhaps?"); 
>> > >>   msg.release();
>> > >>   return;
>> > >>}
>> > >>
>> > >>// From DialogUsageManager.cxx:
>> > >>
>> > >>   if (tid != Data::Empty && !mIncomingFeatureList.empty())
>> > >>   {
>> > >>      FeatureChainMap::iterator it;
>> > >> //efficiently find or create FeatureChain, should prob. be a 
>> > >>utility template
>> > >>      {
>> > >>         FeatureChainMap::iterator lb =
>> > >>mIncomingFeatureChainMap.lower_bound(tid);
>> > >>         if (lb != mIncomingFeatureChainMap.end() &&
>> > >>!(mIncomingFeatureChainMap.key_comp()(tid, lb->first)))
>> > >>         {
>> > >>            it = lb;
>> > >>         }
>> > >>         else
>> > >>         {
>> > >>            assert(dynamic_cast<SipMessage*>(msg.get()));
>> > >>            it = mIncomingFeatureChainMap.insert(lb,
>> > >>FeatureChainMap::value_type(tid, new DumFeatureChain(*this,
>> > >>mIncomingFeatureList, *mIncomingTarget)));
>> > >>         }
>> > >>      }
>> > >>
>> > >>      DumFeatureChain::ProcessingResult res =
>> > >>it->second->process(msg.get());
>> > >>
>> > >>_______________________________________________
>> > >>resiprocate-devel mailing list
>> > >>resiprocate-devel@xxxxxxxxxxxxxxxxxxx
>> > >>https://list.sipfoundry.org/mailman/listinfo/resiprocate-deve
>> > >>
>> > >l
>> > >
>> > >
>> > _______________________________________________
>> > resiprocate-devel mailing list
>> > resiprocate-devel@xxxxxxxxxxxxxxxxxxx
>> > https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>> >
>>
>>
>> _______________________________________________
>> resiprocate-devel mailing list
>> resiprocate-devel@xxxxxxxxxxxxxxxxxxx
>> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>>
>
_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxxxxxx
https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel