Bug #20643 for Error: $Error::THROWN takes precedence over $@

Mon Jul 24 10:30:53 2006 Marek.Rouchal [...] gmx.net - Ticket created

Subject:

$Error::THROWN takes precedence over $@

I have perl-5.8.8, the latest Error (0.17) running on Solaris 8 Sparc and RedHat Enterprise Linux 3.0. I have this somewhat weird situation: I override CORE::GLOBAL::die to make sure all exceptions (from whatever Perl module they come from) are throwing some My::Error exceptions, which are derived from the Error base class. Reason: this way I can get the full stack trace of the exception. This however does not work perfectly for some unknown reason - I failed to find that out. Anyway, the resulting problem is this: try { ... some code that has its own eval { die ... }, where the die is redirected to My::Error->throw, such that $Error::THROWN contains the exception object (1) ... ... more code ... ... code that uses Carp::croak, and that mysteriously does not take the detour via My::Error->throw (2) ... ... more code, but not reached in this case... } catch Error with { ... the usual print and exit ... }; The problem is that the catch block will report the exception (1) and not (2). This is because of this code in Error.pm: $err = defined($Error::THROWN) ? $Error::THROWN : $@; ...which clearly prefers the content of Error::THROWN over $@. I am wondering why that does not read: if($ok) { next CATCHLOOP if $more; undef $err; undef $Error::THROWN; } elsif($@) { $err = ref($@) ? $@ : $Error::ObjectifyCallback->({'text' =>$@}); } elsif(defined $Error::THROWN) { # this is always an object $err = $Error::THROWN; } last CATCH; This way an actual exception caught by the eval{} will always be preferred, and the $Error::THROWN is just a fallback, or it may even be obsolete. Or do I miss anything fundamental here? Note that some Perl modules (AutoLoader, File::Temp) temporarily set $SIG{__DIE__} which seems to cause some confusion. I think overriding CORE::GLOBAL::die by a custom My::Error::dieReplacement(), which does a My::Error->throw is OK, but it is not perfect, as some exceptions (those by Carp::croak? I have seen the problem where (2) was XML::LibXML, which croaks on an XML parse error) do not seem to be redirectable this way. And abusing $SIG{__DIE__} for that purpose is explicitely discoraged in the Perl docs. Sorry, I cannot easily generate a test example - I hope you get the idea, if not, do not hesitate to reply. Cheers, Marek

Tue Aug 08 14:07:58 2006 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Mon Jul 24 10:30:53 2006, MAREKR wrote: Show quoted text

> Note that some Perl modules (AutoLoader, File::Temp) temporarily set > $SIG{__DIE__} which seems to cause some confusion. I think overriding > CORE::GLOBAL::die by a custom My::Error::dieReplacement(), which does > a My::Error->throw is OK, but it is not perfect, as some exceptions > (those by Carp::croak? I have seen the problem where (2) was > XML::LibXML, which croaks on an XML parse error) do not seem to be > redirectable this way. And abusing $SIG{__DIE__} for that purpose is > explicitely discoraged in the Perl docs.

Have you seen the new :warndie tag in Error version 0.17? That might be more what you're wanting to do here. Show quoted text

> Sorry, I cannot easily generate a test example - I hope you get the > idea, if not, do not hesitate to reply.

I've had a play about with the idea, but I'm afraid I can't easily replicate what you mean with the code. Take a look at [the attached], and let me know if that's what you mean. If not, feel free to modify it as required, and send it back. When I run it, the output I get is: ----- DIAG:0 DIAG:1 DIAG:2 eval{} returned a : Error from internal eval{} block at ./bug-rt_20643-test.pl line 27. but we'll ignore it DIAG:3 catch block has caught A different error from the try{} block at /usr/local/share/perl/5.8.8/Error.pm line 417. ----- Let me know how you get on with that... -- Paul Evans

#!/usr/bin/perl -w use strict; use Error qw(:try); use Carp; print STDERR "DIAG:0\n"; { no warnings; *CORE::GLOBAL::die = sub { print STDERR "Overridden die() handler activated\n"; my $e = $@; print STDERR "Overridden die() handler has caught $e\n"; throw Error( -text => "Exception from the overridden die() handler" ); }; } try { print STDERR "DIAG:1\n"; eval { print STDERR "DIAG:2\n"; die "Error from internal eval{} block"; warn "NOTREACH"; }; print STDERR "eval{} returned a " . ref($@) . ":\n$@\n"; print STDERR "but we'll ignore it\n"; print STDERR "DIAG:3\n"; Carp::croak( "A different error from the try{} block\n" ); warn "NOTREACH"; } catch Error with { my $e = shift; print STDERR "catch block has caught $e\n"; };

Tue Aug 08 14:09:22 2006 The RT System itself - Status changed from 'new' to 'open'

Wed Aug 09 09:33:51 2006 Marek.Rouchal [...] gmx.net - Correspondence added

On Tue Aug 08 14:07:58 2006, PEVANS wrote: Show quoted text

> Have you seen the new :warndie tag in Error version 0.17? That might

be Show quoted text

> more what you're wanting to do here.

Yes, but I also saw the objectify callback, and that is likely to become the way to go for me, since it allows me to turn the textual exceptions of die/croak into an exception object of my own class. But there is a major downside: I cannot get the stack trace! Only when I override the die (with CORE::GOBAL::die) I can use the caller() funtion in the override sub to determine the stack trace, and put that in the object, which is then thrown by CORE::die. That is the ultimate rationale for this ticket. Show quoted text

> > Sorry, I cannot easily generate a test example - I hope you get

the Show quoted text

> > idea, if not, do not hesitate to reply.

> > I've had a play about with the idea, but I'm afraid I can't easily > replicate what you mean with the code. Take a look at [the attached], > and let me know if that's what you mean. If not, feel free to modify

it Show quoted text

> as required, and send it back.

Many thanks - that was an excellent starting point for the adapted version, which you can find attached - it generates the error that I observed in my system originally. Major change: I had to put the CORE::GLOBAL::die definition in a BEGIN{} block, as I "use MyError", the derived class, where the override is done. And I put in some more diagnostics. But I do not understand why the override of die is only active in this script, and not in the Carp module - the second exception (Carp::croak) should also trigger the CORE::GLOBAL::die sub, but I do not see the diagnostic messages. Anyway, the good thing is that Perl seems to automatically disable the override of "die" withing the overriding sub, so we do not end up in a deep recursion. I tried to play with the order of the BEGIN{} block and the "use Carp" line, but that did not change anything. I must admit that I do not fully understand what is going on there - I hope you are more successful... Thanks a lot anyway for looking into this, Marek

#!/opt/perl_5.8.8/bin/perl -w use strict; print STDERR "DIAG:0: Start\n"; use Error qw(:try); BEGIN{ no warnings; *CORE::GLOBAL::die = sub { my $e = $_[0]; print STDERR "Overridden die() handler has caught '$e'\n"; throw Error::Simple("Exception thrown by the overridden die() handler: '$e'"); }; } use Carp; print STDERR "DIAG:0: Start\n"; try { print STDERR "DIAG:1: Start of the try{} block\n"; eval { print STDERR "DIAG:2: die 'Error from internal eval{} block'\n"; die "Error from internal eval{} block"; warn "NOTREACH"; }; print STDERR "eval{} returned a " . ref($@) . ":\n$@\n"; print STDERR "but we'll ignore it\n"; print STDERR "DIAG:3: throwing 'A different error from the try{} block'\n"; Carp::croak( "A different error from the try{} block\n" ); #die "A different error from the try{} block\n"; warn "NOTREACH"; } catch Error with { my $e = shift; print STDERR "catch block has caught: '$e'\n"; }; print STDERR "DIAG:4: we should see caught exception 'A different error from the try{} block'\n";

Thu Aug 17 16:11:26 2006 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Wed Aug 09 09:33:51 2006, MAREKR wrote: Show quoted text

> But there is a major downside: I cannot get the stack trace! Only when > I override the die (with CORE::GOBAL::die) I can use the caller() > funtion in the override sub to determine the stack trace, and put that > in the object, which is then thrown by CORE::die. That is the ultimate > rationale for this ticket.

$Error::DEBUG = 1; Now all Error-derived exceptions will store a stack trace. Show quoted text

> Many thanks - that was an excellent starting point for the adapted > version, which you can find attached - it generates the error that I > observed in my system originally. Major change: I had to put the > CORE::GLOBAL::die definition in a BEGIN{} block, as I "use MyError", > the derived class, where the override is done. And I put in some more > diagnostics.

I see your problem. Read "perldoc perlvars" on the $^S variable. Then do the following: *CORE::GLOBAL::die = sub { die @_ if $^S; my $e = $_[0]; That die line is the magic that all __DIE__ handlers need to start with, so that they don't break during inner eval{} exception handling. I added that line to your supplied script, and I get the expected behaviour. -- Paul Evans

Sun Aug 20 15:37:47 2006 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Thu Aug 17 16:11:26 2006, PEVANS wrote: Show quoted text

> That die line is the magic that all __DIE__ handlers need to start with, > so that they don't break during inner eval{} exception handling. I added > that line to your supplied script, and I get the expected behaviour.

Is this bug all sorted now? Can I close it, or are there still outstanding issues? -- Paul Evans

Mon Aug 21 10:02:26 2006 Marek.Rouchal [...] gmx.net - Correspondence added

From:

MAREKR [...] cpan.org

On Sun Aug 20 15:37:47 2006, PEVANS wrote: Show quoted text

> Is this bug all sorted now? Can I close it, or are there still > outstanding issues?

Yes - thank you very much for the pointer to $^S, that was unknown to me. I can confirm that with this trick things work as expected. Just one final word to this one: Show quoted text

>> But there is a major downside: I cannot get the stack trace! Only

when Show quoted text

>> I override the die (with CORE::GOBAL::die) I can use the caller() >> funtion in the override sub to determine the stack trace, and put

that Show quoted text

>> in the object, which is then thrown by CORE::die. That is the

ultimate Show quoted text

>> rationale for this ticket.

> >$Error::DEBUG = 1; >Now all Error-derived exceptions will store a stack trace.

Correct - but I do not only want stack traces for Error-derived exceptions, but for _all_ exceptions - and that is why I CORE::GLOBALly override die() with the Error->throw; and certainly, I set $Error::DEBUG=1 to get the trace. Anyway - there is no bug any more now; perhaps you can consider to give the user a possibility to do the override of die() using CORE::GLOBAL::die to turn all die()s into Error->throws, e.g. with use Error qw(:die); but what exception object to use? Probably Error::Simple. Maybe it would be better to say: use Error qw(:try); Error->die_as('Error::Simple'); with sub die_as { my ($self,$class) = @_; *CORE::GLOBAL::die = sub { CORE::die @_ if $^S; $class->throw($_[0]); }; 1; } Did not test it yet, though.

Thu Aug 24 03:37:35 2006 Marek.Rouchal [...] gmx.net - Correspondence added

RT-Send-CC:

lds [...] cpan.org

I have played with this subject a little more - and I am afraid there is no clean solution. One general suggestion, that hopefully won't hurt anybody: The "die" in Error::throw should be written as CORE::die, since at this point we do not want any more interaction with other exception handling systems etc. Then I saw that CGI::Carp tries to do similar things as I do, and I read this: 1.26 Replaced CORE::GLOBAL::die with the evil $SIG{__DIE__} because the former isn't working in some people's hands. There is no such thing as reliable exception handling in Perl. Looking at my test script again, one thing puzzles me: CORE::GLOBAL::die cannot override the die in Carp::croak! Maybe this is what Lincoln meant with the above... -Marek

Thu Aug 24 17:23:30 2006 LDS [...] cpan.org - Correspondence added

From:

LDS [...] cpan.org

I found the handling of $SIG{__DIE__} and $CORE::GLOBAL::die to be inconsistent among Perl versions and sometimes inconsistent with what I thought the documentation was saying (not that I am sure that this wasn't my misunderstanding). In any case, I got tired of getting CGI::Carp to work properly in all cases, and there are still situations when it doesn't do the right thing when responding to an eval { die } I would welcome any help that people can offer in modifying CGI::Carp so that it works properly in all cases. Lincoln

Thu Dec 14 05:47:16 2006 Marek.Rouchal [...] gmx.net - Correspondence added

On So. 20. Aug. 2006, 15:37:47, PEVANS wrote: Show quoted text

> Is this bug all sorted now? Can I close it, or are there still > outstanding issues?

Finally here is some news on this topic: $^S unfortunately does not help at all - I found that during a more thorough analysis. Since the try{} block in fact is an eval{}, $^S is always true inside that block, and my original requirements are not fulfilled at all. To remember, here they are again: - I want to use Error exception handling throughout the application - I want to force all modules throwing Error exceptions - I want to have the possibility to debug stack traces (for that I need the above). Obviously I cannot force everybody to use Error exceptions in all their CPAN modules. So the only way is to play with $SIG{__DIE__} or CORE::GLOBAL::die. The former is deprecated in the manuals, the latter seems to work OK. So I can do something like this: BEGIN { *CORE::GLOBAL::die = sub { Error::Simple->throw(shift) }; } and all should be fine (note that this requires Error::throw to use explicitely CORE::die, otherwise we end up in an endless loop). But alas, there is a gotcha... when in some module there is an eval{} around some code that does a die(), then $Error::THROWN is set - and not cleared. And that masks any CORE::die exception (which may occur e.g. when an XS module fails to load its binary *.so part - such an exception does not go through CORE::GLOBAL::die). Try this script: #!/opt/perl_5.8.8/bin/perl -w use Error qw(:try); BEGIN { *CORE::GLOBAL::die = sub { Error::Simple->throw(shift) }; *Error::die = sub { CORE::die @_ }; } try { # this contains an eval { ... } around some code which # throws an Error::Simple exception because of the # above CORE::GLOBAL::die override. $Error::THROWN is # set require File::Temp; # this is an alternative way to produce this situation: #eval { # Error::Simple->throw("This is caught by a simple eval"); #}; print "Error::THROWN = $Error::THROWN\n"; # this exception is caught nicely: #Error::Simple->throw("thrown exception"); # also this is caught with the right text: # since it triggers the CORE::GLOBAL::die #die "ARGH"; # this one is masked by the $Error::THROWN CORE::die "Do you see me?"; } otherwise { my $E = shift; print "Caught exception:\n$E\n"; }; exit 0; __END__ Uncomment and try the other exceptions - however, you won't see the "Do you see me?" exception. I looked at the code around line 358 in Error.pm: else { $err = defined($Error::THROWN) ? $Error::THROWN : $@; $err = $Error::ObjectifyCallback->({'text' =>$err}) unless ref($err); } I am wondering, why $Error::THROWN is preferred over $@, which actually contains the exception that caused the eval{} to abort. Why not rewrite it like this (attention, also on lines 393 and 433 etc.): else { $err = $Error::THROWN = defined($@) ? $@ : $Error::THROWN; $err = $Error::ObjectifyCallback->({'text' =>$err}) unless ref($err); } All the regression tests in the Error package pass OK with this change, and my requirements would be fulfilled as well. Thank you for your patience and best regards, Marek

Fri Mar 16 11:40:56 2007 Marek.Rouchal [...] gmx.net - Correspondence added

RT-Send-CC:

gbarr [...] pobox.com,peter [...] weblogic.com,jglick [...] sig.bsh.com,leonerd [...] leonerd.org.uk,shlomif [...] iglu.org.il,u_arunkumar [...] yahoo.com

Dear all, did you notice my previous posting, especially the suggestion at the end? I think this would be a valid and safe change, without losing any functionality of this module. Can you consider updating Error accordingly? Cheers, Marek On Do. 14. Dez. 2006, 05:47:16, MAREKR wrote: ... Show quoted text

> I am wondering, why $Error::THROWN is preferred over $@, which > actually contains the exception that caused the eval{} to abort. Why > not rewrite it like this (attention, also on lines 393 and 433 etc.): > > else { > $err = $Error::THROWN = defined($@) ? $@ : $Error::THROWN; > $err = $Error::ObjectifyCallback->({'text' =>$err}) > unless ref($err); > } > > All the regression tests in the Error package pass OK with this > change, and my requirements would be fulfilled as well.

Tue Aug 28 07:42:14 2007 Marek.Rouchal [...] gmx.net - Correspondence added

RT-Send-CC:

gbarr [...] pobox.com,peter [...] weblogic.com,jglick [...] sig.bsh.com,leonerd [...] leonerd.org.uk,shlomif [...] iglu.org.il,u_arunkumar [...] yahoo.com

Dear all, after some more investigations I have now prepared a patch, which solves my problem (to 80% - the problem that Carp::croak() still won't produce Error::Simple exceptions when I override CORE::GLOBAL::die is still there). The patch consists of the actual code change and a new test (12wrong.t) that will fail with version 0.17008 of Error and pass with the patched code. From that test you can easily see what the problem is. The test reproduces (with simple means) a much more complex situation, where the first exception occurs while processing a "use File::Temp", and the second exception occurs elsewhere deep inside some other CPAN module. Anyway, all other tests pass (together the tests have a 75% statement coverage) with the patch, and I do not see right now why one shouldn't examine $@ instead of $Error::THROWN after the eval{} in try() - since $@ is the ultimate source of whether an exception occurred or not. Or do I miss anything? Cheers, Marek

diff -ruN Error-0.17008/lib/Error.pm Error-0.17008p1/lib/Error.pm --- Error-0.17008/lib/Error.pm 2006-10-25 22:10:14.000000000 +0200 +++ Error-0.17008p1/lib/Error.pm 2007-08-27 17:12:18.459970000 +0200 @@ -15,7 +15,7 @@ use vars qw($VERSION); use 5.004; -$VERSION = "0.17008"; +$VERSION = "0.17008_01"; use overload ( '""' => 'stringify', @@ -356,8 +356,7 @@ undef $err; } else { - $err = defined($Error::THROWN) - ? $Error::THROWN : $@; + $err = $@ || $Error::THROWN; $err = $Error::ObjectifyCallback->({'text' =>$err}) unless ref($err); } @@ -391,8 +390,7 @@ undef $err; } else { - $err = defined($Error::THROWN) - ? $Error::THROWN : $@; + $err = $@ || $Error::THROWN; $err = $Error::ObjectifyCallback->({'text' =>$err}) unless ref($err); @@ -430,7 +428,7 @@ 1; }; - $err = defined($Error::THROWN) ? $Error::THROWN : $@ + $err = $@ || $Error::THROWN unless $ok; }; diff -ruN Error-0.17008/MANIFEST Error-0.17008p1/MANIFEST --- Error-0.17008/MANIFEST 2006-10-25 22:10:14.000000000 +0200 +++ Error-0.17008p1/MANIFEST 2007-08-28 08:19:25.582681000 +0200 @@ -25,6 +25,7 @@ t/09dollar-at.t t/10throw-in-catch.t t/11rethrow.t +t/12wrong.t t/lib/MyDie.pm t/pod-coverage.t t/pod.t diff -ruN Error-0.17008/t/12wrong.t Error-0.17008p1/t/12wrong.t --- Error-0.17008/t/12wrong.t 1970-01-01 01:00:00.000000000 +0100 +++ Error-0.17008p1/t/12wrong.t 2007-08-28 08:18:41.624772000 +0200 @@ -0,0 +1,38 @@ +#!/usr/bin/perl -w + +use Error qw(:try); + +print "1..2\n"; + +try { + eval { + throw Error::Simple "This is caught by eval, not by try."; + }; + + if($@ && $@ =~ /This is caught by eval, not by try/) { + print "ok 1\n"; + } else { + print "not ok 1\n"; + } + + print "# Error::THROWN = $Error::THROWN\n"; + + die "This is a simple 'die' exception."; + + # not reached +} +otherwise { + my $E = shift; + my $t = $Error::THROWN ? "$Error::THROWN" : ''; + print "# Error::THROWN = $t\n"; + $E ||= ''; + print "# E = $E\n"; + if("$E" =~ /This is a simple 'die' exception/) { + print "ok 2\n"; + } else { + print "not ok 2\n"; + } +}; + +exit 0; +

Tue Aug 28 12:38:55 2007 SHLOMIF [...] cpan.org - Correspondence added 15 min

On Tue Aug 28 07:42:14 2007, MAREKR wrote: Show quoted text

> Dear all, > > after some more investigations I have now prepared a patch, which > solves my problem (to 80% - the problem that Carp::croak() still

won't Show quoted text

> produce Error::Simple exceptions when I override CORE::GLOBAL::die

is Show quoted text

> still there). The patch consists of the actual code change and a new > test (12wrong.t) that will fail with version 0.17008 of Error and

pass Show quoted text

> with the patched code. From that test you can easily see what the > problem is. The test reproduces (with simple means) a much more > complex situation, where the first exception occurs while processing > a "use File::Temp", and the second exception occurs elsewhere deep > inside some other CPAN module. > > Anyway, all other tests pass (together the tests have a 75%

statement Show quoted text

> coverage) with the patch, and I do not see right now why one

shouldn't Show quoted text

> examine $@ instead of $Error::THROWN after the eval{} in try() -

since Show quoted text

> $@ is the ultimate source of whether an exception occurred or not.

Or Show quoted text

> do I miss anything? > > Cheers, > > Marek

Hi Marek! Thanks for your persistence. I applied a modified version of your patch to Error-0.17009, which I just uploaded to CPAN. After I inspected the tests I understood that the behaviour at the moment was wrong. Regards, Shlomi Fish

Tue Aug 28 12:38:59 2007 SHLOMIF [...] cpan.org - Status changed from 'open' to 'resolved'

Tue Aug 28 12:39:03 2007 SHLOMIF [...] cpan.org - Given to SHLOMIF