Re: [reSIProcate] Treatment of CANCEL in UAS_Accepted or UAS_AcceptedWaitingAnswer
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/