Re: [reSIProcate] Treatment of CANCEL in UAS_Accepted or UAS_AcceptedWaitingAnswer
Hi Scott,
your patch works, thanks. So far there is only minor issue detected (which does
not make any damage): When initial INVITE was sent with no SDP the ACK must
include one (Answer). In our case UAC DialogSet sends always ACK with no SDP
and UAS hits "Illegal negotiation" condition (just before being terminated
anyway).
Thanks again,
Boris
--- On Thu, 9/3/09, Boris Rozinov <borisrozinov@xxxxxxxx> wrote:
> From: Boris Rozinov <borisrozinov@xxxxxxxx>
> Subject: Re: [reSIProcate] Treatment of CANCEL in UAS_Accepted or
> UAS_AcceptedWaitingAnswer
> To: "Scott Godin" <sgodin@xxxxxxxxxxxxxxx>
> Cc: resiprocate-devel@xxxxxxxxxxxxxxx
> Received: Thursday, September 3, 2009, 4:07 PM
> Thanks a lot Scott, I'll try the
> patch you prepared
>
> --- On Wed, 9/2/09, Scott Godin <sgodin@xxxxxxxxxxxxxxx>
> wrote:
>
> > From: Scott Godin <sgodin@xxxxxxxxxxxxxxx>
> > Subject: Re: [reSIProcate] Treatment of CANCEL in
> UAS_Accepted or UAS_AcceptedWaitingAnswer
> > To: "Boris Rozinov" <borisrozinov@xxxxxxxx>
> > Cc: resiprocate-devel@xxxxxxxxxxxxxxx
> > Received: Wednesday, September 2, 2009, 6:40 PM
> > I just realized that cancel responses
> > are not routed to the
> > Dialog/DialogSet - I made a modification to the fix -
> try
> > this one
> > instead:
> >
> > Index: DialogSet.cxx
> >
> ===================================================================
> > --- DialogSet.cxx (revision 8559)
> > +++ DialogSet.cxx (working copy)
> > @@ -239,7 +239,10 @@
> > bool
> > DialogSet::handledByAuthOrRedirect(const
> SipMessage&
> > msg)
> > {
> > - if (msg.isResponse() && !(mState
> > == Terminating || mState ==
> > WaitingToEnd || mState == Destroying))
> > + if (msg.isResponse() && !(mState
> > == Terminating ||
> > +
> >
> > mState == WaitingToEnd ||
> > +
> >
> > mState == Destroying ||
> > +
> >
> > mState == Cancelling))
> > {
> > // !dcm! -- multiple usage
> > grief...only one of each method type allowed
> > if (getCreator()
> > &&
> > @@ -447,7 +450,52 @@
> > }
> > return;
> > }
> > + else if(mState == Cancelling)
> > + {
> > + assert(mDialogs.empty());
> > + if (msg.isResponse())
> > + {
> > + int code =
> > msg.header(h_StatusLine).statusCode();
> > +
> >
> switch(mCreator->getLastRequest()->header(h_CSeq).method())
> > + {
> > + case INVITE:
> > +
> > if (code / 100 == 1)
> > +
> > {
> > +
> > // do nothing - wait for final response
> > +
> > }
> > +
> > // 200/Inv crossing CANCEL case
> > +
> > else if (code / 100 == 2)
> > +
> > {
> > +
> > Dialog dialog(mDum, msg, *this);
> >
> > +
> > SharedPtr<SipMessage> ack(new SipMessage);
> > +
> > dialog.makeRequest(*ack, ACK);
> > +
> > ack->header(h_CSeq).sequence() =
> > msg.header(h_CSeq).sequence();
> > +
> > dialog.send(ack);
> > +
> > +
> > SharedPtr<SipMessage> bye(new SipMessage);
> > +
> > dialog.makeRequest(*bye, BYE);
> > +
> > dialog.send(bye);
> > +
> > +
> > // Note: Destruction of this dialog object
> > will
> > cause DialogSet::possiblyDie to be called thus
> invoking
> > mDum.destroy
> > +
> > }
> > +
> > else
> > +
> > {
> > +
> > mState = Destroying;
> > +
> > mDum.destroy(this);
> > +
> > }
> > +
> > break;
> > + }
> > + }
> > + else
> > + {
> > +
> > SharedPtr<SipMessage> response(new
> > SipMessage);
> > +
> > mDum.makeResponse(*response, msg, 481);
> > +
> > mDum.send(response);
> > + }
> > + return;
> > + }
> > +
> > if (handledByAuthOrRedirect(msg))
> > {
> > return;
> > @@ -903,26 +951,17 @@
> >
> > SharedPtr<SipMessage>
> >
> cancel(Helper::makeCancel(*getCreator()->getLastRequest()));
> > mDum.send(cancel);
> >
> > + if
> > (mDum.mDialogEventStateManager)
> > + {
> > +
> > mDum.mDialogEventStateManager->onTerminated(*this,
> > *cancel, InviteSessionHandler::LocalCancel);
> > + }
> > +
> > if (mDialogs.empty())
> > {
> > - // !jf! if
> > 200/INV crosses a CANCEL that was sent after
> receiving
> > - // non-dialog
> > creating provisional (e.g. 100), then we need to:
> > - // Add a new
> > state, if we receive a 200/INV in this state, ACK and
> > - // then send a
> > BYE and destroy the dialogset.
> > - if
> > (mDum.mDialogEventStateManager)
> > - {
> > -
> >
> mDum.mDialogEventStateManager->onTerminated(*this,
> > *cancel, InviteSessionHandler::LocalCancel);
> > - }
> > - mState =
> > Destroying;
> > -
> > mDum.destroy(this);
> > + mState =
> > Cancelling;
> > }
> > else
> > {
> > - if
> > (mDum.mDialogEventStateManager)
> > - {
> > -
> >
> mDum.mDialogEventStateManager->onTerminated(*this,
> > *cancel, InviteSessionHandler::LocalCancel);
> > - }
> > -
> > //need
> > to lag and do last element ouside of look as this
> > DialogSet will be
> >
> > //deleted if all dialogs are destroyed
> > for
> > (DialogMap::iterator it = mDialogs.begin(); it !=
> > mDialogs.end(); it++)
> > @@ -956,6 +995,7 @@
> > break;
> > }
> > case Terminating:
> > + case Cancelling:
> > case Destroying:
> > DebugLog (<<
> > "DialogSet::end() called on a DialogSet that is
> > already Terminating");
> > //assert(0);
> > Index: DialogSet.hxx
> >
> ===================================================================
> > --- DialogSet.hxx (revision 8559)
> > +++ DialogSet.hxx (working copy)
> > @@ -69,6 +69,7 @@
> > ReceivedProvisional,
> > Established,
> > Terminating,
> > +
> > Cancelling, // only used
> > when cancelling and no dialogs exist
> > Destroying
> > } State;
> >
> >
> > Scott
> >
> > On Wed, Sep 2, 2009 at 4:49 PM, Scott Godin<sgodin@xxxxxxxxxxxxxxx>
> > wrote:
> > > Here is a patch you can try out against SVN head.
> I
> > haven't had any
> > > chance to test it myself, but let me know if it
> works
> > for you:
> > >
> > > Index: DialogSet.cxx
> > >
> >
> ===================================================================
> > > --- DialogSet.cxx (revision 8559)
> > > +++ DialogSet.cxx (working copy)
> > > @@ -239,7 +239,10 @@
> > > bool
> > > DialogSet::handledByAuthOrRedirect(const
> > SipMessage& msg)
> > > {
> > > - if (msg.isResponse() && !(mState ==
> > Terminating || mState ==
> > > WaitingToEnd || mState == Destroying))
> > > + if (msg.isResponse() && !(mState ==
> > Terminating ||
> > > +
> > mState == WaitingToEnd ||
> > > +
>
> > mState == Destroying
> ||
> > > +
>
> > mState ==
> Cancelling))
> > > {
> > > // !dcm! -- multiple usage grief...only
> one
> > of each method type allowed
> > > if (getCreator() &&
> > > @@ -447,7 +450,52 @@
> > > }
> > > return;
> > > }
> > > + else if(mState == Cancelling)
> > > + {
> > > + assert(mDialogs.empty());
> > > + if (msg.isResponse())
> > > + {
> > > + int code =
> > msg.header(h_StatusLine).statusCode();
> > > +
> >
> switch(mCreator->getLastRequest()->header(h_CSeq).method())
> > > + {
> > > + case INVITE:
> > > + //
> 200/Inv
> > crossing CANCEL case
> > > + if (code / 100 == 2)
> > > + {
> > > + Dialog dialog(mDum,
> msg,
> > *this);
> > >
> > > +
> > SharedPtr<SipMessage> ack(new SipMessage);
> > > +
> dialog.makeRequest(*ack,
> > ACK);
> > > +
> > ack->header(h_CSeq).sequence() =
> > > msg.header(h_CSeq).sequence();
> > > + dialog.send(ack);
> > > +
> > > +
> > SharedPtr<SipMessage> bye(new SipMessage);
> > > +
> dialog.makeRequest(*bye,
> > BYE);
> > > + dialog.send(bye);
> > > +
> > > + // Note: Normally
> > Destruction of this dialog
> > > object will cause DialogSet::possiblyDie to be
> called
> > thus invoking
> > > mDum.destroy
> > > +
> //
> > but we don't want the dialogset to go away
> until
> > the
> > > cancel response is received. Setting
> > mReUseDialogSet
> > > +
> //
> > will cause DialogSet::possiblyDie to not
> be
> > called in
> > > the Dialog destructor
> > > +
>
> > dialog.mReUseDialogSet = true;
> > > + }
> > > + break;
> > > +
> > > + case CANCEL:
> > > + // Destroy dialog set
> when
> > cancel response is received
> > > + mState = Destroying;
> > > + mDum.destroy(this);
> > > + break;
> > > + }
> > > + }
> > > + else
> > > + {
> > > + SharedPtr<SipMessage>
> response(new
> > SipMessage);
> > > + mDum.makeResponse(*response, msg,
> 481);
> > > + mDum.send(response);
> > > + }
> > > + return;
> > > + }
> > > +
> > > if (handledByAuthOrRedirect(msg))
> > > {
> > > return;
> > > @@ -903,26 +951,17 @@
> > > SharedPtr<SipMessage>
> > >
> >
> cancel(Helper::makeCancel(*getCreator()->getLastRequest()));
> > > mDum.send(cancel);
> > >
> > > + if (mDum.mDialogEventStateManager)
> > > + {
> > > +
> >
> mDum.mDialogEventStateManager->onTerminated(*this,
> > > *cancel, InviteSessionHandler::LocalCancel);
> > > + }
> > > +
> > > if (mDialogs.empty())
> > > {
> > > - // !jf! if 200/INV crosses a
> CANCEL
> > that was sent after receiving
> > > - // non-dialog creating
> provisional
> > (e.g. 100), then we need to:
> > > - // Add a new state, if we
> receive a
> > 200/INV in this state, ACK and
> > > - // then send a BYE and destroy
> the
> > dialogset.
> > > - if
> (mDum.mDialogEventStateManager)
> > > - {
> > > -
> > mDum.mDialogEventStateManager->onTerminated(*this,
> > > *cancel, InviteSessionHandler::LocalCancel);
> > > - }
> > > - mState = Destroying;
> > > - mDum.destroy(this);
> > > + mState = Cancelling;
> > > }
> > > else
> > > {
> > > - if
> (mDum.mDialogEventStateManager)
> > > - {
> > > -
> > mDum.mDialogEventStateManager->onTerminated(*this,
> > > *cancel, InviteSessionHandler::LocalCancel);
> > > - }
> > > -
> > > //need to lag and do last
> element
> > ouside of look as this
> > > DialogSet will be
> > > //deleted if all dialogs are
> > destroyed
> > > for (DialogMap::iterator it =
> > mDialogs.begin(); it !=
> > > mDialogs.end(); it++)
> > > @@ -956,6 +995,7 @@
> > > break;
> > > }
> > > case Terminating:
> > > + case Cancelling:
> > > case Destroying:
> > > DebugLog (<<
> "DialogSet::end()
> > called on a DialogSet that is
> > > already Terminating");
> > > //assert(0);
> > > Index: DialogSet.hxx
> > >
> >
> ===================================================================
> > > --- DialogSet.hxx (revision 8559)
> > > +++ DialogSet.hxx (working copy)
> > > @@ -69,6 +69,7 @@
> > > ReceivedProvisional,
> > > Established,
> > > Terminating,
> > > + Cancelling, // only
> used
> > when cancelling and no dialogs exist
> > > Destroying
> > > } State;
> > >
> > >
> > > Scott
> > >
> > > On Wed, Sep 2, 2009 at 12:05 PM, Scott
> Godin<sgodin@xxxxxxxxxxxxxxx>
> > wrote:
> > >> Yes I agree - that is a real issue in DUM, we
> need
> > do something like
> > >> keep the DialogSet around (after
> > destruction/cancel) for some period
> > >> of time (likely 32s) in order to correctly
> handle
> > scenarios like this.
> > >>
> > >> Scott
> > >>
> > >> On Tue, Sep 1, 2009 at 5:45 PM, Boris
> > Rozinov<borisrozinov@xxxxxxxx>
> > wrote:
> > >>> Scott,
> > >>>
> > >>> I believe you are right and UAS behaves
> > correctly by just responding 200OK to Cancel. It
> brings us
> > to UAC side (in our case both UAC and UAS are
> implemented
> > using resiprocate). UAC was supposed to wait for
> 200OK/INV
> > and then respond by ACK and then send BYE. But
> > DialogSet::end() sends CANCEL and in absence of
> created
> > dialogs invokes mDum.destroy(this) and effectively
> destruct
> > itself and as result delayed (or retransmitted)
> 200OK/INV
> > are thrown away as stray response.
> > >>>
> > >>> Thanks,
> > >>> Boris
> > >>>
> > >>>
> > >>> --- On Tue, 9/1/09, Scott Godin <sgodin@xxxxxxxxxxxxxxx>
> > wrote:
> > >>>
> > >>>> From: Scott Godin <sgodin@xxxxxxxxxxxxxxx>
> > >>>> Subject: Re: [reSIProcate] Treatment
> of
> > CANCEL in UAS_Accepted or UAS_AcceptedWaitingAnswer
> > >>>> To: "Boris Rozinov" <borisrozinov@xxxxxxxx>
> > >>>> Cc: resiprocate-devel@xxxxxxxxxxxxxxx
> > >>>> Received: Tuesday, September 1, 2009,
> 1:22
> > PM
> > >>>> I believe DUM is behaving according
> > >>>> to RFC3261 - section 9.2:
> > >>>>
> > >>>> If the transaction
> > >>>> for the original request still
> > exists,
> > >>>> the behavior of the UAS on
> > >>>> receiving a CANCEL request
> depends
> > on
> > >>>> whether it has already sent a
> > >>>> final response for the
> original
> > >>>> request. If it has, the CANCEL
> > >>>> request has no effect on the
> > processing
> > >>>> of the original request, no
> > >>>> effect on any session state,
> and no
> > >>>> effect on the responses generated
> > >>>> for the original request.
> > >>>>
> > >>>> Scott
> > >>>>
> > >>>> On Mon, Aug 31, 2009 at 11:52 PM,
> Boris
> > Rozinov<borisrozinov@xxxxxxxx>
> > >>>> wrote:
> > >>>> > Hi all,
> > >>>> >
> > >>>> > Due to race condition UAS may
> receive
> > CANCEL for the
> > >>>> initial INVITE transaction in
> Accepted or
> > >>>> AcceptedWaitingAnswer state. As
> transition
> > to OnCancel state
> > >>>> occurs, 200 OK response for Cancel
> is
> > sent, but session is
> > >>>> not terminated and session handler is
> not
> > notified.
> > >>>> > Does not it make more sense to
> send
> > BYE (dialog is
> > >>>> already established), terminate
> session
> > and notify session
> > >>>> handler (or just dispatch Cancel
> treatment
> > to
> > >>>> InviteSession)?
> > >>>> >
> > >>>> > Thanks,
> > >>>> > Boris
> > >>>> >
> > >>>> >
> > >>>> >
> > >>>>
> >
> __________________________________________________________________
> > >>>> > Yahoo! Canada Toolbar: Search
> from
> > anywhere on the
> > >>>> web, and bookmark your favourite
> sites.
> > Download it now
> > >>>> > http://ca.toolbar.yahoo.com.
> > >>>> >
> > _______________________________________________
> > >>>> > resiprocate-devel mailing list
> > >>>> > resiprocate-devel@xxxxxxxxxxxxxxx
> > >>>> > https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
> > >>>> >
> > >>>>
> > >>>
> > >>>
> > >>>
> >
> __________________________________________________________________
> > >>> Looking for the perfect gift? Give the
> gift of
> > Flickr!
> > >>>
> > >>> http://www.flickr.com/gift/
> > >>>
> > >>
> > >
> >
>
>
>
> __________________________________________________________________
> Looking for the perfect gift? Give the gift of Flickr!
>
> http://www.flickr.com/gift/
>
__________________________________________________________________
The new Internet Explorer® 8 - Faster, safer, easier. Optimized for Yahoo!
Get it Now for Free! at http://downloads.yahoo.com/ca/internetexplorer/