Menu
▾
▴
Re: [WWdevel] summary of $SIG{__DIE__} and $SIG{__WARN__} usage in Translator.pm
|
From: Sam H. <sh...@ma...> - 2005-07-29 18:43:40
|
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Jul 29, 2005, at 14:24, Michael E. Gage wrote:
> 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.
I'll implement that then.
>> 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> /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).
As far as I can tell, the actual reference is preserved until the
point at which it is printed. Our outermost error reporting code,
which generates the "Software error" text, assumes that the argument
to die is a string, not any sort of reference or object. It's up to
us (in Hardcopy.pm in this case) to deal with non-string exceptions.
So we'll be fine as long as we fix this part to restore the original
handlers when we're done, or better yet, use localized handlers --
we'll need that if we ever want this to be thread safe.
>> # 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> /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.
That sounds good. I think it would be bad if %errorTable had to be
local, since it would leak into called subroutines. I'm also unclear
as to whether it's robust. Is it possible for there to be two errors
that have the same value for $error but different values for $fullerror?
Maybe Davide would like to give some input on this one.
>> 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 @_};
I think we avoid a level of subroutine call, since there's no
anonymous sub getting called.
- -sam
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (Darwin)
iD8DBQFC6njO14CQX+2WvVgRAhKZAJ9MiprIQ+EaZHG4/CD1ILmjc0sdowCdFXVM
5MpEOuQZd/kUDSQzoSMXLng=
=yrOi
-----END PGP SIGNATURE-----
|
