[Home]

Summary:ASTERISK-04733: [patch] allows asterisk to send SIGHUP to child AGI scripts on hangup
Reporter:Matthew Nicholson (mnicholson)Labels:
Date Opened:2005-07-29 15:59:05Date Closed:2011-06-07 14:10:15
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Resources/res_agi
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) agi_sighup.diff.txt
Description:Currently Asterisk sends SIGHUP on hangup to child AGI scripts.  The scripts do not recieve the signal because Asterisk has blocked the SIGHUP signal and the child inherits this signal mask.  This patch unblocks signals before exec'ing AGI scripts.

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

This patch should fix a bug may perl users have been seeing where their AGI scripts continue to run long after the user hangs up.
Comments:By: Olle Johansson (oej) 2005-07-31 10:44:56

This seems like an important patch, thank you!

By: Olle Johansson (oej) 2005-07-31 10:45:13

Is this needed for stable as well?

By: Matthew Nicholson (mnicholson) 2005-08-01 10:01:33

It does look like this is needed for stable as well.  Asterisk blocks the signals during start up.  So when modules spawn threads they get the same signal mask.  When asterisk is finished starting up it unblocks the signals, but all the modules stay blocked.  Glancing over the code, it looks like both stable and head suffer from this issue.

By: Mark Spencer (markster) 2005-08-03 00:13:07

Fixed in CVS head.

By: Russell Bryant (russell) 2005-08-04 18:48:48

fixed in 1.0 as well

By: Clod Patry (junky) 2005-08-18 08:46:23

That patch broke behavior of DeadAGI.

I was playing with 2005-08-17 16:35:53.

I was running an DeadAGI, and it was killed before the end of my script. I was wondering what could caused that.
I reverted that patch, and now it works.

Apparently, i'm not the only one to have that problem: http://lists.digium.com/pipermail/asterisk-dev/2005-August/014551.html

If that could help:
asterisk*CLI> show version files res_agi.c
File                      Revision
----                      --------
res_agi.c                 Revision: 1.46


I make a call, make a record, then hangup, right after my record.
here's agi debug:
[...]
[Aug 18 09:45:50] AGI Rx << RECORD FILE /tmp/123456 gsm "123456789*#" 60000 BEEP s=5
[Aug 18 09:45:50]     -- Playing 'beep' (language 'fr')
[Aug 18 09:45:52] AGI Tx >> 200 result=0 (hangup) endpos=12640
[Aug 18 09:45:52]   == Spawn extension (test_sip, 1002, 1) exited non-zero on 'SIP/100-4243'



Could we review that patch, cause it has huge impact on both HEAD and stable.

By: Michael Jerris (mikej) 2005-08-18 09:05:02

How does it work if you pass the dead param from agi_exec_full to launch script, then wrap:
+
+ /* unblock important signal handlers */
+ if(sigfillset(&signal_set) || pthread_sigmask(SIG_UNBLOCK, &signal_set, NULL)) {
+ ast_log(LOG_WARNING, "unable to unblock signals for AGI script: %s\n", strerror(errno));
+ exit(1);
+ }
+

in an if(!dead) ?

By: Clod Patry (junky) 2005-08-18 09:11:47

it works if you fully reverted the patch.
Which means if you replace the code just after the /* Notify process */ too.

By: Michael Jerris (mikej) 2005-08-18 09:13:45

I am unclear if we should be controlling this via the unmasking like that, or in agi_run, via not sending the signals on hangup for deadagi??

By: Matthew Nicholson (mnicholson) 2005-08-18 09:15:37

Is that how DeadAGI is supposed to work?  I thought DeadAGI was supposed to be executed *after* the channel was already hung up.  In any event, you can ignore or block the HUP signal to keep your script from terminating, or you can write a custom signal handler to detect hangups.

By: Michael Jerris (mikej) 2005-08-18 09:16:41

the stuff after  /* Notify process */ just adds a warning, does not change behavior.. .

By: Clod Patry (junky) 2005-08-18 09:18:26

matt: not exactly.
cause if u do a script, which will make a sleep(60); and during that sleep, do a ps aux|grep perl, you won't see your script running anymore.

Or another test, i made, is just make a loop [1,1000000], you will see, with your patch, the loop will terminated at anything, like 125,563,133, but never at the end.

By: Michael Jerris (mikej) 2005-08-18 09:20:37

A deadagi script should never be killed on hangup, so we need to handl this in asterisk code.

By: Sławomir Małota (malot) 2005-08-18 09:23:28

I don't know if DeadAgi is suposed to work in this way, but it is very useful and I'm sure people are using it.
It is nice feature if you get control back in your script after hangup to record some data, mail recorded file or something like that.

By: Matthew Nicholson (mnicholson) 2005-08-18 09:28:39

junky: I don't understand what point you are trying to make with that example.

MikeJ: are you sure this is the right behavior for DeadAGI?  I don't see how this is any different than just manually ignoring the signal in your script.

Also once you get that SIGHUP asterisk is nolonger listening for AGI stuff anyway so you can nolonger communicate with asterisk.  I think sending SIGHUP is the right behavior here.

By: Michael Jerris (mikej) 2005-08-18 09:39:32

Yes, the script should always keep running, we should not require handlers in the script to keep it running.  That is what deadagi is for.

By: Sławomir Małota (malot) 2005-08-18 09:39:51

mnicholson: That is the point, that DeadAgi before patch could communicate with asterisk after Hangup. You could read channel variables etc.

By: Matthew Nicholson (mnicholson) 2005-08-18 10:54:49

malot: I do not see my patch changing that behavior.  The patch merely changes signal masks and adds a warning message.

MikeJ: DeadAGI is for executing AGI scripts on *hungup* channels.  My patch fixed a bug.  Even before my patch asterisk sent SIGHUP to DeadAGI scripts, they just didn't get it because of the signal masks.

Also once asterisk goes away, you script will recieve a SIGPIPE when you try to send commands to asterisk.  This patch does have the effect that your script will no longer block that signal and be terminated.  If you do not want that to happend you will have to block, ignore, or handle SIGPIPE as well as SIGHUP.  Alternatively, you could use your SIGHUP handler to set a flag and then not send asterisk any more data.

If you would like the same behavior as before the patch, ignore SIGHUP and SIGPIPE in your script.

By: Donny Kavanagh (donnyk) 2005-08-18 11:08:28

I have many scripts, state machines if you will, which are entire systems written in agi,  mostly php-agi.  For me, when agi() was called.... my state machine within the script detects hangup, and when it does, it runs some cleanup code, and then before it quits, issues a agi->hangup, which then removes any control the agi had of asterisk.  Then the script termintes, all this after the user has physically hung up on the other end.  I feel that many people would have entire systems contained within agi, written as state machines.  This patch is obviously going to affect that?  Deadagi is not an option as the script starts while the call is active.  How can we solve this issue, but not affect the current way its been functioning.

By: Donny Kavanagh (donnyk) 2005-08-18 11:17:00

Looking into this further, at least when it comes to using php for agi, it will require custom builds of php (with --enable-pcntl) to be able to trap SIGHUP, such that we can return agi to its original behavior.  Can we perhaps add a flag to agi() to send (or not send) a signal when the user hangs up?  I think this would allow users to retain their original code without being forced to make changes to return it to how it worked before.

By: Matthew Nicholson (mnicholson) 2005-08-18 11:26:22

I suppose that is an option.  Another option is to write a small program that adjusts the signal masks and then exec's the AGI script.

By: ewieling (ewieling) 2005-08-18 11:28:28

This patch changes behavour and should not be in 1.0.x because of that.

By: Donny Kavanagh (donnyk) 2005-08-18 11:51:59

Writing another program sounds fine to you, and even to me, but think lowest common denominator here.  75% of the asterisk user base is lucky to get it compiled, not alone write extra progams to counter changes in the way * operates.  I think the best solution is an option for agi() signal the script on hangup (yes/no).  I would even take yes as the default option, at least then i would only have to make changes to how my agi is called rather then recompile php (to support traping sighup) and then making code changes to all my agi's to trap it.

By: Clod Patry (junky) 2005-08-18 11:54:15

With that patch (or current HEAD, like its already in the tree),
try:
$SIG{HUP} = "IGNORE";

at the beginning of your script, it should fix your problem.

By: Donny Kavanagh (donnyk) 2005-08-18 11:57:36

My agi is all php, not perl.  Php seems to require a recompile to enable process control functions which would not be the default config.

By: Clod Patry (junky) 2005-08-18 11:59:36

what about you  malot ?

By: Sławomir Małota (malot) 2005-08-18 12:01:31

I'm also using php agi and make changes to all my scripts is not my favourite way to solve this problem:)

By: Matthew Nicholson (mnicholson) 2005-08-18 12:03:22

I do not think an option should be added to HEAD or STABLE.  Perhaps a proxy program should be destributed with CVS HEAD, but we don't need an option to turn off the signals.  As for STABLE, it would be best to revert to the old behavior, and anyone who needs the new behavior can manually apply this patch.

By: Donny Kavanagh (donnyk) 2005-08-18 12:10:14

I dont think its required that scripts be signaled to terminate, furthermore a proxy program adds unneeded complexity.  There are plenty of ways to detect a hangup within agi without being notified this way.  I feel an option on agi() is waranted.

By: ewieling (ewieling) 2005-08-18 12:13:37

I think this patch should go into CVS-HEAD, but not in 1.0.x.  CVS-HEAD already has many changes that make it incompatable with 1.0.x stuff.  See the upgrade readme file of CVS-HEAD for a list.

Sending a SIGHUP to an AGI script when the channel is hungup is The Right Thing To Do.

By: Donny Kavanagh (donnyk) 2005-08-18 12:16:09

Agreed, however there may be some situations (eg php) where ignoring SIGHUP is more complex then it should be, hence an option to disable this and handle via checking for hangup within the agi would be ideal.

By: Matthew Nicholson (mnicholson) 2005-08-18 13:37:48

A small proxy script executed as follows would be much better suited than an option I think.

AGI(no-signals-proxy.agi|real_script.php|arg1|arg2)

By: Donny Kavanagh (donnyk) 2005-08-18 13:40:24

Regardless this needs to be removed from cvs-stable.

By: Donny Kavanagh (donnyk) 2005-08-20 17:33:01

So whats going on, are we pretending its not a problem to change functionality in cvs-stable?  Has this been reverted?  An update please.

By: Mark Spencer (markster) 2005-08-21 12:51:03

I'm going to leave this up to Russell for 1.0, but for CVS head, it's definitely the right behavior to be sure a SIGHUP is received on a hangup.  If DeadAGI is used, then you shouldn't get the signal until the end.  In either case you can just ignore SIGHUP.

By: Donny Kavanagh (donnyk) 2005-08-21 14:34:10

"Right behavior" and "the way it was" are very different.

With 1.0.9 i have agi scrips which detect hangup, and do database cleanup etc before the scripts terminate.  With CVS-STABLE that will no longer be possible.  I will instead have to keep my state of the agi within a asterisk var, and then when i get sighup'ed call deadagi, passing in that state to peoperly clean up.

People who had working agi scripts are going to have them broken by this change.
Also DeadAGI used to work when the call was active, and when there was a hangup you still kept total control.  Now if DeadAGI is active and a call is terminated the script within DeadAGI will terminate, you will have to call DeadAGI again to finish what you were doing.

So i think this change in behavior is going to cause alot of hassle who have already developed agi applications.  Its a total change which removes the ability to do cleanup within a AGI() call, unless your perticular scripting language of choice allows you to ignore sighup.  And of course you are aware that you need to do so, which most people wont be.



By: Russell Bryant (russell) 2005-08-22 01:46:09

I have reverted this to restore original behvior for 1.0.

Can someone please write something up about this for UPGRADE.txt to warn people when they upgrade to 1.2?

By: Digium Subversion (svnbot) 2008-01-15 15:43:27.000-0600

Repository: asterisk
Revision: 6264

U   trunk/res/res_agi.c

------------------------------------------------------------------------
r6264 | markster | 2008-01-15 15:43:27 -0600 (Tue, 15 Jan 2008) | 2 lines

Fix sighup with AGI (bug ASTERISK-4733)

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

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

By: Digium Subversion (svnbot) 2008-01-15 15:43:44.000-0600

Repository: asterisk
Revision: 6283

U   branches/v1-0/CHANGES
U   branches/v1-0/res/res_agi.c

------------------------------------------------------------------------
r6283 | russell | 2008-01-15 15:43:44 -0600 (Tue, 15 Jan 2008) | 2 lines

unmask SIGHUP before exec'ing AGI scripts (bug ASTERISK-4733)

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

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

By: Digium Subversion (svnbot) 2008-01-15 15:44:38.000-0600

Repository: asterisk
Revision: 6347

U   branches/v1-0/res/res_agi.c

------------------------------------------------------------------------
r6347 | russell | 2008-01-15 15:44:37 -0600 (Tue, 15 Jan 2008) | 2 lines

revert SIGHUP patch to restore original behavior for 1.0 (bug ASTERISK-4733)

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

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