[Home]

Summary:ASTERISK-19336: h exten is not run in the context that calls a AEL macro
Reporter:Johan Wilfer (johan)Labels:
Date Opened:2012-02-10 03:38:56.000-0600Date Closed:2012-03-13 03:14:18
Priority:MajorRegression?No
Status:Closed/CompleteComponents:PBX/pbx_ael
Versions:1.8.9.2 Frequency of
Occurrence		Constant
Related
Issues:
Environment:Attachments:( 0) 20120304__ael_bugfix_for_trunk__2_round2.txt
( 1) 20120304__ael_bugfix_for_trunk__2.diff
( 2) 20120307__ael_bugfix_for_trunk.diff
( 3) 20120307_testcases_version3_ASTERISK-19336.ael
( 4) 20120307_testcases_version3_FAIL_details_ASTERISK-19336.txt
( 5) 20120308__ael_bugfix_for_trunk__2.diff
( 6) 20120308__ael_bugfix_for_trunk.diff
( 7) testing-ASTERISK-19336_20120304__ael_bugfix_for_trunk__2.diff.txt
Description:I expect the h exten to be run both in the macro catch-block and in the context calling the macro. This is not the case.

The following code:
{code:xml}
 context conference {
   incoming => {
     &testmacro();
   }

   h => {
     Noop(Called from the context that invokes the macro - THIS DOESN'T WORK);
   }
 }

 macro testmacro() {
   Noop(Running testmacro!);
   Wait(20);

   catch h {
     Noop(h catched in testmacro - OK!);
   }
 }
{code}

Creates this output:
{code:xml}
   -- Executing [incoming@conference:1] Gosub("SIP/trunk-00000002", "testmacro,~~s~~,1") in new stack
   -- Executing [~~s~~@testmacro:1] NoOp("SIP/trunk-00000002", "Running testmacro!") in new stack
   -- Executing [~~s~~@testmacro:2] Wait("SIP/trunk-00000002", "20") in new stack
 == Spawn extension (testmacro, ~~s~~, 2) exited non-zero on 'SIP/trunk-00000002'
   -- Executing [h@testmacro:1] NoOp("SIP/trunk-00000002", "h catched in testmacro - OK!") in new stack
{code}

The h-exten in the calling context is never called..
The issue is the same with 1.6.2.20, and it's also the same if you remove the "catch h { ... } "-block in the macro - the context that calls the macro never executes it's h-exten.

Comments:By: Matt Jordan (mjordan) 2012-02-10 08:36:01.905-0600

Only one hangup handler will ever be called.  The hangup handler that is called will be the one closest in scope to the current location of the executing call.

Note that there is a proposed feature that would allow for a stack of hangup handlers to be called; however, that would be a new feature and not a bug.
By: Johan Wilfer (johan) 2012-02-10 08:48:52.260-0600

Ok, but if I remove the catch h { ... } in the macro the h-exten are still not executed.

Shouldn't it be run in this case?

By: Tilghman Lesher (tilghman) 2012-03-02 10:55:33.348-0600

AEL will need to be altered, which is a complex task.  I've created a STACKPEEK() dialplan function which will be helpful in this regard, on reviewboard: https://reviewboard.asterisk.org/r/1776/ .  The necessary implementation for the bugfix is detailed here: http://lists.digium.com/pipermail/asterisk-dev/2012-February/054040.html
By: Tilghman Lesher (tilghman) 2012-03-04 03:53:58.913-0600

The review on 1776 now includes the necessary changes to AEL to make the bugfix complete.
By: Johan Wilfer (johan) 2012-03-05 02:43:31.418-0600

Cool! I'll put together some ael dialplan that tests different scenarios and report back. Thanks!
By: Johan Wilfer (johan) 2012-03-06 16:13:59.564-0600

I created a testing dialplan that tests nested macros, and macros with catch h in them.
In all cases but one the patch works as expected.

The case that don't work is a macro nested in another macro, this causes a infinite loop.
See the testing document for more details. In the other cases this works great.
By: Johan Wilfer (johan) 2012-03-06 16:34:06.400-0600

I created another test, hangup at level 2 or level 5 triggers the same infinite loop.
By: Tilghman Lesher (tilghman) 2012-03-07 00:41:30.624-0600

I have uploaded an updated patch that should deal correctly with the infinite loop issue, by ensuring that stack frames are popped off before the Goto occurs.
By: Johan Wilfer (johan) 2012-03-07 16:59:10.363-0600

New testcase I've tested with
By: Johan Wilfer (johan) 2012-03-07 17:02:40.153-0600

Now all tests fail if there isn't a catch h { ... } in the macro.

Actually the "macro-bubbeling" up to the calling context seems to work fine, but at the calling context there is an infinite loop.

It seems like ael-builtin-h-bubble is included on regular contexts as well, shouldn't ael-builtin-h-bubble only be included on macro-contexts?

--

dialplan show macrotest
[ Context 'macrotest' created by 'pbx_ael' ]
 'h' =>            1. Noop(Catch h in context that invokes the macro) [pbx_ael]
 's' =>            1. Answer()                                   [pbx_ael]
                   2. Gosub(macro_level1,~~s~~,1)                [pbx_ael]
 Include =>        'ael-builtin-h-bubble'                        [pbx_ael]

-= 2 extensions (3 priorities) in 1 context. =-
By: Tilghman Lesher (tilghman) 2012-03-08 01:37:31.990-0600

This patch uses a dialplan trick to avoid partial logic from appending onto existing h extensions.
By: Tilghman Lesher (tilghman) 2012-03-08 10:49:05.061-0600

Technically, the logic could only be included in macro contexts.  If you can figure out a practical way to either verify whether a context is a macro from the intermediate generation or insert the Include only in macro contexts when it is generated (but ensure that the Include is absolutely the last one), I'd be happy to make that change.  As it stands, I've minimized the amount of code that I needed to modify within the quite complex module.
By: Tilghman Lesher (tilghman) 2012-03-08 15:34:31.307-0600

New patch uploaded, after Terry's review of the code.
By: Johan Wilfer (johan) 2012-03-10 11:49:00.920-0600

Nice work!

Tested the new patch (20120308__ael_bugfix_for_trunk__2.diff) in 1.8.10.0 and it works as expected.
I also ran the tests with a start context with no h-exten and this also works without problems.
As it is right now I'm very satisfied with the current patch.

One last thought: How should we handle the other special extensions: i, t, T, o, a (have I missed any?)
I expect the same difference in implementation between 1.4 and 1.8 for those extensions.

Maybe similar code should be added for those cases for the implementation to consistent?
By: Johan Wilfer (johan) 2012-03-12 04:52:55.557-0500

Or maybe the T (absolute timeout) should be treated like h, as the functionality is similar..

Maybe the user is supposed to implement i, t, o, a in the macro with catch if he wants to catch these?
It could maybe cause more confusion to have the i-exten in another level execute?

I think I would prefer to implement the same logic to T and not implement the same logic to the i,t,o,a extensions. Or to simply implement it the way it is now in the patch, for the h exten only.
By: Tilghman Lesher (tilghman) 2012-03-13 03:14:18.463-0500

Committed to 1.8 revision 358810.