Re: [cgiapp] Re: CGI::App 3.2 proposed release available

[Steve Hay <suppressed>]

Mark Stosberg wrote:

>>The only other thing that I'd still really like is for something to be 
>>done about the problems with exception handling that the catch/rethrow 
>>(eval/die) in run() causes.  This has been discussed at length before in 
>>several threads (see the archives), but nothing has ever happened.  Is 
>>it ever going to?
>>    
>>
>
>This is not something I am fresh on or have an opinion on at the moment.
>I know that if there is a complete patch, something is more likely to
>happen, though. :)
>
I'll accept that invitation!  First, a quick refresher of what the 
problem is:

The code in C::A that causes problems for my exception handling is these 
two lines in the run() method:

    my $body = eval { $autoload_mode ? $self->$rmeth($rm) : 
$self->$rmeth() };
    die "Error executing run mode '$rm': $@" if $@;

In the case of exceptions thrown by the run mode method in question (as 
opposed to C::A itself having an error, e.g. the run mode method can't 
be dispatched), catching (eval) and then rethrowing (die) the exception 
in this way causes two problems for me:

Problem 1. It assumes that the exception in $@ is a string.  I'm trying 
to setup a system based around Exception::Class that uses Perl's 
exception objects.  The above code stringifies the exception object in 
suppressed  (Exception::Class' objects overload stringification, so that at 
least a decent error message is still produced, but all of the other 
information on the object is lost.)

Problem 2. It adds an extra frame into the stack trace, which confuses 
modules like Exception::Class as to where the exception comes from.  
(That is, E::C now thinks that the error was in C::A::run() instead of 
somewhere inside the run mode method where it really was.)

There are various ways to improve things here.  Here's a couple that 
spring to mind (with patches against the pre-release 3.2):

Solution 1.

The whole catch/rethrow thing above is almost pointless.  It clearly 
makes no difference if there are no errors, and it doesn't stop the code 
from die()'ing if an error does occur either.

I believe the only point of the catch/rethrow is to give a nicer error 
message ("Error executing run mode ...") than whatever Perl's default 
error message would be in the case where the run mode method can't be 
dispatched.

We could simply remove the catch/rethrow altogether, and leave it up to 
Perl to say that the method can't be dispatched if that's the case:

=====
--- Application.pm.orig    2003-11-25 14:01:29.053896600 +0000
+++ Application.pm    2003-11-25 15:46:44.971140300 +0000
@@ -150,8 +150,7 @@
     }
 
     # Process run mode!
-    my $body = eval { $autoload_mode ? $self->$rmeth($rm) : 
$self->$rmeth() };
-    die "Error executing run mode '$rm': $@" if $@;
+    my $body = $autoload_mode ? $self->$rmeth($rm) : $self->$rmeth();
 
     # Make sure that $body is not undefined (supress 'uninitialized 
value' warnings)
     $body = "" unless defined $body;
=====

Solution 2.

Slightly nicer than that:  Pick off those cases where the run mode 
method dispatch might fail and use the catch/rethrow to give a nice 
error in those cases.  Don't use it otherwise.

We can use the UNIVERSAL::can() method to see whether the run mode 
method dispatch should work.  If $self->can($rmeth) is true then the 
method definitely can be dispatched so any exception raised will be from 
within that method and we shouldn't mess with it.  If $self->can($rmeth) 
is false then we try the method dispatch anyway, just in case the run 
mode method is AUTOLOAD'ed (which UNIVERSAL::can() can't pick up on), 
but within the protection of an eval { ... } just in case the dispatch 
fails, in which case we throw a (nicer) error message ourselves as we 
currently do:

=====
--- Application.pm.orig    2003-11-25 14:01:29.053896600 +0000
+++ Application.pm    2003-11-25 15:51:36.922114000 +0000
@@ -150,8 +150,14 @@
     }
 
     # Process run mode!
-    my $body = eval { $autoload_mode ? $self->$rmeth($rm) : 
$self->$rmeth() };
-    die "Error executing run mode '$rm': $@" if $@;
+    my $body;
+    if ($self->can($rmeth)) {
+        $body = $autoload_mode ? $self->$rmeth($rm) : $self->$rmeth();
+    }
+    else {
+        $body = eval { $autoload_mode ? $self->$rmeth($rm) : 
$self->$rmeth() };
+        die "Error executing run mode '$rm': $@" if $@;
+    }
 
     # Make sure that $body is not undefined (supress 'uninitialized 
value' warnings)
     $body = "" unless defined $body;
=====

Solution 3.

Retain the exception catching in all circumstances, but provide (yet 
another) overridable method, cgiapp_exception(), to deal with the 
exception that we've just caught.  The default implementation of the new 
method would, of course, be the current behaviour.

In my case, I would override cgiapp_exception() to simply die with the 
exception (object) that is passed to it, without tampering with it in 
any way.

=====
--- Application.pm.orig    2003-11-25 14:01:29.053896600 +0000
+++ Application.pm    2003-11-25 15:59:40.869378700 +0000
@@ -151,7 +151,10 @@
 
     # Process run mode!
     my $body = eval { $autoload_mode ? $self->$rmeth($rm) : 
$self->$rmeth() };
-    die "Error executing run mode '$rm': $@" if $@;
+    if ($@) {
+        $self->cgiapp_exception($rm, $@);
+        return; # in case cgiapp_exception returns
+    }
 
     # Make sure that $body is not undefined (supress 'uninitialized 
value' warnings)
     $body = "" unless defined $body;
@@ -224,6 +227,15 @@
 }
 
 
+sub cgiapp_exception {
+    my $self = shift;
+    my $rm   = shift;
+    my $err  = shift;
+
+    die "Error executing run mode '$rm': $err";
+}
+
+
 sub setup {
     my $self = shift;
 
=====

Of these three solutions, I prefer Solution 2.

It's a nice compromise between the current behaviour of catching too 
much and the behaviour in Solution 1 of catching too little.

Solution 3 only solves the first of the two problems outlined at the 
start of this e-mail; the second problem (confusing Exception::Class and 
similar about the origin of the exception) still remains.  It could 
perhaps be addressed by a patch for Exception:Class to provide some 
interface which the cgiapp_exception() override could take advantage of 
to have this extra stack frame ignored, but Solution 2 just avoids this 
confusion in the first place.

Which solution do you prefer?  What other solutions are there?

- Steve



------------------------------------------------
Radan Computational Ltd.

The information contained in this message and any files transmitted with it are confidential and intended for the addressee(s) only.  If you have received this message in error or there are any problems, please notify the sender immediately.  The unauthorized use, disclosure, copying or alteration of this message is strictly forbidden.  Note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of Radan Computational Ltd.  The recipient(s) of this message should check it and any attached files for viruses: Radan Computational will accept no liability for any damage caused by any virus transmitted by this email.


---------------------------------------------------------------------
Web Archive:  http://www.mail-archive.com/suppressed/
              http://marc.theaimsgroup.com/?l=cgiapp&r=1&w=2
To unsubscribe, e-mail: suppressed
For additional commands, e-mail: suppressed