[reSIProcate] More problems with `assert'
Daniel Pocock
daniel at readytechnology.co.uk
Mon Nov 6 17:05:41 CST 2006
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 at readytechnology.co.uk> 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 at icescape.com> 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 at list.sipfoundry.org
>> >> > [mailto:resiprocate-devel-bounces at list.sipfoundry.org] On Behalf Of
>> >> > Daniel Pocock
>> >> > Sent: Monday, November 06, 2006 9:07 AM
>> >> > To: resiprocate-devel at list.sipfoundry.org
>> >> > 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 at list.sipfoundry.org
>> >> > >>[mailto:resiprocate-devel-bounces at list.sipfoundry.org] On
>> Behalf Of
>> >> > >>Daniel Pocock
>> >> > >>Sent: Sunday, November 05, 2006 6:37 PM
>> >> > >>To: resiprocate-devel at list.sipfoundry.org
>> >> > >>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 at list.sipfoundry.org
>> >> > >>https://list.sipfoundry.org/mailman/listinfo/resiprocate-deve
>> >> > >>
>> >> > >l
>> >> > >
>> >> > >
>> >> > _______________________________________________
>> >> > resiprocate-devel mailing list
>> >> > resiprocate-devel at list.sipfoundry.org
>> >> > https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>> >> >
>> >>
>> >>
>> >> _______________________________________________
>> >> resiprocate-devel mailing list
>> >> resiprocate-devel at list.sipfoundry.org
>> >> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>> >>
>> >
>> _______________________________________________
>> resiprocate-devel mailing list
>> resiprocate-devel at list.sipfoundry.org
>> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>>
>
More information about the resiprocate-devel
mailing list