Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated May 22, 2014, 10:44 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

Committed in revision 414400


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs
-

  /branches/12/res/res_pjsip_refer.c 413303 
  /branches/12/res/parking/parking_bridge_features.c 413303 
  /branches/12/res/parking/parking_applications.c 413303 
  /branches/12/main/parking.c 413303 
  /branches/12/main/bridge_basic.c 413303 
  /branches/12/main/bridge.c 413303 
  /branches/12/include/asterisk/parking.h 413303 
  /branches/12/include/asterisk/bridge.h 413303 
  /branches/12/channels/sig_analog.c 413303 
  /branches/12/channels/chan_sip.c 413303 
  /branches/12/channels/chan_mgcp.c 413303 
  /branches/12/channels/chan_dahdi.c 413303 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated May 21, 2014, 11:29 a.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

Address jcolp + mjordan's findings.

I want to go ahead and point out that I found there are some conditions during 
blind transfers that will lead to the refer_progress_bridge stasis subscription 
never getting unsubscribed (bad, especially since it's holding on to references 
to other things). This is a pre-existing problem... happens with or without the 
patch in place. Basically any transfer that doesn't put the recipient in a 
bridge will cause this (transfers to IVRs for instance). One possible fix might 
be to track the subscription on a channel datastore for the transferer. I have 
a work around in place for parking so that it doesn't leak the subscription, 
but this is a problem that should probably be fixed at some point.


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/res_pjsip_refer.c 413303 
  /branches/12/res/parking/parking_bridge_features.c 413303 
  /branches/12/res/parking/parking_applications.c 413303 
  /branches/12/main/parking.c 413303 
  /branches/12/main/bridge_basic.c 413303 
  /branches/12/main/bridge.c 413303 
  /branches/12/include/asterisk/parking.h 413303 
  /branches/12/include/asterisk/bridge.h 413303 
  /branches/12/channels/sig_analog.c 413303 
  /branches/12/channels/chan_sip.c 413303 
  /branches/12/channels/chan_mgcp.c 413303 
  /branches/12/channels/chan_dahdi.c 413303 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Matt Jordan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11942
---



/branches/12/main/bridge.c
https://reviewboard.asterisk.org/r/3485/#comment21843

The 'completed flag'



/branches/12/res/res_pjsip_refer.c
https://reviewboard.asterisk.org/r/3485/#comment21846

Is there any reason why the transfer_channel_data struct wasn't just added 
to the refer_progress struct?



/branches/12/res/res_pjsip_refer.c
https://reviewboard.asterisk.org/r/3485/#comment21845

(1) Don't leave comments with your name. That's about equivalent to putting 
in issue nubmers.

(2) From the perspective of SIP, the subscription is still correct: we need 
to know when the channel is re-bridged to send the 200 OK. If the transfer goes 
into something other than a bridge, the subscription should get terminated when 
the channel is hung up.



/branches/12/res/res_pjsip_refer.c
https://reviewboard.asterisk.org/r/3485/#comment21847

This struct is the same as refer_progress_bridge_data. Why do we have two 
identical definitions?



/branches/12/res/res_pjsip_refer.c
https://reviewboard.asterisk.org/r/3485/#comment21848

This should see if the subscription is still active on the bridge and, if 
so, remove the subscription.



/branches/12/res/res_pjsip_refer.c
https://reviewboard.asterisk.org/r/3485/#comment21849

This feels very messy. We just created a wrapper struct for the 
user_data_wrapper and the refer_data-progress, bumped their ref counts ... 
then cleaned up and destroyed them.

And now we're doing it again. I think there's a way to combine the logic 
here.


- Matt Jordan


On May 21, 2014, 11:29 a.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 21, 2014, 11:29 a.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11944
---



/branches/12/res/parking/parking_bridge_features.c
https://reviewboard.asterisk.org/r/3485/#comment21851

Extra ;


- Joshua Colp


On May 21, 2014, 4:29 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 21, 2014, 4:29 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 21, 2014, 12:16 p.m., Matt Jordan wrote:
  /branches/12/res/res_pjsip_refer.c, lines 145-146
  https://reviewboard.asterisk.org/r/3485/diff/7/?file=58717#file58717line145
 
  Is there any reason why the transfer_channel_data struct wasn't just 
  added to the refer_progress struct?

At first I thought this would lead to a circular reference, but at this point 
I'm aware that no, it actually isn't.  I can just do that.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11942
---


On May 21, 2014, 11:29 a.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 21, 2014, 11:29 a.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated May 21, 2014, 1:18 p.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

Once more unto the breach -- I've simplified a lot of the changes in 
res_pjsip_refer.


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/res_pjsip_refer.c 413303 
  /branches/12/res/parking/parking_bridge_features.c 413303 
  /branches/12/res/parking/parking_applications.c 413303 
  /branches/12/main/parking.c 413303 
  /branches/12/main/bridge_basic.c 413303 
  /branches/12/main/bridge.c 413303 
  /branches/12/include/asterisk/parking.h 413303 
  /branches/12/include/asterisk/bridge.h 413303 
  /branches/12/channels/sig_analog.c 413303 
  /branches/12/channels/chan_sip.c 413303 
  /branches/12/channels/chan_mgcp.c 413303 
  /branches/12/channels/chan_dahdi.c 413303 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11947
---



/branches/12/main/bridge_basic.c
https://reviewboard.asterisk.org/r/3485/#comment21854

So you've documented this as being an ao2 object and then it's not used as 
one here. If someone else comes along and assumes this in 
ast_bridge_transfer_blind there will be problems.


- Joshua Colp


On May 21, 2014, 6:18 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 21, 2014, 6:18 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 21, 2014, 3:09 p.m., Joshua Colp wrote:
  /branches/12/main/bridge_basic.c, line 3156
  https://reviewboard.asterisk.org/r/3485/diff/8/?file=58725#file58725line3156
 
  So you've documented this as being an ao2 object and then it's not used 
  as one here. If someone else comes along and assumes this in 
  ast_bridge_transfer_blind there will be problems.

That should have been reverted... and what would have happened if I left it 
that way is that the transfer_channel_data struct would have gotten wrapped up 
in another transfer_channel_data struct.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11947
---


On May 21, 2014, 1:18 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 21, 2014, 1:18 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated May 21, 2014, 3:25 p.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

revert some flawed stuff in bridge_basic.c


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/res_pjsip_refer.c 413303 
  /branches/12/res/parking/parking_bridge_features.c 413303 
  /branches/12/res/parking/parking_applications.c 413303 
  /branches/12/main/parking.c 413303 
  /branches/12/main/bridge_basic.c 413303 
  /branches/12/main/bridge.c 413303 
  /branches/12/include/asterisk/parking.h 413303 
  /branches/12/include/asterisk/bridge.h 413303 
  /branches/12/channels/sig_analog.c 413303 
  /branches/12/channels/chan_sip.c 413303 
  /branches/12/channels/chan_mgcp.c 413303 
  /branches/12/channels/chan_dahdi.c 413303 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 21, 2014, 3:09 p.m., Joshua Colp wrote:
  /branches/12/main/bridge_basic.c, line 3156
  https://reviewboard.asterisk.org/r/3485/diff/8/?file=58725#file58725line3156
 
  So you've documented this as being an ao2 object and then it's not used 
  as one here. If someone else comes along and assumes this in 
  ast_bridge_transfer_blind there will be problems.
 
 Jonathan Rose wrote:
 That should have been reverted... and what would have happened if I left 
 it that way is that the transfer_channel_data struct would have gotten 
 wrapped up in another transfer_channel_data struct.

Went ahead and did some DTMF blind transfers to make sure I hit this code path 
this time around when retesting.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11947
---


On May 21, 2014, 3:25 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 21, 2014, 3:25 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11950
---

Ship it!


Ship It!

- Joshua Colp


On May 21, 2014, 8:25 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 21, 2014, 8:25 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 20, 2014, 8:47 a.m., Matt Jordan wrote:
  /branches/12/include/asterisk/bridge.h, line 911
  https://reviewboard.asterisk.org/r/3485/diff/6/?file=58657#file58657line911
 
  Don't use a bit field; it's overkill and you're really just going to 
  waste space in the struct anyway.
  
  Also: how is this raised? How does it get used? Right now this is 
  passed to my callback - when is this zero and when is it non-zero? Do I set 
  it, or does something else set it?

struct transfer_channel_data {
void *data;   /*! Data to be used by the transfer_channel_cb */
int ready;/*! Initially 0, This will be set to 1 by either the 
transfer
   *  code or by transfer code hooks (e.g. parking) when the
   *  transfer is completed and any remaining actions have 
taken
   *  place (e.g. parking announcements). It will never be 
reset
   *  to 0. This is used for deferring progress for channel
   *  drivers that support deferred progress. */
};


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11924
---


On May 19, 2014, 1:25 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 19, 2014, 1:25 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11927
---



/branches/12/include/asterisk/bridge.h
https://reviewboard.asterisk.org/r/3485/#comment21821

This should be documented more since it's now a wrapper. Are there any 
expectations? Should it be allocated in any specific way? In the case of 
res_pjsip_refer it expects this as an AO2 object, but that's currently an 
implementation detail of the blind transfer/parking code that is not 
documented. Could your stack usage of this be passed into the announcement 
tracker and hilarity ensue?



/branches/12/include/asterisk/bridge.h
https://reviewboard.asterisk.org/r/3485/#comment21819

I also don't think ready is appropriate here. completed would be better 
since the transfer operation has fully completed when it is set.



/branches/12/include/asterisk/parking.h
https://reviewboard.asterisk.org/r/3485/#comment21820

You're adding this so make the name right. parked_channel_data. This 
applies to elsewhere too.



/branches/12/res/parking/parking_bridge_features.c
https://reviewboard.asterisk.org/r/3485/#comment21823

What value does this add over just using transfer_channel_data directly?



/branches/12/res/parking/parking_bridge_features.c
https://reviewboard.asterisk.org/r/3485/#comment21824

Add some documentation that this stasis message handling will block if it 
is tracking the announcement. This could be important in the future if how 
message dispatch occurs changes.



/branches/12/res/res_pjsip_refer.c
https://reviewboard.asterisk.org/r/3485/#comment21822

Can you further elaborate on this? Why did you add this? What problem was 
there? What does it solve?


- Joshua Colp


On May 19, 2014, 6:25 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 19, 2014, 6:25 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 20, 2014, 11 a.m., Joshua Colp wrote:
  /branches/12/include/asterisk/bridge.h, lines 906-908
  https://reviewboard.asterisk.org/r/3485/diff/6/?file=58657#file58657line906
 
  This should be documented more since it's now a wrapper. Are there any 
  expectations? Should it be allocated in any specific way? In the case of 
  res_pjsip_refer it expects this as an AO2 object, but that's currently an 
  implementation detail of the blind transfer/parking code that is not 
  documented. Could your stack usage of this be passed into the announcement 
  tracker and hilarity ensue?

Yeah, that's true. The data field for this wrapper is only valid during the 
transfer_channel_cb itself and not for anything that it spins off for instance. 
Trying to access it at that point could be pretty bad. I'll make a note of that 
in the documentation.

It will always be allocated as an AO2 object. That way transfer_channel_cb 
functions can bump the ref and track so that they can track the 'completed' 
variable in another function.

/*!
 * \brief AO2 object that wraps data for transfer_channel_cb
 */
struct transfer_channel_data {
void *data;/*! Data to be used by the transfer_channel_cb -- note 
that this
*  pointer is going to be pointing to something on the 
stack, so
*  it must not be used at any point after returning 
from the
*  transfer_channel_cb. */
int completed; /*! Initially 0, This will be set to 1 by either the 
transfer
*  code or by transfer code hooks (e.g. parking) when 
the
*  transfer is completed and any remaining actions have 
taken
*  place (e.g. parking announcements). It will never be 
reset
*  to 0. This is used for deferring progress for channel
*  drivers that support deferred progress. */
};


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11927
---


On May 19, 2014, 1:25 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 19, 2014, 1:25 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 20, 2014, 11 a.m., Joshua Colp wrote:
  /branches/12/res/parking/parking_bridge_features.c, lines 58-82
  https://reviewboard.asterisk.org/r/3485/diff/6/?file=58663#file58663line58
 
  What value does this add over just using transfer_channel_data directly?

Probably nothing. In an earlier version of the patch this was used for tracking 
a mutex/condition so that I could block the return of the main transfer 
function and I just never thought to clean it up. I'll take care of this in a 
while.


 On May 20, 2014, 11 a.m., Joshua Colp wrote:
  /branches/12/res/res_pjsip_refer.c, line 519
  https://reviewboard.asterisk.org/r/3485/diff/6/?file=58664#file58664line519
 
  Can you further elaborate on this? Why did you add this? What problem 
  was there? What does it solve?

Ok, so now that I'm looking through this again, I see that this approach is 
totally wrong, but here's the basic problem...

the transfer_channel_cb applies a subscription here:
/* We also will need to detect if the transferee enters a 
bridge. This is currently the only reliable way to
 * detect if the transfer target has answered the call
 */
refer-progress-bridge_sub = 
stasis_subscribe(ast_bridge_topic_all(), refer_progress_bridge, 
refer-progress);


This subscription listens for bridge messages and if the channel bridge matches 
the channel being tracked, then a 200 notify is pushed. Obvious in the case of 
parking this is bad since the channel is bridged before the announcement 
completes and this will end up terminating the call before the announcement is 
over.

My silly ham-fisted fix for this essentially made refer_progress_bridge just 
kill itself... so if I were going to continue on that approach, I'd have to 
remove refer_progress_bridge entirely. At the moment, I'm thinking the thing to 
do is just to add a reference to the transfer_channel_data so that it can also 
track whether or not completed has been set.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11927
---


On May 19, 2014, 1:25 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 19, 2014, 1:25 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Matt Jordan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11915
---



/branches/12/include/asterisk/bridge.h
https://reviewboard.asterisk.org/r/3485/#comment21793

Terminal does not feel like a good name for this parameter.

What's more, in most blind transfers, media is never played to the 
transferer. The context of this is definitely not clear from the description of 
the function.

The fact that we have a single parameter - which ripples throughout the 
_entire_ codebase - merely to work around chan_sip feels like a mistake. 
Ideally, the blind transfer routine would be smart enough to bump the reference 
count of the channel to prevent it getting nuked before attempting the playback 
and - if the channel is marked to be hung up - not attempt the playback in the 
first place.


- Matt Jordan


On May 16, 2014, 5:02 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 16, 2014, 5:02 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/manager.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_skinny.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_oss.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_iax2.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 19, 2014, 9:11 a.m., Matt Jordan wrote:
  /branches/12/include/asterisk/bridge.h, lines 951-953
  https://reviewboard.asterisk.org/r/3485/diff/5/?file=58486#file58486line951
 
  Terminal does not feel like a good name for this parameter.
  
  What's more, in most blind transfers, media is never played to the 
  transferer. The context of this is definitely not clear from the 
  description of the function.
  
  The fact that we have a single parameter - which ripples throughout the 
  _entire_ codebase - merely to work around chan_sip feels like a mistake. 
  Ideally, the blind transfer routine would be smart enough to bump the 
  reference count of the channel to prevent it getting nuked before 
  attempting the playback and - if the channel is marked to be hung up - not 
  attempt the playback in the first place.

There is no way to prevent the playback based on the channel's current hangup 
state. The hangup state generally hasn't changed by the time we enter the 
announcement function. The blind transfer itself isn't what initiates the 
hangup. It's the response we get from the phone to the SIP notify after 
returning from everything in the bridge blind transfer code. At best we'd end 
up with a race condition between the announcement subscription and hanging up 
on the BYE we get from the phones.

This stuff worked with chan_sip in Asterisk 11 because Asterisk 11's chan_sip 
had logic specifically in place to handle deferring the final notify (the one 
that triggers the BYE from phones) when parking completed.


Perhaps it would be more palpable if instead of adding this to flag to the API 
call, I instead added it to the transfer channel data wrapper. As for a name 
change, how about just 'disable_media'?

Alternatively, I can start investigating what it would take to be able to defer 
the final notify of chan_sip blind transfers. I know you said we don't want to 
go down that road right now, but it's basically the only real way of getting 
this to behave like it behaved in Asterisk 11.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11915
---


On May 16, 2014, 5:02 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 16, 2014, 5:02 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/manager.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_skinny.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_oss.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_iax2.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 19, 2014, 9:11 a.m., Matt Jordan wrote:
  /branches/12/include/asterisk/bridge.h, lines 951-953
  https://reviewboard.asterisk.org/r/3485/diff/5/?file=58486#file58486line951
 
  Terminal does not feel like a good name for this parameter.
  
  What's more, in most blind transfers, media is never played to the 
  transferer. The context of this is definitely not clear from the 
  description of the function.
  
  The fact that we have a single parameter - which ripples throughout the 
  _entire_ codebase - merely to work around chan_sip feels like a mistake. 
  Ideally, the blind transfer routine would be smart enough to bump the 
  reference count of the channel to prevent it getting nuked before 
  attempting the playback and - if the channel is marked to be hung up - not 
  attempt the playback in the first place.
 
 Jonathan Rose wrote:
 There is no way to prevent the playback based on the channel's current 
 hangup state. The hangup state generally hasn't changed by the time we enter 
 the announcement function. The blind transfer itself isn't what initiates the 
 hangup. It's the response we get from the phone to the SIP notify after 
 returning from everything in the bridge blind transfer code. At best we'd end 
 up with a race condition between the announcement subscription and hanging up 
 on the BYE we get from the phones.
 
 This stuff worked with chan_sip in Asterisk 11 because Asterisk 11's 
 chan_sip had logic specifically in place to handle deferring the final notify 
 (the one that triggers the BYE from phones) when parking completed.
 
 
 Perhaps it would be more palpable if instead of adding this to flag to 
 the API call, I instead added it to the transfer channel data wrapper. As for 
 a name change, how about just 'disable_media'?
 
 Alternatively, I can start investigating what it would take to be able to 
 defer the final notify of chan_sip blind transfers. I know you said we don't 
 want to go down that road right now, but it's basically the only real way of 
 getting this to behave like it behaved in Asterisk 11.

On second thought, I can't really move the variable to the wrapper either since 
the transfer channel callback data isn't wrapped until it is already in the 
bridging function. Meh.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11915
---


On May 16, 2014, 5:02 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 16, 2014, 5:02 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/manager.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_skinny.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_oss.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_iax2.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle 

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 19, 2014, 9:11 a.m., Matt Jordan wrote:
  /branches/12/include/asterisk/bridge.h, lines 951-953
  https://reviewboard.asterisk.org/r/3485/diff/5/?file=58486#file58486line951
 
  Terminal does not feel like a good name for this parameter.
  
  What's more, in most blind transfers, media is never played to the 
  transferer. The context of this is definitely not clear from the 
  description of the function.
  
  The fact that we have a single parameter - which ripples throughout the 
  _entire_ codebase - merely to work around chan_sip feels like a mistake. 
  Ideally, the blind transfer routine would be smart enough to bump the 
  reference count of the channel to prevent it getting nuked before 
  attempting the playback and - if the channel is marked to be hung up - not 
  attempt the playback in the first place.
 
 Jonathan Rose wrote:
 There is no way to prevent the playback based on the channel's current 
 hangup state. The hangup state generally hasn't changed by the time we enter 
 the announcement function. The blind transfer itself isn't what initiates the 
 hangup. It's the response we get from the phone to the SIP notify after 
 returning from everything in the bridge blind transfer code. At best we'd end 
 up with a race condition between the announcement subscription and hanging up 
 on the BYE we get from the phones.
 
 This stuff worked with chan_sip in Asterisk 11 because Asterisk 11's 
 chan_sip had logic specifically in place to handle deferring the final notify 
 (the one that triggers the BYE from phones) when parking completed.
 
 
 Perhaps it would be more palpable if instead of adding this to flag to 
 the API call, I instead added it to the transfer channel data wrapper. As for 
 a name change, how about just 'disable_media'?
 
 Alternatively, I can start investigating what it would take to be able to 
 defer the final notify of chan_sip blind transfers. I know you said we don't 
 want to go down that road right now, but it's basically the only real way of 
 getting this to behave like it behaved in Asterisk 11.
 
 Jonathan Rose wrote:
 On second thought, I can't really move the variable to the wrapper either 
 since the transfer channel callback data isn't wrapped until it is already in 
 the bridging function. Meh.

Ah, but on the other hand... I could set the variable in the 
transfer_channel_cb since that gets executed before the parked channel is put 
into the bridge anyway. That solves that problem.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11915
---


On May 16, 2014, 5:02 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 16, 2014, 5:02 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/manager.c 413303 
   /branches/12/main/bridge_basic.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/include/asterisk/bridge.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_skinny.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_oss.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_iax2.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen 

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated May 19, 2014, 1:25 p.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

Removed chan_sip workaround stuff that ravaged the bridging API.
Note that one API change remains - blind transfer cb must now expect blind 
transfer data wrapper rather than a void pointer. mjordan suggested that I 
could do some preprocessor magic to deal with this, but I figure it's probably 
better to get the review back on track before I delve too deeply into that.

I also cleaned up some inconsistencies for failure conditions in the multi 
party parking code, at least one of which might have ended with a leaked 
channel reference.


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/res_pjsip_refer.c 413303 
  /branches/12/res/parking/parking_bridge_features.c 413303 
  /branches/12/res/parking/parking_applications.c 413303 
  /branches/12/main/parking.c 413303 
  /branches/12/main/bridge_basic.c 413303 
  /branches/12/main/bridge.c 413303 
  /branches/12/include/asterisk/parking.h 413303 
  /branches/12/include/asterisk/bridge.h 413303 
  /branches/12/channels/sig_analog.c 413303 
  /branches/12/channels/chan_sip.c 413303 
  /branches/12/channels/chan_mgcp.c 413303 
  /branches/12/channels/chan_dahdi.c 413303 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 14, 2014, 6:57 a.m., Joshua Colp wrote:
  /branches/12/include/asterisk/parking.h, line 167
  https://reviewboard.asterisk.org/r/3485/diff/4/?file=58327#file58327line167
 
  The naming of this as new_channel_cb confuses me...

This is a carried over from the name it has in bridge.h - I haven't changed 
this yet, but I guess I'll change it to parked_channel_cb.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11887
---


On May 8, 2014, 12:40 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 8, 2014, 12:40 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated May 16, 2014, 5:02 p.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

The old patch blocked the PJSIP monitor thread which is... horrible. This new 
one doesn't.


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/res_pjsip_refer.c 413303 
  /branches/12/res/parking/parking_bridge_features.c 413303 
  /branches/12/res/parking/parking_applications.c 413303 
  /branches/12/main/parking.c 413303 
  /branches/12/main/manager.c 413303 
  /branches/12/main/bridge_basic.c 413303 
  /branches/12/main/bridge.c 413303 
  /branches/12/include/asterisk/parking.h 413303 
  /branches/12/include/asterisk/bridge.h 413303 
  /branches/12/channels/sig_analog.c 413303 
  /branches/12/channels/chan_skinny.c 413303 
  /branches/12/channels/chan_sip.c 413303 
  /branches/12/channels/chan_oss.c 413303 
  /branches/12/channels/chan_mgcp.c 413303 
  /branches/12/channels/chan_iax2.c 413303 
  /branches/12/channels/chan_dahdi.c 413303 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11887
---



/branches/12/include/asterisk/parking.h
https://reviewboard.asterisk.org/r/3485/#comment21770

The naming of this as new_channel_cb confuses me...



/branches/12/res/parking/parking_bridge_features.c
https://reviewboard.asterisk.org/r/3485/#comment21772

This seems like a rather arbitrary amount of time. Will it always be long 
enough?



/branches/12/res/parking/parking_bridge_features.c
https://reviewboard.asterisk.org/r/3485/#comment21771

It is possible for this to wake up early so you need to put it in a while 
loop which checks conditions. Same for the below duplicated code.


- Joshua Colp


On May 8, 2014, 5:40 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 8, 2014, 5:40 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated May 8, 2014, 12:40 p.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

This new patch delays execution of the new_channel_cb as well as returning from 
the parking function when a new_channel_cb is specified until after the parking 
announcement is complete. Digium phones were able to listen to those 
announcements and the old patch was forcing them to hang up immediately.


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/res_pjsip_refer.c 413303 
  /branches/12/res/parking/parking_bridge_features.c 413303 
  /branches/12/res/parking/parking_applications.c 413303 
  /branches/12/main/parking.c 413303 
  /branches/12/main/bridge.c 413303 
  /branches/12/include/asterisk/parking.h 413303 
  /branches/12/channels/sig_analog.c 413303 
  /branches/12/channels/chan_sip.c 413303 
  /branches/12/channels/chan_mgcp.c 413303 
  /branches/12/channels/chan_dahdi.c 413303 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Joshua Colp]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11861
---



/branches/12/res/res_pjsip_refer.c
https://reviewboard.asterisk.org/r/3485/#comment21741

I need time to grok your other changes and the possible repercussions but 
I'll comment on this right now. Would this get called twice since the framehook 
is invoked when detached?


- Joshua Colp


On May 8, 2014, 5:40 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 8, 2014, 5:40 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 8, 2014, 3:22 p.m., Joshua Colp wrote:
  /branches/12/res/res_pjsip_refer.c, lines 185-193
  https://reviewboard.asterisk.org/r/3485/diff/4/?file=58332#file58332line185
 
  I need time to grok your other changes and the possible repercussions 
  but I'll comment on this right now. Would this get called twice since the 
  framehook is invoked when detached?

Yep, it's hitting it twice.  Guess I need to make sure it doesn't push a second 
notify.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11861
---


On May 8, 2014, 12:40 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 8, 2014, 12:40 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

 On May 8, 2014, 3:22 p.m., Joshua Colp wrote:
  /branches/12/res/res_pjsip_refer.c, lines 185-193
  https://reviewboard.asterisk.org/r/3485/diff/4/?file=58332#file58332line185
 
  I need time to grok your other changes and the possible repercussions 
  but I'll comment on this right now. Would this get called twice since the 
  framehook is invoked when detached?
 
 Jonathan Rose wrote:
 Yep, it's hitting it twice.  Guess I need to make sure it doesn't push a 
 second notify.

I'm not sure if this is the proper and elegant way to do this, but I just added 
a check for if the event value is AST_FRAMEHOOK_EVENT_DETACHED and if it is, 
I'm returning early right now.  It fixes the behavior, but it might be cleaner 
if I just put the whole thing in as a check for if the event type is ATTACHED 
seeing as this is to handle hangups prior to actually attaching the frame hook 
and all.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11861
---


On May 8, 2014, 12:40 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated May 8, 2014, 12:40 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 413303 
   /branches/12/res/parking/parking_bridge_features.c 413303 
   /branches/12/res/parking/parking_applications.c 413303 
   /branches/12/main/parking.c 413303 
   /branches/12/main/bridge.c 413303 
   /branches/12/include/asterisk/parking.h 413303 
   /branches/12/channels/sig_analog.c 413303 
   /branches/12/channels/chan_sip.c 413303 
   /branches/12/channels/chan_mgcp.c 413303 
   /branches/12/channels/chan_dahdi.c 413303 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated April 29, 2014, 10:47 a.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

Missed a couple calls to ast_parking_blind_transfer_park in the last review.  
This should be all of them.


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/parking/parking_bridge_features.c 413071 
  /branches/12/main/parking.c 413071 
  /branches/12/main/bridge.c 413071 
  /branches/12/include/asterisk/parking.h 413071 
  /branches/12/channels/sig_analog.c 413071 
  /branches/12/channels/chan_mgcp.c 413071 
  /branches/12/channels/chan_dahdi.c 413071 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Mark Michelson]

 On April 28, 2014, 5:14 p.m., Matt Jordan wrote:
  /branches/12/res/res_pjsip_refer.c, lines 898-906
  https://reviewboard.asterisk.org/r/3485/diff/1/?file=57945#file57945line898
 
  This feels like the wrong way to solve this problem. Having a 'parked' 
  variable bleed up into this handling feels like we've got a design flaw in 
  how we're handling parking.
  
  refer_progress_bridge *should* be detecting that the channel entered 
  into a holding bridge. That should be sending the 200 notification. If that 
  isn't happening, then there is some other bug here that this solution is 
  masking.
  
 

The problem here is that the stasis subscription that sets up 
refer_progress_bridge never gets the opportunity to get set up. 
refer_blind_callback() is called on the local channel that ends up running 
dialplan during a blind transfer. When transferring to parking, such a local 
channel is typically not created since the transferee channel is moved directly 
from its current bridge into the parking bridge. Thus all the progress-tracking 
code is bypassed.

It was my suggestion to use a separate return value to indicate a successful 
transfer had occurred due to being parked (or more generically, the transfer 
was successful and was entirely complete). There's no bug that the solution is 
masking, it's just that parking bypasses the typical blind transfer process, 
and so it has to be taken into account. The problem is that transfers to 
parking require special handling, and that unfortunately bleeds out to others.


- Mark


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11766
---


On April 25, 2014, 10:48 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated April 25, 2014, 10:48 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 412824 
   /branches/12/main/manager.c 412824 
   /branches/12/main/bridge_basic.c 412824 
   /branches/12/main/bridge.c 412824 
   /branches/12/include/asterisk/bridge.h 412824 
   /branches/12/channels/chan_unistim.c 412824 
   /branches/12/channels/chan_skinny.c 412824 
   /branches/12/channels/chan_sip.c 412824 
   /branches/12/channels/chan_oss.c 412824 
   /branches/12/channels/chan_iax2.c 412824 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11770
---


I've got a new patch in the works for this that adds the transfer_channel_cb 
stuff to the parking function. I've tested it with two-party bridges so far and 
it seems to work fine without having to add special logic to the consumers of 
the blind transfer function for handling parked calls.

- Jonathan Rose


On April 25, 2014, 5:48 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated April 25, 2014, 5:48 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/res_pjsip_refer.c 412824 
   /branches/12/main/manager.c 412824 
   /branches/12/main/bridge_basic.c 412824 
   /branches/12/main/bridge.c 412824 
   /branches/12/include/asterisk/bridge.h 412824 
   /branches/12/channels/chan_unistim.c 412824 
   /branches/12/channels/chan_skinny.c 412824 
   /branches/12/channels/chan_sip.c 412824 
   /branches/12/channels/chan_oss.c 412824 
   /branches/12/channels/chan_iax2.c 412824 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/
---

(Updated April 28, 2014, 2:10 p.m.)


Review request for Asterisk Developers, Matt Jordan and Mark Michelson.


Changes
---

Ok, this changes the angle of approach significantly. Instead of making an 
exception for parking as far as the PJSIP logic goes, we forward the 
new_channel_cb into the parking blind transfer function and use it on whichever 
channel is going to get parked (either local or the one being transferred). 
I've now tested this on both two party bridges and multi party bridges and it 
seems to work without fail.


Repository: Asterisk


Description
---

If a PJSIP endpoint attempts to blind transfer to a parking extension, there is 
an override to the normal transfer logic that can make things act a little 
weird. We noticed that this would leave various phones hanging on transfer 
screens without progressing. When the transfer was considered successful, PJSIP 
deferred the actual action of sending the 200 notify and the actual trigger for 
that happening never occurs when the transfer is to a parking extension.

In order to handle this, the bridge function that handles blind transfers now 
returns a different value if a call was parked and if the channel driver needs 
to react differently in this case, it can.  In the case of PJSIP, we respond to 
transfers to park by immediately sending the notify with 200 OK sip frag 
instead of deferring the action.


Diffs (updated)
-

  /branches/12/res/parking/parking_bridge_features.c 413071 
  /branches/12/main/parking.c 413071 
  /branches/12/main/bridge.c 413071 
  /branches/12/include/asterisk/parking.h 413071 

Diff: https://reviewboard.asterisk.org/r/3485/diff/


Testing
---

Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
it either manually hangs up or 60 seconds pass and Asterisk terminates the 
session.

After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
screen and goes back to idle mode.


Thanks,

Jonathan Rose

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11771
---

Ship it!


Yes, I like this much better! Much better than what I had originally suggested.

- Mark Michelson


On April 28, 2014, 7:10 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated April 28, 2014, 7:10 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/parking/parking_bridge_features.c 413071 
   /branches/12/main/parking.c 413071 
   /branches/12/main/bridge.c 413071 
   /branches/12/include/asterisk/parking.h 413071 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Jonathan Rose]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11772
---



/branches/12/res/parking/parking_bridge_features.c
https://reviewboard.asterisk.org/r/3485/#comment21577

Oops, accidentally used a tab instead of a space here. It's fixed in my 
local copy.


- Jonathan Rose


On April 28, 2014, 2:10 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated April 28, 2014, 2:10 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/parking/parking_bridge_features.c 413071 
   /branches/12/main/parking.c 413071 
   /branches/12/main/bridge.c 413071 
   /branches/12/include/asterisk/parking.h 413071 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Matt Jordan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11774
---

Ship it!


Ship It!

- Matt Jordan


On April 28, 2014, 2:10 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated April 28, 2014, 2:10 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/parking/parking_bridge_features.c 413071 
   /branches/12/main/parking.c 413071 
   /branches/12/main/bridge.c 413071 
   /branches/12/include/asterisk/parking.h 413071 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3485: pjsip: Fix a bug where transferring to a parking extension causes calls to hang

[Matt Jordan]

 On April 28, 2014, 12:14 p.m., Matt Jordan wrote:
  /branches/12/res/res_pjsip_refer.c, lines 898-906
  https://reviewboard.asterisk.org/r/3485/diff/1/?file=57945#file57945line898
 
  This feels like the wrong way to solve this problem. Having a 'parked' 
  variable bleed up into this handling feels like we've got a design flaw in 
  how we're handling parking.
  
  refer_progress_bridge *should* be detecting that the channel entered 
  into a holding bridge. That should be sending the 200 notification. If that 
  isn't happening, then there is some other bug here that this solution is 
  masking.
  
 
 
 Mark Michelson wrote:
 The problem here is that the stasis subscription that sets up 
 refer_progress_bridge never gets the opportunity to get set up. 
 refer_blind_callback() is called on the local channel that ends up running 
 dialplan during a blind transfer. When transferring to parking, such a local 
 channel is typically not created since the transferee channel is moved 
 directly from its current bridge into the parking bridge. Thus all the 
 progress-tracking code is bypassed.
 
 It was my suggestion to use a separate return value to indicate a 
 successful transfer had occurred due to being parked (or more generically, 
 the transfer was successful and was entirely complete). There's no bug that 
 the solution is masking, it's just that parking bypasses the typical blind 
 transfer process, and so it has to be taken into account. The problem is that 
 transfers to parking require special handling, and that unfortunately bleeds 
 out to others.

Okay, that makes sense now how we ended up on this solution, but I agree with 
your later comment - I like the new approach much better. Keeping consumers 
unaware of whether or not the destination happens to be the craziness that is 
parking is a win.


- Matt


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3485/#review11766
---


On April 28, 2014, 2:10 p.m., Jonathan Rose wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/3485/
 ---
 
 (Updated April 28, 2014, 2:10 p.m.)
 
 
 Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 If a PJSIP endpoint attempts to blind transfer to a parking extension, there 
 is an override to the normal transfer logic that can make things act a little 
 weird. We noticed that this would leave various phones hanging on transfer 
 screens without progressing. When the transfer was considered successful, 
 PJSIP deferred the actual action of sending the 200 notify and the actual 
 trigger for that happening never occurs when the transfer is to a parking 
 extension.
 
 In order to handle this, the bridge function that handles blind transfers now 
 returns a different value if a call was parked and if the channel driver 
 needs to react differently in this case, it can.  In the case of PJSIP, we 
 respond to transfers to park by immediately sending the notify with 200 OK 
 sip frag instead of deferring the action.
 
 
 Diffs
 -
 
   /branches/12/res/parking/parking_bridge_features.c 413071 
   /branches/12/main/parking.c 413071 
   /branches/12/main/bridge.c 413071 
   /branches/12/include/asterisk/parking.h 413071 
 
 Diff: https://reviewboard.asterisk.org/r/3485/diff/
 
 
 Testing
 ---
 
 Before patch:
 * Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until 
 it either manually hangs up or 60 seconds pass and Asterisk terminates the 
 session.
 
 After the patch:
 * Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer 
 screen and goes back to idle mode.
 
 
 Thanks,
 
 Jonathan Rose
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev