< 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


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/