[Home]

Summary:ASTERISK-13367: [patch] h exten getting run at the wrong time
Reporter:jmls (jmls)Labels:
Date Opened:2009-01-14 12:34:31.000-0600Date Closed:2009-01-28 14:56:04.000-0600
Priority:BlockerRegression?No
Status:Closed/CompleteComponents:Resources/res_features
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 14241_redirect_no_bridgeCDR_or_h_exten_via_async_goto
( 1) 14241_redirect_no_bridgeCDR_or_h_exten_via_transfer
( 2) cdr.bxfer.txt
Description:in 1.4 svn, If A calls B, and A is redirected via AMI to another extension (meetme room for example), the h extension is being run on A before it is redirected.

The h extension should be run on A when A hangs up.

****** STEPS TO REPRODUCE ******

A calls B;  B answers; An ami redirect is performed to put A in a Meetme conf.

The AMI command to do this:

Action: Redirect
Channel: Dahdi/1-1
Context: jmls_parking
exten: park
Priority: 1


The jmls_parking context in the dialplan:

context jmls_parking
{
       park => {
               Meetme(853,dMqx1);
               /* goto jmls_parked,park,1; */
       }
       h => {
               NoOp(In the h-exten for jmls_parking);
               NoOp(channel is ${CHANNEL});
               NoOp(myvar is ${CDR(myvar)} and accountcode is ${CDR(accountcode)} billsec is ${CDR(billsec)} and duration is ${CDR(duration)} and end is ${CDR(end)}.);
       }
}


****** ADDITIONAL INFORMATION ******

*long* chat with codefreeze on asterisk-dev. He hinted that he wanted this bug raised ;)

Before this fix, the h-exten would be run in the bridge code, when the redirect took place, given unsavory results in the terms of correctness.
Comments:By: Steve Murphy (murf) 2009-01-14 13:45:41.000-0600

OK, I've attached the 14241_redirect_no_bridgeCDR_or_h_exten_via_async_goto
patch; it is horribly intrusive, but I need to see just how intrusive it is.
The patch has a few debug logs that I'll remove before I commit; but I have a feeling I will definitely have to reduce the scope of this change, because,
if every behavior mod it introduces is an improvement, it will still most likely be perceived as an unwelcome change.

I advise using this in conjunction with the patch on 13892. In unanswered=yes mode, you will see a CDR covering the time A is in the meetme when "B calls A,
A answers, then the AMI redirect is given that shifts A to a meetme conf".

I tested this with both "A calls B, then A is redirected via AMI" and
                       "B calls A, then A is redirected via AMI".

This patch affects the first case, and does not touch the second.

It allows the h-exten to be run for A, when A hangs up, rather than when
A is vectored to the meetme.
By: Steve Murphy (murf) 2009-01-14 13:52:12.000-0600

Wail away on testing this, jmls. But don't forget 13892; but 13892 isn't required, either, so it's up to you.

By: jmls (jmls) 2009-01-15 14:34:30.000-0600

patch has applied, checking now.
By: Steve Murphy (murf) 2009-01-19 11:22:23.000-0600

I attached the notes I took while testing to see where the async_goto mod might affect blind and attended xfers. Good news: blind xfers and attended xfers don't run the modified code (so far). It did turn up some anomalies in the h exten execution code, in a few cases specific to the method used, where the h-exten
is run on the wrong channel... I tested several call sequences that included blind and attended xfers, with 3-ways, mostly with dahdi phones, using features and hookflashes; The sip channel needs to be tested also.

Put the notes here lest I forget to put them *somewhere*. Here seemed appropriate.
By: Steve Murphy (murf) 2009-01-20 14:17:23.000-0600

OK, I took some time and ran around the code base and looked at just how many different places this patch could affect, and it made me nervous. So, I generated a new patch where the patch is localized to the Redirect handler in the manager code. I tested it and it looks good. I made the same mods to the app_channelredirect app, which is basically the same operation. I also went after the one bad (obviously bad, that is) case where, with the one-touch blind transfer (builtin_blindtransfer()) you get two h extens run on the same channel (but at different times). Now, just one gets run. Since changing the h-exten run time changes how the CDR is generated, I did a CDR swap operation to give reasonable results (just as good/bad as before).

So, if people find h-exten probs with the fax extension in chan_dahdi, or chan_misdn, or chan_vpb, then I have a quick fix. The same for the console_transfer func in the chan_oss; the transfer command handler in chan_iax2 and chan_sip-- then I have a possible quick fix.

So, jmls, if you have some time, take a spin with the 14241_redirect_no_bridgeCDR_or_h_exten_via_transfer file I attached above!
By: jmls (jmls) 2009-01-21 17:02:52.000-0600

Have applied the patch, will be testing tomorrow . Will let you know asap. Thanks!
By: jmls (jmls) 2009-01-23 13:06:27.000-0600

10's of thousands of calls, there doesn't seem to be any side-effects. Thanks !
By: Jason Haltom (somej) 2009-01-23 14:56:50.000-0600

Would this patch also fix an issue where the h exten is run when called party picks up after being called through the local channel? (Asterisk version 1.4.22-rc5)

Example: party A calls party B. (both parties have local extensions on the box)
Due to needing a separate copy of the CDR for each party as well as each channel needing its own set of variables, the callee must be run through the local channel before the dial macro can be run. (hopefully that makes since.)

What I am seeing is that when A calls B and when B picks up, the h exten for B is run but the call remains in tact.  When B or A hangs up, the h exten for B is not run, and no CDR for B is every generated.  Everything functions properly for A.  Also I have noticed that for these local to local calls, B is not receiving its accountID from the sip config.

These are all sip to sip calls.

pseudo of the dialplan:
local caller makes the call
is this a local to local call (local to local meaning both extensions are on the box)
no: dial(sip/trunk to external provider/did)
yes: dial(local/did) -> now call looks like an “inbound call” and is treated like all calls that originate off network and are run through the dial macro to process things before calling the actual exten.

If I need to submit a new bug for this let me know.
I have tried calling ForkCDR() before executing the dial(local/did) command and still no CDR from the person being called.

Just wandering if this patch deals with this any.
By: Digium Subversion (svnbot) 2009-01-28 12:51:09.000-0600

Repository: asterisk
Revision: 172030

U   branches/1.4/apps/app_channelredirect.c
U   branches/1.4/include/asterisk/channel.h
U   branches/1.4/main/manager.c
U   branches/1.4/main/pbx.c
U   branches/1.4/res/res_features.c

------------------------------------------------------------------------
r172030 | murf | 2009-01-28 12:51:09 -0600 (Wed, 28 Jan 2009) | 46 lines

This patch fixes h-exten running misbehavior in manager-redirected
situations.

What it does:
1. A new Flag value is defined in include/asterisk/channel.h,
AST_FLAG_BRIDGE_HANGUP_DONT, which used as a messenge to the
bridge hangup exten code not to run the h-exten there (nor
publish the bridge cdr there). It will done at the pbx-loop
level instead.
2. In the manager Redirect code, I set this flag on the channel
if the channel has a non-null pbx pointer. I did the same for the
second (chan2) channel, which gets run if name2 is set...
and the first succeeds.
3. I restored the ending of the cdr for the pbx loop h-exten
running code. Don't know why it was removed in the first place.
4. The first attempt at the fix for this bug was to place code
  directly in the async_goto routine, which was called from a
  large number of places, and could affect a large number of
  cases, so I tested that fix against a fair number of transfer
  scenarios, both with and without the patch. In the process,
  I saw that putting the fix in async_goto seemed not to affect
  any of the blind or attended scenarios, but still, I was
  was highly concerned that some other scenarios I had not tested
  might be negatively impacted, so I refined the patch to
  its current scope, and jmls tested both. In the process, tho,
  I saw that blind xfers in one situation, when the one-touch
  blind-xfer feature is used by the peer, we got strange
  h-exten behavior.  So, I  inserted code to swap CDRs and
  to set the HANGUP_DONT field, to get uniform behavior.
5. I added code to the bridge to obey the HANGUP_DONT flag,
  skipping both publishing the bridge CDR, and running
  the h-exten; they will be done at the pbx-loop (higher)
  level instead.
6. I removed all the debug logs from the patch before committing.
7. I moved the AUTOLOOP set/reset in the h-exten code in res_features
  so it's only done if the h-exten is going to be run. A very
  minor performance improvement, but technically correct.


(closes issue ASTERISK-13367)
Reported by: jmls
Patches:
     14241_redirect_no_bridgeCDR_or_h_exten_via_transfer uploaded by murf (license 17)
Tested by: murf, jmls


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=172030
By: Digium Subversion (svnbot) 2009-01-28 14:30:46.000-0600

Repository: asterisk
Revision: 172063

_U  trunk/
U   trunk/apps/app_channelredirect.c
U   trunk/include/asterisk/channel.h
U   trunk/main/features.c
U   trunk/main/manager.c
U   trunk/main/pbx.c

------------------------------------------------------------------------
r172063 | murf | 2009-01-28 14:30:46 -0600 (Wed, 28 Jan 2009) | 52 lines

Merged revisions 172030 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r172030 | murf | 2009-01-28 11:51:16 -0700 (Wed, 28 Jan 2009) | 46 lines
 
 This patch fixes h-exten running misbehavior in manager-redirected
 situations.
 
 What it does:
 1. A new Flag value is defined in include/asterisk/channel.h,
  AST_FLAG_BRIDGE_HANGUP_DONT, which used as a messenge to the
  bridge hangup exten code not to run the h-exten there (nor
  publish the bridge cdr there). It will done at the pbx-loop
  level instead.
 2. In the manager Redirect code, I set this flag on the channel
  if the channel has a non-null pbx pointer. I did the same for the
  second (chan2) channel, which gets run if name2 is set...
  and the first succeeds.
 3. I restored the ending of the cdr for the pbx loop h-exten
  running code. Don't know why it was removed in the first place.
 4. The first attempt at the fix for this bug was to place code
    directly in the async_goto routine, which was called from a
    large number of places, and could affect a large number of
    cases, so I tested that fix against a fair number of transfer
    scenarios, both with and without the patch. In the process,
    I saw that putting the fix in async_goto seemed not to affect
    any of the blind or attended scenarios, but still, I was
    was highly concerned that some other scenarios I had not tested
    might be negatively impacted, so I refined the patch to
    its current scope, and jmls tested both. In the process, tho,
    I saw that blind xfers in one situation, when the one-touch
    blind-xfer feature is used by the peer, we got strange
    h-exten behavior.  So, I  inserted code to swap CDRs and
    to set the HANGUP_DONT field, to get uniform behavior.
 5. I added code to the bridge to obey the HANGUP_DONT flag,
    skipping both publishing the bridge CDR, and running
    the h-exten; they will be done at the pbx-loop (higher)
    level instead.
 6. I removed all the debug logs from the patch before committing.
 7. I moved the AUTOLOOP set/reset in the h-exten code in res_features
    so it's only done if the h-exten is going to be run. A very
    minor performance improvement, but technically correct.
 
 
 (closes issue ASTERISK-13367)
 Reported by: jmls
 Patches:
       14241_redirect_no_bridgeCDR_or_h_exten_via_transfer uploaded by murf (license 17)
 Tested by: murf, jmls
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=172063
By: Digium Subversion (svnbot) 2009-01-28 14:41:01.000-0600

Repository: asterisk
Revision: 172065

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_channelredirect.c
U   branches/1.6.0/include/asterisk/channel.h
U   branches/1.6.0/main/features.c
U   branches/1.6.0/main/manager.c
U   branches/1.6.0/main/pbx.c

------------------------------------------------------------------------
r172065 | murf | 2009-01-28 14:41:00 -0600 (Wed, 28 Jan 2009) | 59 lines

Merged revisions 172063 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r172063 | murf | 2009-01-28 13:31:06 -0700 (Wed, 28 Jan 2009) | 52 lines
 
 Merged revisions 172030 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r172030 | murf | 2009-01-28 11:51:16 -0700 (Wed, 28 Jan 2009) | 46 lines
   
   This patch fixes h-exten running misbehavior in manager-redirected
   situations.
   
   What it does:
   1. A new Flag value is defined in include/asterisk/channel.h,
    AST_FLAG_BRIDGE_HANGUP_DONT, which used as a messenge to the
    bridge hangup exten code not to run the h-exten there (nor
    publish the bridge cdr there). It will done at the pbx-loop
    level instead.
   2. In the manager Redirect code, I set this flag on the channel
    if the channel has a non-null pbx pointer. I did the same for the
    second (chan2) channel, which gets run if name2 is set...
    and the first succeeds.
   3. I restored the ending of the cdr for the pbx loop h-exten
    running code. Don't know why it was removed in the first place.
   4. The first attempt at the fix for this bug was to place code
      directly in the async_goto routine, which was called from a
      large number of places, and could affect a large number of
      cases, so I tested that fix against a fair number of transfer
      scenarios, both with and without the patch. In the process,
      I saw that putting the fix in async_goto seemed not to affect
      any of the blind or attended scenarios, but still, I was
      was highly concerned that some other scenarios I had not tested
      might be negatively impacted, so I refined the patch to
      its current scope, and jmls tested both. In the process, tho,
      I saw that blind xfers in one situation, when the one-touch
      blind-xfer feature is used by the peer, we got strange
      h-exten behavior.  So, I  inserted code to swap CDRs and
      to set the HANGUP_DONT field, to get uniform behavior.
   5. I added code to the bridge to obey the HANGUP_DONT flag,
      skipping both publishing the bridge CDR, and running
      the h-exten; they will be done at the pbx-loop (higher)
      level instead.
   6. I removed all the debug logs from the patch before committing.
   7. I moved the AUTOLOOP set/reset in the h-exten code in res_features
      so it's only done if the h-exten is going to be run. A very
      minor performance improvement, but technically correct.
   
   
   (closes issue ASTERISK-13367)
   Reported by: jmls
   Patches:
         14241_redirect_no_bridgeCDR_or_h_exten_via_transfer uploaded by murf (license 17)
   Tested by: murf, jmls
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=172065
By: Digium Subversion (svnbot) 2009-01-28 14:56:04.000-0600

Repository: asterisk
Revision: 172067

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_channelredirect.c
U   branches/1.6.1/include/asterisk/channel.h
U   branches/1.6.1/main/features.c
U   branches/1.6.1/main/manager.c
U   branches/1.6.1/main/pbx.c

------------------------------------------------------------------------
r172067 | murf | 2009-01-28 14:56:03 -0600 (Wed, 28 Jan 2009) | 59 lines

Merged revisions 172063 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r172063 | murf | 2009-01-28 13:31:06 -0700 (Wed, 28 Jan 2009) | 52 lines
 
 Merged revisions 172030 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r172030 | murf | 2009-01-28 11:51:16 -0700 (Wed, 28 Jan 2009) | 46 lines
   
   This patch fixes h-exten running misbehavior in manager-redirected
   situations.
   
   What it does:
   1. A new Flag value is defined in include/asterisk/channel.h,
    AST_FLAG_BRIDGE_HANGUP_DONT, which used as a messenge to the
    bridge hangup exten code not to run the h-exten there (nor
    publish the bridge cdr there). It will done at the pbx-loop
    level instead.
   2. In the manager Redirect code, I set this flag on the channel
    if the channel has a non-null pbx pointer. I did the same for the
    second (chan2) channel, which gets run if name2 is set...
    and the first succeeds.
   3. I restored the ending of the cdr for the pbx loop h-exten
    running code. Don't know why it was removed in the first place.
   4. The first attempt at the fix for this bug was to place code
      directly in the async_goto routine, which was called from a
      large number of places, and could affect a large number of
      cases, so I tested that fix against a fair number of transfer
      scenarios, both with and without the patch. In the process,
      I saw that putting the fix in async_goto seemed not to affect
      any of the blind or attended scenarios, but still, I was
      was highly concerned that some other scenarios I had not tested
      might be negatively impacted, so I refined the patch to
      its current scope, and jmls tested both. In the process, tho,
      I saw that blind xfers in one situation, when the one-touch
      blind-xfer feature is used by the peer, we got strange
      h-exten behavior.  So, I  inserted code to swap CDRs and
      to set the HANGUP_DONT field, to get uniform behavior.
   5. I added code to the bridge to obey the HANGUP_DONT flag,
      skipping both publishing the bridge CDR, and running
      the h-exten; they will be done at the pbx-loop (higher)
      level instead.
   6. I removed all the debug logs from the patch before committing.
   7. I moved the AUTOLOOP set/reset in the h-exten code in res_features
      so it's only done if the h-exten is going to be run. A very
      minor performance improvement, but technically correct.
   
   
   (closes issue ASTERISK-13367)
   Reported by: jmls
   Patches:
         14241_redirect_no_bridgeCDR_or_h_exten_via_transfer uploaded by murf (license 17)
   Tested by: murf, jmls
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=172067