[Home]

Summary:ASTERISK-12533: [branch] endbeforehexten=yes is useless now
Reporter:Sergey Tamkovich (sergee)Labels:
Date Opened:2008-08-06 15:10:26Date Closed:2008-09-16 17:34:11
Priority:MajorRegression?No
Status:Closed/CompleteComponents:CDR/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) h-exten5-1.4.diff
( 1) h-exten5-trunk.diff
( 2) parking_crash1.txt
Description:1. Part of code:

 if (c->cdr && ast_opt_end_cdr_before_h_exten)
                        ast_cdr_end(c->cdr);

- missing (this is how it was
http://svn.digium.com/view/asterisk/trunk/pbx.c?r1=12896&r2=12895&pathrev=12896)

2. I have very simple scenario SIP/A calls SIP/B they talk 30 seconds, then call is finished. Even after i call ast_cdr_end(chan->cdr) - manually from my app,  these fields aren't initialized:

chan->cdr->billsec == 0,
chan->cdr->duration == 0,
chan->cdr->answer.tv_sec == 0,
chan->cdr->end.tv_sec == 0

3. as far as i understand, CDRs are posted now as soon as channels are unbridged. This behavior breaks billings which make calculations inside 'h' extensions. It would be nice to post CDRs after no more apps are left in the 'h' extension or to have an application in the DialPlan, something like CDRPost().


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

I hope this info will help to improve CDR subsystem, tomorrow i'll see what i can do about CDRPost implemeentation.
Comments:By: Sergey Tamkovich (sergee) 2008-08-06 15:22:20

my dialplan:


exten => _X.,1,Predial()
exten => _X.,1,DIAL(${DST})
exten => h,1,Postdial()


i'm calling ast_cdr_end(chan->cdr) inside Postdial(): CDR is already posted and those variable in chan->cdr aren't initialised.
By: Sergey Tamkovich (sergee) 2008-08-09 14:40:15

This part of code from main_features.c ast_bridge_call()  - This is the place where CDRs get posted. Can anybody explain me initial purpose of thi code?

        /* obey the NoCDR() wishes. */
        if (!chan->cdr || (chan->cdr && !ast_test_flag(chan->cdr, AST_CDR_FLAG_POST_DISABLED))) {
                ast_cdr_end(bridge_cdr);
                ast_cdr_detach(bridge_cdr);
                /* just in case, these channels get bridged again before hangup */
                if (chan->cdr) {
                        ast_cdr_specialized_reset(chan->cdr,0);
                }
                if (peer->cdr) {
                        if (orig_peer_cdr && peer->cdr != orig_peer_cdr) {
                                /* this can only happen if there was a transfer, methinks */
                                ast_cdr_specialized_reset(orig_peer_cdr,0);
                        } else {
                                ast_cdr_specialized_reset(peer->cdr,0);
                        }
                }
        }


By: Steve Murphy (murf) 2008-08-20 13:01:02

Since I'm the one that put this code in place, I'd be best able to explain it.. you would hope. I moved to posting a bridge cdr because of transfers. Every change I've made is because of the complexities of what happens in transfers. The problem here is the specialized reset kills the ability to see the values in hbeforeexten. Yet, because of xfers, we have a complication. I've been thinking on this for a while, and will explore this issue today.
By: Digium Subversion (svnbot) 2008-08-21 17:54:55

Repository: asterisk
Revision: 139347

U   branches/1.4/main/pbx.c
U   branches/1.4/res/res_features.c

------------------------------------------------------------------------
r139347 | murf | 2008-08-21 17:54:54 -0500 (Thu, 21 Aug 2008) | 47 lines


(closes issue ASTERISK-12533)
Reported by: sergee
Tested by: murf



THis is a bold move for a static release fix, but I wouldn't have
made it if I didn't feel confident (at least a *bit* confident)
that it wouldn't mess everyone up.

The reasoning goes something like this:

1. We simply cannot do anything with CDR's at the current point
(in pbx.c, after the __ast_pbx_run loop). It's way too late to
have any affect on the CDRs. The CDR is already posted and gone,
and the remnants have been cleared.

2. I was very much afraid that moving the running of the 'h'
extension down into the bridge code (where it would be now
practical to do it), would result in a lot more calls to the
'h' exten, so I implemented it as another exten under another
name, but found, to my pleasant surprise, that there was a
1:1 correspondence to the running of the 'h' exten in the
pbx_run loop, and the new spot at the end of the bridge.
So, I ifdef'd out the current 'h' loop, and moved it into
the bridge code. The only difference I can see is the stuff
about the AST_PBX_KEEPALIVE, and hopefully, if this
is still an important decision point, I can replicate it
if there are complaints. To be perfectly honest,
the KEEPALIVE situation is not totally clear to me,
and how it relates to a post-bridge situation is less
clear. I suspect the users will point out everything
in total clarity if this steps on anyone's toes!

3. I temporarily swap the bridge_cdr into the channel
before running the 'h' exten, which makes it possible
for users to edit the cdr before it goes out the door.
And, of course, with the endbeforehexten config var set,
the users can also get at the billsec/duration vals.
After the h exten finishes, the cdr is swapped back
and processing continues as normal.

Please, all who deal with CDR's, please test this version
of Asterisk, and file bug reports as appropriate!


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

http://svn.digium.com/view/asterisk?view=rev&revision=139347
By: Steve Murphy (murf) 2008-08-25 11:06:37

I'm reopening this bug, as I have had to revert the changes, as users are seeing differences with current behavior.

We'll try some other approaches and see if we can recapture the previous
hangup environment and behavior.
By: Steve Murphy (murf) 2008-08-25 14:34:35

OK, I uploaded the h-exten3.diff patch for 1.4; please test it and see if it's less more 'compatible' with 1.4. What did I do? I define a channel flag that I set when the bridge closure h-exten is run, that prevents the duplicate h-exten from being run after the pbx_run loop. This allows the normal operation of the pbx_run running of the h-exten when no bridge happens.

Please see if this hits closer to the target of 1.4 as-usual behavior.
Please inform me if there are still behavior difference, and what the differences are.
By: Steve Murphy (murf) 2008-08-25 17:17:05

Sorry, after discussion with Tilghman, I added some code to obey the 'g' option of
dial by adding a flag to the features_caller field 'NO_H_EXTEN', which applies only to the post-bridge case. If that flag is set in Dial, after the bridge terminates, the h-exten is not executed, and no channel flag is set, so the
pbx_run does the h exten on hangup like normal at that time.

So, please, give the h-exten4.diff a spin-- revert any changes from h-exten3,
and then use patch to apply the new diffs. See if you get the hangup extension behavior you have grown accustomed to over time....
By: Steve Murphy (murf) 2008-08-26 11:15:25

Just to emphasize that I need testers for this bug:

I hope all monitoring this bug understand that if no-one considers this important enough to test and report on, then I will close it with prejudice. It may end up being a lot of effort to make it work correctly, and if the users don't care about no longer having the ability to use the CDR() func in the h extension, then so be it.

I will give this bug the rest of the week, and if no reports, I will close it and be happy not to have spent more time on it. If anyone needs more time, let me know.
By: Chris Maciejewski (chris-mac) 2008-08-26 12:33:11

Just to let you know CDR data in 'h' extension is very important in my dial plan and I would like to help testing the patch. However when I try to apply it to 1.6.0 branch (r140157) or 1.6.0-rc3 I am getting the following errors:

root@dev2:/usr/src/asterisk-1.6.0# patch -p0 < ../h-exten4.diff  
patching file apps/app_dial.c
Hunk #1 FAILED at 1739.
1 out of 1 hunk FAILED -- saving rejects to file apps/app_dial.c.rej
patching file include/asterisk/channel.h
Hunk #1 FAILED at 510.
Hunk #2 FAILED at 524.
2 out of 2 hunks FAILED -- saving rejects to file include/asterisk/channel.h.rej
patching file main/pbx.c
Hunk #1 succeeded at 3845 with fuzz 2 (offset 1306 lines).
Hunk #2 succeeded at 3857 (offset 1303 lines).
can't find file to patch at input line 65
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: res/res_features.c
|===================================================================
|--- res/res_features.c (revision 139779)
|+++ res/res_features.c (working copy)
--------------------------
File to patch:

Could you please let me know how shall I apply  h-exten4.diff ??
Regards,
Chris
By: Steve Murphy (murf) 2008-08-26 16:03:57

Chris--

Good to have you testing!

I'm glad to help you here. The patch was for 1.4, so I got busy and
prepared a similar patch for trunk (and it should apply to the 1.6.x
stuff also).

While I was at it, I found a few missing items in the 1.4 stuff, so
I cut the h-exten5 files that are attached to this bug, one for
1.4, one for trunk.

You should find it pretty straightforward to apply the trunk patch
to your 1.6.x releases.

Shout if you have any problems.
By: Chris Maciejewski (chris-mac) 2008-08-26 16:56:12

I can confirm h-exten5-trunk.diff applied to 1.6.0 branch solved my problem, and I can use CDR data in 'h' extension again :-) - CDR(billsec) specifically.

Can we please leave this bug open until end of this week, so I can do some more testing?

Regards,
Chris
By: Andrew Lindh (andrew) 2008-08-26 21:29:39

I gave it a quick test (in branch 1.4) and 'h' looks like it still works. I'll have to give it a few more tests shortly.
By: Steve Murphy (murf) 2008-08-27 12:04:26

andrew-- many thanks. The danger of this patch is that the 'h' exten gets called when it normally would not. I checked it against some transfer situations, and it looked OK, and I coded and tested it against the dial 'g' option, and it appeared to be OK there, too. Let me know what kinds of tests you did, and that way we can see what kind of coverage we have. It should be OK in situations where a bridge doesn't happen, too. So what else do we need to watch out for?
By: Andrew Lindh (andrew) 2008-08-27 15:38:54

We use 'h' in our RxFax application. After the fax application completes, the channel is disconnected (by the app, the dialplan hangup, or the SIP channel closes) and the dialplan continues to the 'h' extension to complete the fax cleanup operations (email the fax and delete the temp file). This is not exactly standard usage, but it has been working correctly for a long time. We don't use it for normal calls so there are no transfers or other user interactions. Let me know if you want more details.
By: Steve Murphy (murf) 2008-08-27 18:13:59

I get the picture. Incoming call gets answered by asterisk, connects to rxfax app, at hangup, probably no bridge involved, and it works. That's good.
By: Chris Maciejewski (chris-mac) 2008-08-28 02:13:03

My scenario is as follows:

1. There is an incoming call to some E.164 number coming into * from user 'A'
2. Asterisk sets two custom CDR vars - (Set(CDR(DST_RATE)=0.033), (Set(CDR(DST_NAME)=Spain Mobile)
3. Asterisk Dials PSTN gateway and creates a bridge with 'A'
4. User 'A' hangs up and in 'h' extension:
  a) Call charge is calculated as CDR(billsec) * CDR(DST_RATE)
  b) Call charge is deducted from customer account
5. Finally CDR record is inserted into MySQL table - cdr_mysql

Regards,
Chris
By: Kaloyan Kovachev (knk) 2008-08-29 04:38:51

Even with this patch applied the h extension is not executed if the channel is closed while Background() is playing
By: Steve Murphy (murf) 2008-08-29 17:05:46

KNK-- really? I just ran a simple dialplan with a sip phone, on the patched 1.4 and trunk version, and the hangup (h) extension gets run if you hang up the sip phone while background is playing. The extension looks like this AEL:

  839 => { Background(demo-instruct); }

And, if demo-instruct actually finishes playing, I see the h exten get run also.

So, give me some detail about the situation you are seeing.
By: Kaloyan Kovachev (knk) 2008-08-30 03:28:38

I have tested again with a single line dialplan:

exten => 222,1,Background(silence/9,m,bg,from-IVR)

and the result is the same:

[2008-08-30 11:30:13]     -- Executing [222@Internal:1] BackGround("SIP/1070-adc06aa0", "silence/9|m|bg|from-IVR") in new stack
[2008-08-30 11:30:13]     -- <SIP/1070-adc06aa0> Playing 'silence/9' (language 'bg')
core show channels
Channel              Location             State   Application(Data)
SIP/1070-adc06aa0    222@Internal:1       Up      BackGround(silence/9|m|bg|from
1 active channel
1 active call
My*CLI> core show channels
Channel              Location             State   Application(Data)
0 active channels
0 active calls

the version is:
Asterisk SVN-branch-1.4-r140299M built by root @ My on a x86_64 running Linux on 2008-08-29 07:39:48 UTC

I have also tried to remove the language and m options, but still the same. Even adding an explicit Answer() before that is not changing the situation.
I am not sure if it is actually related to the h extension changes or there is something else.


By: Steve Murphy (murf) 2008-09-03 14:00:38

KNK--

Using Background(silence/9,m,bg,from-IVR)

I have tried 1.4 with and without this patch; I have done it with silence/9 getting silence/9.gsm, and also silence/bg/9.gsm; In every case, I see the hangup routine getting run in the proper way. I need still more information.

Let me note, however that you need to verify that your problem only occurs on 1.4 with the latest patch applied from this bug, and NOT on 1.4 without this bug report's patch. If you cannot verify this, then you need to file this problem
as a separate bug report.
By: Steve Murphy (murf) 2008-09-03 17:17:05

OK, I have turned my private code dirs into branches, one for trunk, the other for 1.4;

team/murf/trunk-hexten
team/murf/1.4-hexten

which contain the fix code for this bug.
These branches now include fixes for the "continue" option to app-queue.
By: Kaloyan Kovachev (knk) 2008-09-04 08:49:40

I will dig a bit more in the Background problem and will post a bugreport if it proves to be a bug (and not my setup problem)

Just wanted to note that with this patch the rest is working OK for me.
By: Steve Murphy (murf) 2008-09-05 11:55:55

OK, it's Friday, I've given it a whole week; is everyone happy? Have I forgotten any issues? Can I commit this again, and not surprise a lot of folks?
By: Chris Maciejewski (chris-mac) 2008-09-05 12:50:22

Yes, I can confirm h-exten5-trunk.diff works great for me. Please do commit.
By: Steve Murphy (murf) 2008-09-05 15:23:39

OK, Russell advises to wait another week, till after the next 1.4 release, so that we can get any possible rough edges smoothed off in a lower-pressure situation.
By: Steve Murphy (murf) 2008-09-11 23:57:14

Hmmm. I did something wrong in the commit msg, I guess, so I'll close this
bug 'by hand'.

I've applied the changes from the branches advertised to 1.4 and trunk and 1.6.x
via svn revisions 142675 thru 142678.

Here is the verbage that went with the commit:

(closes issue ASTERISK-12566)
Reported by: TechnoPirate

(closes issue ASTERISK-12533)
Reported by: sergee
Tested by: sergee, murf, chris-mac, andrew, KNK


This is a "second attempt" to restore the previous "endbeforeh" behavior
in 1.4 and up. In order to capture information concerning all the
legs of transfers in all their infinite combinations, I was forced
to this particular solution by a chain of logical necessities, the
first being that I was not allowed to rewrite the CDR mechanism from
the ground up!

This change basically leaves the original machinery alone, which allows
IVR and local channel type situations to generate CDR's as normal, but
a channel flag can be set to suppress the normal running of the h exten.
That flag would be set by the code that runs the h exten from the
ast_bridge_call routine, to prevent the h exten from being run twice.
Also, a flag in the ast_bridge_config struct passed into ast_bridge_call
can be used to suppress the running of the h exten in that routine. This
would happen, for instance, if you use the 'g' option in the Dial app.

Running this routine 'early' allows not only the CDR() func to be used
in the h extension for reading CDR variables, but also allows them to
be modified before the CDR is posted to the backends.

While I dearly hope that this patch overcomes all problems, and
introduces no new problems, reality suggests that surely someone
will have problems. In this case, please re-open 13251 (or 13289),
and we'll see if we can't fix any remaining issues.
By: mdu113 (mdu113) 2008-09-15 15:17:34

murf, as you foresaw, at least one problem is indeed introduced by this change ;-(
As I described in issue 13425, this change causes crash if one-touch parking is used during the call and it was initiated by caller. Scenario: A calls B, B answers, A dials feature code to park call. parking_crash1.txt has console output and backtrace of the problem call.
By: Steve Murphy (murf) 2008-09-16 17:32:25

I've looked at 13425, and it looks like jpeeler has a fix that is orthogonal to this problem; I can't immediately see how this fix affects 13425. So, since that bug is separate, and the way these two would interact is distant at best, I will reclose this issue until a compelling link between the two is forthcoming.