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

Re: [reSIProcate] Treatment of CANCEL in UAS_Accepted or UAS_AcceptedWaitingAnswer


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/
>>>
>>
>