Re: [WWdevel] summary of $SIG{__DIE__} and $SIG{__WARN__} usage in Translator.pm

[Michael E. G. <ga...@ma...>]

On Jul 29, 2005, at 1:00 PM, Sam Hathaway wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Mike,
>
> Here is my assessment of what's going on with $SIG{__WARN__} and 
> $SIG{__DIE__} in Translator.pm.
>
> evaluate_modules() uses:
>
>     local $SIG{__DIE__} = "DEFAULT";
>
> to set the default handler for the duration of the subroutine. It then 
> goes onto use eval STRING to read in PG modules, and checks $@ for any 
> error messages. This is not harmful, and it saves us a little time 
> since the handler defined in Apache::WeBWorK would waste time 
> generating a backtrace before calling the default handler. (But see 
> below for why I think we should just let the Apache::WeBWorK handlers 
> run.)
>
> At the beginning of translate(), we see a few things:
>
>     # reset the error detection
>     $SIG{__WARN__} = sub {CORE::warn(@_) } unless ref($SIG{__WARN__}) 
> =~/CODE/;
>     my $save_SIG_warn_trap = $SIG{__WARN__};
>     #FIXME  -- this may not work with the xmlrpc access
>     # this formats the error message within the existing warn message.
>     $SIG{__WARN__} = sub 
> {&$save_SIG_warn_trap(PG_errorMessage('message',@_))};
>     #$SIG{__WARN__} = sub {CORE::warn(PG_errorMessage('message',@_))};
>     my $save_SIG_die_trap = $SIG{__DIE__};
>     $SIG{__DIE__} = sub {CORE::die(PG_errorMessage('traceback',@_))};
>
> This is a trouble spot, since it sets the handlers without using 
> "local". From what I can tell, what we WANT to do here is provide a 
> level of preprocessing to any warning or exception that comes out. We 
> may also want to avoid calling the Apache::WeBWorK warn/die handlers, 
> although I think we should leave them alone and assume our caller has 
> some way they want to collect errors and warnings. (For example, 
> WeBWorK::PG::Local has its own warn handler that accumulates warnings 
> and returns them as part of the WeBWorK::PG object.) It seems to me 
> that the simplest way to accomplish this is:
>
>     # reset the error detection
>     my $outer_sig_warn = $SIG{__WARN__};
>     local $SIG{__WARN__} = sub {
>         my $munged = PG_errorMessage('message', $_[0]);
>         ref $outer_sig_warn eq "CODE" ? &$outer_sig_warn($munged) : 
> warn $munged;
>     };
>     my $outer_sig_die = $SIG{__DIE__};
>     local $SIG{__DIE__} = sub {
>         my $munged = PG_errorMessage('traceback', $_[0]);
>         ref $outer_sig_die eq "CODE" ? &$outer_sig_die($munged) : die 
> $munged;
>     };
>
> This doesn't deal with the case where the handlers are set to string 
> names of subroutines, but that could be added. It also doesn't handle 
> "IGNORE", but that could be added as well.
>
OK.  I'd made this change also, omitting calling the outer DIE and WARN 
handlers.  I think your idea is better.
> in process_answers(), we have:
>
>     local %errorTable;
>     $SIG{__DIE__} = sub {
>         #
>         #  Get full traceback, but save it in local variable so that
>         #  we can add it later.  This is because some evaluators use
>         #  eval to trap errors and then report them in the message
>         #  column of the results table, and we don't want to include
>         #  the traceback there.
>         #
>         my $fullerror = PG_errorMessage('traceback',@_);
>         my ($error,$traceback) = split /\n/, $fullerror, 2;
>         $fullerror =~ s/\n   /<BR>&nbsp;&nbsp;&nbsp;/g; $fullerror =~ 
> s/\n/<BR>/g;
>         $error .= "\n";
>         $errorTable{$error} = $fullerror;
>         CORE::die($error);
>     };
>     # reset the error detection
>     my $save_SIG_warn_trap = $SIG{__WARN__};
>     $save_SIG_warn_trap = sub {CORE::warn @_} unless  
> ref($save_SIG_warn_trap) =~/CODE/;
>     $SIG{__WARN__} = sub 
> {&$save_SIG_warn_trap(PG_errorMessage('message',@_))};
>     my $save_SIG_die_trap = $SIG{__DIE__};
>
> and then later on, after the answers are evaluated:
>
>     $SIG{__DIE__} = $save_SIG_die_trap;
>     $SIG{__WARN__} = $save_SIG_warn_trap;
>
> First of all, restoring the die handler doesn't work at all, since 
> $save_SIG_die_trap is set to the custom die handler we just installed 
> a few lines before. In addition, I think we can do the same thing here 
> that we did up above, inserting the code that accumulates exceptions. 
> Also, I think %errorTable can be a lexical.
>
This is the error that was causing the immediate trouble in Hardcopy.pm 
(the generation of the ARRAY string, but no ARRAY reference).
>     # reset the error detection
>     my $outer_sig_warn = $SIG{__WARN__};
>     local $SIG{__WARN__} = sub {
>         my $munged = PG_errorMessage('message', $_[0]);
>         ref $outer_sig_warn eq "CODE" ? &$outer_sig_warn($munged) : 
> warn $munged;
>     };
>     my %errorTable;
>     my $outer_sig_die = $SIG{__DIE__};
>     local $SIG{__DIE__} = sub {
>         my $fullerror = PG_errorMessage('traceback', $_[0]);
>         my ($error,$traceback) = split /\n/, $fullerror, 2;
>         $fullerror =~ s/\n   /<BR>&nbsp;&nbsp;&nbsp;/g; $fullerror =~ 
> s/\n/<BR>/g;
>         $error .= "\n";
>         $errorTable{$error} = $fullerror;
>         ref $outer_sig_die eq "CODE" ? &$outer_sig_die($error) : die 
> $error;
>     };
>
The other thing I did in my fix was to move the definition of the WARN 
and DIE handlers outside of the for loop.  %errorTable can be
moved outside the loop to.  I think this works ok.  I'm still thinking 
about the role of %errorTable and whether it has to be localize
for each answer.  If necessary it could be emptied out at the top of 
each loop.

> There are three more places where warn/die handlers get messed with - 
> -- PG_restricted_eval(), PG_macro_file_eval(), and PG_answer_eval(). 
> In each case, subroutine begins with:
>
>     my $save_SIG_warn_trap = $SIG{__WARN__};
>     $SIG{__WARN__} = sub { CORE::die @_};
>     my $save_SIG_die_trap = $SIG{__DIE__};
>     $SIG{__DIE__}= sub {CORE::die @_};
>
> then some code gets passed to eval STRING, and then:
>
>     $SIG{__DIE__} = $save_SIG_die_trap;
>     $SIG{__WARN__} = $save_SIG_warn_trap;
>
> We can replace these with the same type of code that's used in 
> evaluate_modules():
>
>     local $SIG{__WARN__} = "DEFAULT";
>     local $SIG{__DIE__} = "DEFAULT";
>
Is there an advantage to calling "DEFAULT" instead of
local $SIG{__WARN__} = sub { CORE::die @_};

One of us should make these changes and check in a new version of 
Translator.pm
Then we can test and refine from there if needed.

Take care,

Mike

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.4 (Darwin)
>
> iD8DBQFC6mDE14CQX+2WvVgRAmifAKCf2Mjte5IUWjE/FVINpt+CMZkeVwCePwGh
> 8EQzltAV6e4ObIZN8iRpLvU=
> =LagG
> -----END PGP SIGNATURE-----
>
>
> -------------------------------------------------------
> SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
> from IBM. Find simple to follow Roadmaps, straightforward articles,
> informative Webcasts and more! Get everything you need to get up to
> speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
> _______________________________________________
> OpenWeBWorK-Devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openwebwork-devel
>