[asterisk-dev] [Code Review] 2543: Gulp blind and attended transfer support
Joshua Colp
reviewboard at asterisk.org
Thu May 16 19:00:25 CDT 2013
> On May 16, 2013, 9:43 p.m., Mark Michelson wrote:
> > In general, there is a huge lack of logging in this review (one count of ast_log that I could find). Transfers are always destined to be the place where bugs will occur most. Therefore, this should be absolutely riddled with log messages. Way more than you think you should need. We'll be glad they're there once people start reporting transfer bugs.
I've added lots of debug messages, and some error/warning messages.
> On May 16, 2013, 9:43 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/channels/chan_gulp.c, line 856
> > <https://reviewboard.asterisk.org/r/2543/diff/2/?file=37944#file37944line856>
> >
> > Did you ever run any tests where you called the Ringing() application and then the Transfer() application?
> >
> > I suspect that if you did that, you might end up with a case where you end up sending a 302 with two Contact headers. The reason is that pjsip_inv_end_session() will reuse the latest request/response that was sent. If the previous response sent had a Contact header, then I suspect that adding a new one will just tack on a second one.
> >
> > I suggest erasing any Contact header already present in the packet before adding the new Contact header to the response.
Instead of removing it I just overwrite the URI portion if present, this saves a bit of memory.
> On May 16, 2013, 9:43 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/res/res_sip_refer.c, lines 110-113
> > <https://reviewboard.asterisk.org/r/2543/diff/2/?file=37946#file37946line110>
> >
> > This has some flaws. notification->state is the state of the subscription when this task was queued, not the actual current state. So just because notification->state is not TERMINATED, it doesn't guarantee that the synchronous task from the state change callback isn't currently running. Thus the deadlock can still occur.
> >
> > Second, the deadlock described isn't necessarily going to occur even if the TERMINATED state is accurately detected because this callback can run in either the refer_progress's serializer or a session's serializer.
> >
> > So let's think through this a bit.
> >
> > Right now, a refer_progress gets its own serializer allocated. This is used for sending progress notifications and for setting the progress's subscription NULL when the subscription is terminated.
> >
> > Well, consider this. What if the refer_progress just used a reference to the session serializer from the session on which the REFER was received? The progress notifications are sent in the same dialog, so it makes sense to do this.
> >
> > This way, the evsub state change to TERMINATED will happen in one of two cases
> >
> > 1) An incoming BYE terminates the subscription
> > 2) The REFER subscription times out
> >
> > In case 1) The evsub state change callback will run in the session serializer. Since this is the same serializer where event notifications are sent, you won't have to push a synchronous task to set the progress->sub NULL. You can just set it in place.
> >
> > For case 2, I think you could just push an asynchronous task that does everything that your state change callback does now, to include setting progress->sub NULL.
SO! A dialog serializer is tied to a session and INVITE session, when the INVITE session is disconnected the dialog serializer is removed and is no longer enforced. Changing this becomes complicated with managing the lifetime of that serializer. That's why I took the approach of using a serializer just for this. This ensures that the evsub pointer, when present, remains valid. How? When the evsub is terminated (and the evsub presumably going to be destroyed) a synchronous task is queued which sets the evsub pointer to NULL. Before doing this the dialog lock is released so other pushed tasks *can* invoke evsub operations. The evsub structure is guaranteed to be valid. Once the synchronous task is complete (and evsub pointer set to NULL) any further tasks will not act on the evsub and stuff finishes.
> On May 16, 2013, 9:43 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/res/res_sip_refer.c, lines 260-262
> > <https://reviewboard.asterisk.org/r/2543/diff/2/?file=37946#file37946line260>
> >
> > Since these messages are sent in the session's dialog, why not use the session's serializer?
See previous comments.
> On May 16, 2013, 9:43 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/res/res_sip_refer.c, lines 250-253
> > <https://reviewboard.asterisk.org/r/2543/diff/2/?file=37946#file37946line250>
> >
> > This return point causes a return value of NULL to be ambiguous.
> >
> > Callers of refer_progress_alloc() assume that a NULL return means that progress was not requested (i.e. no error), but it can also indicate that a failure occurred when attempting to allocate the progress structure (i.e. an error).
I erred on this on purpose to give a better chance for the requested action (transfer) to be completed successfully, despite the inability to allocate some memory and monitor its progress. I'm fine with changing this to fail if you think so.
> On May 16, 2013, 9:43 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/res/res_sip_refer.c, lines 629-632
> > <https://reviewboard.asterisk.org/r/2543/diff/2/?file=37946#file37946line629>
> >
> > This comment seems out of place since it isn't by a synchronous task and we are operating in the session's serializer here.
Since this is a new session it isn't yet sent to the session serializer (hrm, should we make the code do that?)
> On May 16, 2013, 9:43 p.m., Mark Michelson wrote:
> > /team/group/bridge_construction/res/res_sip_refer.c, lines 298-299
> > <https://reviewboard.asterisk.org/r/2543/diff/2/?file=37946#file37946line298>
> >
> > This use of "transferee" is non-standard and liable to cause confusion. This session is actually also the transferer, but it is bridged with the transfer target rather than the transferee.
Can you suggest an alternate?
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2543/#review8636
-----------------------------------------------------------
On May 15, 2013, 6:32 p.m., Joshua Colp wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2543/
> -----------------------------------------------------------
>
> (Updated May 15, 2013, 6:32 p.m.)
>
>
> Review request for Asterisk Developers and Mark Michelson.
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> These changes add support for the following:
>
> 1. Blind transfers originating from a remote device
> 2. Blind transfers originated to a remote device
> 3. Redirection originated to a remote device (before session has been answered)
> 4. Attended transfers originating from a remote device when both legs are local
> 5. Attended transfers originating from a remote device when only one leg is local (additional dialplan logic required)
> 6. Replacement of a session using INVITE with replaces
> 7. Transfer progress notification using a frame hook
> 8. norefersub support
>
> * This patch relies on pimp my SIP, bridge_construction, and more_transfer.
>
>
> Diffs
> -----
>
> /team/group/bridge_construction/res/res_sip_refer.c PRE-CREATION
> /team/group/bridge_construction/include/asterisk/res_sip_session.h 388524
> /team/group/bridge_construction/channels/chan_gulp.c 388524
> /team/group/bridge_construction/res/res_sip_session.c 388524
> /team/group/bridge_construction/res/res_sip_session.exports.in 388524
>
> Diff: https://reviewboard.asterisk.org/r/2543/diff/
>
>
> Testing
> -------
>
> Testing the various scenarios using SIPP and phones.
>
>
> Thanks,
>
> Joshua Colp
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130517/fbd4766f/attachment-0001.htm>
More information about the asterisk-dev
mailing list