[Home]

Summary:ASTERISK-12999: Executing 'h' extension if parked channel hangs up
Reporter:mdu113 (mdu113)Labels:
Date Opened:2008-10-31 13:50:40Date Closed:2008-12-23 19:39:45.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Resources/res_features
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 13820.diff1
Description:I hope this is just an oversight and something that can be added easily.
Currently there's no way specify 'h' extension to be executed when parked channel hangs up ("got tired of being parked" in asterisk language).
If I add "parkedcalls" context to my dialplan and create an 'h' extension there, it creates 2 different contexts of the same name and 'h' extension is not executed:
devel*CLI> dialplan show parkedcalls
[ Context 'parkedcalls' created by 'pbx_config' ]
 'h' =>            1. NoOp(Parking H extension)                  [pbx_config]

[ Context 'parkedcalls' created by 'res_features' ]
 '700' =>          1. Park()                                     [res_features]
devel*CLI>
-= 2 extensions (2 priorities) in 2 contexts. =-
I really need to be able to do some additional stuff on parked channel hang up.
I think the current behavior is incorrect or at least inconsistent.
Comments:By: Steve Murphy (murf) 2008-11-03 10:56:50.000-0600

Quite interesting. In a fairly recent fix to prevent crashes with early parking hangups,
I found that some code wasn't protected from operation on parked channels.
Running the h-exten in main/pbx.c was one of those places. Since the channel
being parked is given to the parking manager thread, the pbx thread should not
be trying to run the h-exten for that channel any more; cases of "tired" hangups could yield a crash. This has always been there. So, I ran an experiment; I theorized that, if h-exten had always been run on parked 'tired' channels, and no-one complained until now (about crashes/weirdness), then maybe some logical sequence of events I missed made that code valid. So, I removed the parking restriction from the if that wraps the h-exten run code, and tried the scenarios. Got crashes and weird behavior.

This must be stuff that was always there.

So replacing the conditional, I tried the 4 scenarios, and watched how the h-exten was run.

A calls B, B answers; A parks B, B hangs up during A's parking exten ann-ment
 -A  gets the hangup exten run

A calls B, B answers; A parks B, B hangs up before his parking time expires.
 -A  gets the hangup exten run

A calls B, B answers; B parks A, A hangs up during B's parking exten ann-ment
 -No h exten run on A (of course)

A calls B, B answers; B parks A, A hangs up before his parking time expires.
 -No h exten run on A (of course)

So, for half the cases, the h-exten is run as normal.

I'll investigate to see how easy/hard it might be to make the parking thread run the hangup routine, iff it was "channel" and not "peer" in the call role.


By: Steve Murphy (murf) 2008-11-04 18:44:58.000-0600

OK, doing it that way would have been difficult. Really quite tricky!

So, Russell advised using the masq_park_call stuff instead of park_call
stuff. which I did, and entirely got rid of the KEEPALIVE and NO_HANGUP_PEER
stuff in the res_features.c and app_dial and app_queue code, simplifying things
and making them more crash-resistant.

There are some minor differences, tho.

In the 4 scenarios I mentioned above, the hangup routine is run in all
four cases (good), but in the latter two cases (where B parks A),
the channel name appears "Parked/DAHDI/1-1<ZOMBIE>", where it would normally
have been just "DAHDI/1-1". The CDR's generated from these scenarios are
as normal... they report DAHDI/1-1 and DAHDI/2-1 as previous.

(SIP (and other channel types) behave the same way).

I will continue testing to see how blindxfers, atxfers, etc, might or (hopefully) might not have been affected. More to come.

I've attached the 13820.diff1 patch to this bug; please give it a spin and let me know if there are problems.

Also, I noted that there are more than one way to park a channel:
1. Request a park via the manager interface: this uses the masq_park_call
  facility (which is good).
2. Via ASTERISK-694 (blind xfer feature to parking exten) -- this I modified to
  use masq_park_call_announce().
3. Via *3 (one-touch park feature)... this used masq_park_call_announce
  in one case and the park_call facility in the other. Now, it just does the
  masq_park_call thing.
4. Via the Park() app... This calls park_call_exec() which USED to return
  KEEPALIVE; I make it return 0 now; I did not update this to use the
  masq_park_call facility, because I wasn't certain that this would
  be of benefit. I'll research it out and see.
By: mdu113 (mdu113) 2008-11-04 20:08:41.000-0600

murf, I'll test your patch tomorrow.
Please take a look at issue 13425. while fixing that issue, jpeeler has made several important changes about doing masquerading for parked calls and also he explained why he believes masquerading should not be used in all cases. I thought you may want to talk to him.
By: mdu113 (mdu113) 2008-11-05 11:52:00.000-0600

I've update asterisk to the latest SVN-branch-1.4-r154724 and applied your patch.
The 'h' extension still isn't executed whatever I do. I see no difference.
I'm using very simple dialplan:

[xyz]
exten => 100,1,Dial(SIP/xyz010001,10,kK)
exten => 650,1,Dial(SIP/poly_650_01,10,kK)
exten => h,1,NoOp(Hangup extension)

[parkedcalls]
exten => h,1,NoOp(Parking H extension)

Test: 100 calls 650, 650 answers, 100 performs one-touch parking. 650 hangs up before timeout:

   -- Executing [650@xyz:1] Dial("SIP/xyz010001-081e7db0", "SIP/poly_650_01|10|kK") in new stack
   -- Called poly_650_01
   -- SIP/poly_650_01-081e9340 is ringing
   -- SIP/poly_650_01-081e9340 answered SIP/xyz010001-081e7db0
   -- Started music on hold, class 'default', on channel 'SIP/poly_650_01-081e9340'
 == Parked SIP/poly_650_01-081e9340 on 701@parkedcalls. Will timeout back to extension [xyz] , 1 in 600 seconds
   -- Added extension '701' priority 1 to parkedcalls
   -- <SIP/xyz010001-081e7db0> Playing 'digits/7' (language 'en')
   -- <SIP/xyz010001-081e7db0> Playing 'digits/0' (language 'en')
   -- <SIP/xyz010001-081e7db0> Playing 'digits/1' (language 'en')
   -- Executing [h@xyz:1] NoOp("SIP/xyz010001-081e7db0", "Hangup extension") in new stack
[Nov  5 13:02:40] WARNING[8493]: pbx.c:2470 __ast_pbx_run: Channel 'SIP/xyz010001-081e7db0' sent into invalid extension 's' in context 'xyz', but no invalid handler
devel*CLI>
   -- Stopped music on hold on SIP/poly_650_01-081e9340
 == SIP/poly_650_01-081e9340 got tired of being parked
By: Steve Murphy (murf) 2008-11-05 13:11:09.000-0600

No h exten?

I see:

   -- Executing [h@xyz:1] NoOp("SIP/xyz010001-081e7db0", "Hangup extension") in new stack
[Nov 5 13:02:40] WARNING[8493]: pbx.c:2470 __ast_pbx_run: Channel 'SIP/xyz010001-081e7db0' sent into invalid extension 's' in context 'xyz', but no invalid handler
devel*CLI>


I will see if I get the same;
By: mdu113 (mdu113) 2008-11-05 13:54:42.000-0600

Right. That's hangup by the parkee. We're talking about 'h' extension executed at parked channel hangup, aren't we? That should say 'Parking H extension'
By: Steve Murphy (murf) 2008-11-05 14:16:33.000-0600

Normally, the h-exten should get run on the guy that's running the pbx.
Normally, this is the "chan" in a bridged conversation, (not the "peer"),
so only one of the two gets the h-exten run.

You can call dial and ask it run the h-exten on the peer in a dial, but
I think this is a trunk/1.6 feature.

AFAICT, there has never been a special h-exten execution for just a parked
channel; it usually only gets run on one channel. In the 4 early-hangup scenarios,
It would be A, since he initiated the call in all four cases.


By: mdu113 (mdu113) 2008-11-05 15:06:30.000-0600

Correct me if I'm wrong, but I think that the channel given to the parking thread looses its chan/peer status, doesn't it?
I'm talking about 'h' extension for channel under parking thread control.
I don't know if it's ever been implemented. Up until now I actually didn't know that there's a difference between executing 'h' for parked channel and for regular channel, so it was obvious to me it should work and it came as a surprise when it didn't.
Anyway I believe it's very inconsistent with the rest of asterisk dialplan.
Is it too hard to make parking thread to execute 'h' extension? may be with restricted set of applications supported?
By: Steve Murphy (murf) 2008-11-10 14:46:13.000-0600

I have the fixes on http://svn.digium.com/svn/asterisk/team/murf/masqpark;
all paths behave the same now.

I have the code also under reviewboard; it needs a reviewer.

Now, the biggest problem with this code is that an h-exten gets run immediately
when the parker gets hung up. When A calls B, and A transfers, that's fine.
But when A calls B, and B parks the call, then that's not fine; basically
A's call is the one that's getting the h-exten run, but under the channel
name "Parked/<chan>/1-1<ZOMBIE>.

I'm going to have to think about this.


The good news is that all channels get hungup and not left in a hung state,
and everything works fine, with no memory messups.
By: mdu113 (mdu113) 2008-11-11 16:57:51.000-0600

I did a quick testing and can't see any problems. Everything seems to work fine with the exception that 'h' extension is still not being executed on parked channel hangup
By: Steve Murphy (murf) 2008-12-23 19:39:44.000-0600

OK, I've commit the masq_park branch to 1.4, and adapted it to trunk, and 1.6.0 and 1.6.1, see revs (1.4) 166093, (trunk) 166665, (1.6.0) 166729, (1.6.1) 166730,
in the which I removed all the KEEPALIVE stuff; and in trunk and 1.6.x, even
the consts are removed. This gets rid of the crashes and hangups, etc.

But, it leaves us with strange behavior concerning CDRs and h-extens, most
notably when "B parks A", after A calls B (an incoming call situation).
In these cases, the 'h' exten is run at that point in time where B achieves the
park and is hung up; here the h-exten is run for the A channel, and that channel has it name as "Parked/DAHDI/1-1<ZOMBIE>". It might seem strange that when B
hangs up, you get the 'h' exten run on the A channel, but... that's the way it is.

In the past, you'd have gotten no h-exten run at all in some situations.

In conference with Russell and Josh Colp, we decided that fixing this in 1.4
or 1.6.x would be impractical for the following reasons:

1. Any change would be invasive and change the basic structure of how things are being done. Such extensive changes would not be appropriate for a release.
2. To be honest, the fix is not yet obvious or forthcoming; we'll think about it. If anyone has any bright ideas, let me know, but you'd better explain how
your cool fix will involve masquerades, and not louse up all the other places it is done, and not louse up CDR generation. Good Luck!

I marked this bug as "not fixable", but I could also have marked as "won't fix"...
It's kinda a cross between the two. Whatever the fix entails, I suggest that it entail a different structuring of asterisk channel information, or you'll end up in the same morass that CDR's are in. Also, the fix should involve a fairly good specification for hangup exten execution in situations that involve parks, xfers, and conferences!