Summary:	ASTERISK-09499: [patch] Add 'e' extension to handle application errors
Reporter:	Matthew Nicholson (mnicholson)	Labels:
Date Opened:	2007-05-23 14:10:45	Date Closed:	2007-10-03 16:54:22
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20071003__bug9785__3.diff.txt ( 1) e-exten16.diff
Description:	Currently when an application encounters an error asterisk hangs up the channel.  This patch adds a special 'e' extension (like the 'i' extension) to gracefully handle these errors instead of hanging up the channel. This patch defines three new macros: AST_PBX_HANGUP (-1), AST_PBX_OK (0), and AST_PBX_ERROR (1).  If an application returns AST_PBX_ERROR then the 'e' extension is triggered. If an application executing on the 'e' exten encounters an error, the call is terminated similar to how errors are handled now. ****** ADDITIONAL INFORMATION ****** All of the current asterisk apps will need to be audited and modified to make use of this patch.  Currently if an application recieves invalid arguments or encounters any number of other random errors, the channel is abruptly hungup.
Comments:	By: Tilghman Lesher (tilghman) 2007-05-23 20:59:13 You actually can do the same thing already, by wrapping an application with a TryExec.  i.e. exten => s,1,TryExec(Authenticate) exten => s,n,GotoIf($[${TRYSTATUS} != SUCCESS]?e,1) but TryExec gives you far more control, as you can do different things based upon which application failed. By: Matthew Nicholson (mnicholson) 2007-05-23 23:47:36 e-exten2.diff adds documentation to doc/extensions.tex By: Matthew Nicholson (mnicholson) 2007-05-23 23:55:02 Yes, it appears TryExec would work as well, but I like my solution better.  Using TryExec is a somewhat 'C' style to do things, where as the 'e' exten works more like C++ or Java. I think using the 'e' exten results in a much cleaner dialplan, although it does not completely replace TryExec.  Either way, I don't think just hanging up on users without warning is a sane practice. By: Matthew Nicholson (mnicholson) 2007-05-23 23:57:18 I probably should have phrased the above comments on C error handling vs C++ error handling differently.  Basically it is the difference between checking return codes and using exceptions, except the way it is now, if you don't check for failed return codes, your program is terminated.  It's like exception handling with out the handling part. By: Tilghman Lesher (tilghman) 2007-05-24 08:03:16 Okay, but if you're going to do it that way, then you need to set channel variables to the extension and application which failed, similar to how the "i" extension logic sets INVALID_EXTEN, so that you can deal with the way that different things fail (and take appropriate action).  The way the patch is now, it's one exception, with no way to detect (from within the dialplan) which extension/application failed. By: Matthew Nicholson (mnicholson) 2007-05-24 20:35:48 e-exten12.diff adds the EXCEPTION function which can be used to store and retrieve information about the current EXCEPTION. There are still a few issues with the code so I will upload a new file soon. By: Matthew Nicholson (mnicholson) 2007-05-24 20:54:27 e-exten13.diff makes asterisk use the 'e' exten if no 'i' exten is available and also clears any existing exceptions before creating a new one. By: Leif Madsen (lmadsen) 2007-06-12 09:48:33 I like this idea a lot. Wrapping every application (or most) in a TryExec() seems to make the dialplan a bit harder to read, and also you have to remember to wrap everything that could error out into a TryExec() (and handle the exception below it in multiple places -- although I'm sure you could easily create a GoSub() to handle the errors -- which I actually do now (just not with TryExec())). By: Michiel van Baak (mvanbaak) 2007-09-08 06:31:03 Can you update this patch to current trunk ? The .tex documentation now live in doc/tex/ The pbx.c part has 2 hunks failing, uploading .rej file now By: Matthew Nicholson (mnicholson) 2007-09-14 11:05:32 Updated. By: Matthew Nicholson (mnicholson) 2007-09-14 11:21:26 e-exten15.diff added, e-exten14.diff was missing files, e-exten15.diff should be complete. By: Michiel van Baak (mvanbaak) 2007-09-23 05:16:55 Thank you. Works great here. Any dev can have a look at this ? By: Michiel van Baak (mvanbaak) 2007-09-23 05:32:03 Oops, remind me to add -enable-dev-mode when testing patches. This one wont compile with that configure option:   [CC] pbx.c -> pbx.o cc1: warnings being treated as errors pbx.c: In function '__ast_pbx_run': pbx.c:2461: warning: ISO C90 forbids mixed declarations and code pbx.c:2521: warning: ISO C90 forbids mixed declarations and code make[1]: *** [pbx.o] Error 1 make: *** [main] Error 2 By: Matthew Nicholson (mnicholson) 2007-09-24 10:13:03 I thought asterisk was C99 which allows that.  Any comments? By: Tilghman Lesher (tilghman) 2007-10-03 11:58:31 I have revised the code a bit.  I really didn't like the datastore functions being public, so I have changed the code to properly encapsulate those functions and made a single function to create the exception condition. Also, I have added a dialplan function EXCEPTION, which I think was an oversight:  while you had code to go to the "e" extension, there was no way to query the type of error or where the exception came from. By: Matthew Nicholson (mnicholson) 2007-10-03 13:13:40 There was already a dialplan function to query the exception. By: Matthew Nicholson (mnicholson) 2007-10-03 13:15:41 It is missing from patch 15 though. By: Tilghman Lesher (tilghman) 2007-10-03 13:28:16 Now that you mention it, I see it in 13.  I'm not so sure about being able to set the fields, though.  I suppose we could create a Exception app whose sole purpose would be to set a type (context, extension, and priority should be set to the place where Exception was called) and immediately jump to the "e" extension. There we go.  RaiseException. By: Matthew Nicholson (mnicholson) 2007-10-03 13:40:41 The reason the internal exception structure was open to the rest of the code is so that eventually, applications can throw exceptions and associate data with them.  I am not opposed to streamlining the interface, but I think applications should be allowed to set their own data (I was going to add this in a different patch). By: Matthew Nicholson (mnicholson) 2007-10-03 13:43:22 Added patch 16 with the function. By: Tilghman Lesher (tilghman) 2007-10-03 15:03:48 I would suggest just using the RaiseException app (or the underlying public function that I put in my code:  pbx_builtin_setexception). By: Digium Subversion (svnbot) 2007-10-03 16:54:22 Repository: asterisk Revision: 84580 U   trunk/doc/tex/extensions.tex U   trunk/include/asterisk/pbx.h U   trunk/main/pbx.c ------------------------------------------------------------------------ r84580 | tilghman | 2007-10-03 16:54:21 -0500 (Wed, 03 Oct 2007) | 2 lines Create a universal exception handling extension, "e" (closes issue ASTERISK-9499) ------------------------------------------------------------------------