DESTROY can hide $@
Brandon Black
I ran into a little oddity, than some googling says other people
have run into before. It was extremely painful to debug this and
figure it out. Basically, if a DESTROY sub contains an eval statement
(or otherwise modifies $@), it can screw up error handling, as in:
---------------------------
{
package Foo;
sub new { bless {} => shift }
sub DESTROY { eval "123" }
}
eval {
my $x = Foo->new;
die "preserve_me";
warn "unreached";
};
print "exception: $@\n";
--------------------------
In the above example, the unreached line is truly unreached, as the
eval block does exit prematurely via the preserve_me death statement.
However, when $x is destructed while leaving the scope, the successful
eval in Foo::DESTROY wipes out the existing $@, and thus $@ appears
empty (no error) when we try to look at the exception on the final
line.
Adding a "local $@" to Foo::DESTROY fixes the issue, but it's a real
PITA to debug otherwise, and there will undoubtedly be CPAN modules
with DESTROY subs that make this same mistake for you, no matter how
careful you are in your own code.
So I've made a very brief patch against perl-current that seems to fix
this. In the code that handles stack unwinding for exceptions
occuring within evals, it preserves a copy of ERRSV contents before
unwinding the stack (which triggers the DESTROYs), and restores it
before actually returning from the eval. I added a test to t/op/die.t
that's a lot like the example above too.
Given how few lines are in this patch, I suspect I am being naive and
this doesn't actually work for some cases, but I figured I'd throw
this out there at least and let people that understand this stuff
better stab at it and make it better or something :)
-- Brandon
Rafael Garcia-Suarez
> So I've made a very brief patch against perl-current that seems to fix
> this. In the code that handles stack unwinding for exceptions
> occuring within evals, it preserves a copy of ERRSV contents before
> unwinding the stack (which triggers the DESTROYs), and restores it
> before actually returning from the eval. I added a test to t/op/die.t
> that's a lot like the example above too.
You're making a mortal copy of ERRSV, but that's perhaps too
short-lived, esp. if unwinding the call stack involves traversing
subroutine calls.
Jerry D. Hedden
> Basically, if a DESTROY sub contains an eval statement
> (or otherwise modifies $@), it can screw up error handling
>
> PITA to debug otherwise, and there will undoubtedly be CPAN modules
> with DESTROY subs that make this same mistake for you, no matter how
> careful you are in your own code.
This behavior is documented in the "Destructors" section of "perlobj".
In Object::InsideOut, I take this into account, preserving $@ and
adding any additional errors generated in DESTROY. It may be that
other modules also handle this behavior such that changing it would
break them. Therefore, I recommend against changing this behavior.
Brandon Black
I can't imagine a sane DESTROY behavior that this sort of change would
fundamentally break (the patch as it stands is obviously flawed, but
if it were done right ...), but perhaps I'm not as imaginative as
CPAN. It doesn't affect DESTROY in normal scope exits, it only
affects whether any $@ generated by DESTROY is preserved during a
stack unwind due to an earlier exception being thrown. In the case of
Object::InsideOut, you would lose the extra info you were tacking on,
but is that a big issue compared to resolving this insanity?
Exceptions are what we generally use to find coding problems. If I
happen to use JoesMagicalModule from CPAN in my complex project, and I
throw an error which causes one of his objects to destruct during
unwind, the last headache I need is his DESTROY method wiping out my
$@ and making me scratch my head and think all of Perl is broken, and
spend half an hour in the debugger trying to figure out how and where
my $@ got wiped out. Now that I've done it once I'm sure I'll figure
it out quicker the next time, but it's certainly not obvious,
intuitive, expected, or well-understood from the average Perl
programmer's POV, and I pity any other Perl programmer that runs into
it for the first time.
-- Brandon
Jerry D. Hedden
> I can't imagine a sane DESTROY behavior that this sort of change would
> fundamentally break (the patch as it stands is obviously flawed, but
> if it were done right ...), but perhaps I'm not as imaginative as
> CPAN.
I don't disagree that this behavior may be problematic and hard to debug. My
point is that the current behavior is known and documented, and that
changing it most likely will break things.
Joshua ben Jore
Dunno, I've considered the current behaviour broken as well. 5.10
would be the kind of major increment that could accept such a fix.
Then our code becomes sane. Huzzah. I was unaware of this for awhile
until C++ programmers came along to school me. They told me "don't you
know you should never throw exceptions inside destructors? It's
obvious and well known!" I thought it was strange I was supposed to
learn this from C++ because I've only ever done a little bit of that
but hey, people's prejudices are strange.
Brandon,
here's some other related cases:
# Fails to die while reaping $x because there's an implicit eval
during scope cleanup
{ my $x = X->new; };
sub X::new { bless [], 'X' }
sub X::DESTROY { die }
# $@ is empty but shouldn't be. You fixed this, apparently.
eval { my $x = X->new; die }; die if $@;
sub X::new { bless [], 'X' }
sub X::DESTROY { eval {} }
Josh
Nicholas Clark
Which would mean that a real copy is needed, into a new SV.
The problem, and I'm really not familiar with how stack unwinding works, is
getting rid of that reference at the right time.
(Either the reference currently in ERRSV, or the reference one is holding)
And I'm not sure where the C execution flies off to if some code throws an
exception (ie longjmp()) whilst in the unwinding loop, and hence where to
clean up that extra reference.
Nicholas Clark
ma...@mark.mielke.cc
> I ran into a little oddity, than some googling says other people
> have run into before. It was extremely painful to debug this and
> figure it out. Basically, if a DESTROY sub contains an eval statement
> (or otherwise modifies $@), it can screw up error handling, as in:
I've hit this problem a few times over the years. The answer I have
stuck with is to use local($@); within DESTROY.
Note that any subroutine or method that DESTROY calls may itself call
eval. In our product, we use eval heavily. Therefore, it becomes a good
practice to always local($@); within DESTROY.
There are similar issues with other variables, such as $!, with similar
solutions. Using global variables for lexical state is yucky - but that's
how perl4 worked.
Cheers,
mark
--
ma...@mielke.cc / ma...@ncf.ca / ma...@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada
One ring to rule them all, one ring to find them, one ring to bring them all
and in the darkness bind them...
Nicholas Clark
> On Tue, Jun 12, 2007 at 08:32:08AM -0500, Brandon Black wrote:
> > I ran into a little oddity, than some googling says other people
> > have run into before. It was extremely painful to debug this and
> > figure it out. Basically, if a DESTROY sub contains an eval statement
> > (or otherwise modifies $@), it can screw up error handling, as in:
>
> I've hit this problem a few times over the years. The answer I have
> stuck with is to use local($@); within DESTROY.
>
> Note that any subroutine or method that DESTROY calls may itself call
> eval. In our product, we use eval heavily. Therefore, it becomes a good
> practice to always local($@); within DESTROY.
>
> There are similar issues with other variables, such as $!, with similar
> solutions. Using global variables for lexical state is yucky - but that's
> how perl4 worked.
I agree with the general statement, but I'm in two minds on the specific
example, given that $! is documented as being the current value of C's errno,
with all the insanity that entails. Much like "C combines the power of
assembler with the portability of assembler."
Nicholas Clark
ma...@mark.mielke.cc
> > There are similar issues with other variables, such as $!, with similar
> > solutions. Using global variables for lexical state is yucky - but that's
> > how perl4 worked.
> I agree with the general statement, but I'm in two minds on the specific
> example, given that $! is documented as being the current value of C's errno,
> with all the insanity that entails. Much like "C combines the power of
> assembler with the portability of assembler."
Yes. Perl inherited much traditional C madness... :-)
I agree with you agreeing with the general statement. The case that I
had a problem was:
if (!(... some system call ...)) {
require POSIX;
if ($! == POSIX::EINTR()) {
... not what you think! ...
}
}
Why would I load POSIX on demand? Well, we have a bunch of scripts that
are potentially launched many times a second, and loading POSIX for no
other reason that we *might* need to check an error code, has a noticeable
performance hit. The above code shows the first naive attempt that we made.
It didn't work. If require actually loads the module, as intended, than
errno is overwritten. Ouch. This leads to:
if (!(... some system call ...)) {
my $error_saved = $!;
require POSIX;
if ($error_saved == POSIX::EINTR()) {
... phew! much better! ...
}
}
Perl (and as you point out, C) can be lovely... :-)
Rafael Garcia-Suarez
> I agree with you agreeing with the general statement. The case that I
> had a problem was:
>
> if (!(... some system call ...)) {
> require POSIX;
> if ($! == POSIX::EINTR()) {
> ... not what you think! ...
> }
> }
>
> Why would I load POSIX on demand? Well, we have a bunch of scripts that
> are potentially launched many times a second, and loading POSIX for no
> other reason that we *might* need to check an error code, has a noticeable
> performance hit. The above code shows the first naive attempt that we made.
Using Errno might be faster.
> It didn't work. If require actually loads the module, as intended, than
> errno is overwritten. Ouch. This leads to:
>
> if (!(... some system call ...)) {
> my $error_saved = $!;
> require POSIX;
> if ($error_saved == POSIX::EINTR()) {
> ... phew! much better! ...
> }
> }
>
> Perl (and as you point out, C) can be lovely... :-)
That's uncool, but since perl searches for POSIX.pm in @INC, I don't
see how to avoid it -- except by saving errno in pp_require...
Brandon Black
I don't think longjmp is an issue, as during unwind the code is run in
an implicit eval, so control should return to the unwind function
always (die_where). I'm not 100% sure on this. Given that
assumption, it's pretty easy to modify the patch to make an alias with
a proper refcnt and handle the decref at the exit point(s).
The more important question to me is whether this is worth pursuing at
all, or whether even if it didn't leak, this idea is dead from the
start because nobody wants to break the existing behavior.
-- Brandon
Assuming we could be sure we're not leaking