RE: :Compress::Base destructor

From: Zefram [mailto:zeframfysh.org]
 
> The destructors in IO::Compress::Base and
IO::Uncompress::Base can clobber
> $! and other global status variables.  (Via subclassing
of the "close"
> method they can clobber any of them.)  Destructors
should avoid this,
> because they're called implicitly.  Attached patch
fixes.

Thanks for the patch.

I see you've posted the same patch for a number of modules,
so I'm CC'ing
perl5-porters on this to discuss in a wider forum.

P5P -- the proposed change is to localise the global error
variables like
this in the DESTROY method


sub DESTROY
{
    my $self = shift ;
    local($., $, $!, $^E, $?); # <= Add This
    $self->close() ;
}


I'm in two minds about this change. Any opinions?

Paul

On 11/09/2007, Paul Marquess <paul_marquessyahoo.co.uk> wrote:
> P5P -- the proposed change is to localise the global
error variables like
> this in the DESTROY method
>
>
> sub DESTROY
> {
>     my $self = shift ;
>     local($., $, $!, $^E, $?); # <= Add This
>     $self->close() ;
> }
>
>
> I'm in two minds about this change. Any opinions?

I was thinking the same thing. My conclusion was, more or
less, "if
you want to check the value of $! anyway, you can always
call close()
directly, so that patch seems safe". But Graham, who
maintains
IO:ir, has
not answered yet. (And that is not 5.10-critical)

Paul Marquess wrote:
>P5P -- the proposed change is to localise the global
error variables like
>this in the DESTROY method

A bit more background: the general issue is with global
status variables
and code that gets run implicitly/unexpectedly.  Over three
years ago
I reported a bug that was actually a special case of this
problem,
bug#28824 concerning signal handlers clobbering $!.  I
argued that
$! ought to be implicitly preserved across the signal
handler, and as
I predicted someone else argued that (as now) it should
not.

More recently I came on a related issue, where a cleanup
block established
using the End module <http://sea
rch.cpan.org/dist/End/End.pm> clobbered
$. 
The situation here was roughly:

	eval {
		init_stuff();
		my $cleanup = end { do_cleanup_stuff(); };
		something_that_might_throw_exception();
	};
	if($ ne "") { ... }

do_cleanup_stuff() internally used an eval {}.  When
something_that_might_throw_exception() threw an exception, I
meant
to catch it in the last line and report it appropriately. 
However,
do_cleanup_stuff()'s eval ended up setting $ to
"" (because the stuff
in its eval didn't die), so I was misled into thinking that
there had
been no error.  If the code in do_cleanup_stuff()'s eval had
died,
I would have seen that error instead of the right one.

So, basically, the implicitly-run code needs to take care to
preserve the
global status variables.  After examination of perlvar(1) I
think there
are exactly five such variables: $., $, $!, $^E, and $?.  I've
submitted
similar patches regarding destructors and signal handlers in
a bunch
of modules.  (Roughly, the modules on which my code
depends.)  It's a
general problem in writing Perl code.

An issue that has been mentioned: if cleanup or signal stuff
has an
error, or other status, to report then it needs an
appropriate way to
report it, but I think the usual globals are *not* an
appropriate way.
They belong to the main program flow.  It is already the
case that if
a destructor dies then the exception is emitted as a warning
and the
original stack unwind continues; this seems fairly
appropriate behaviour.
(Actually if the unwind is due to an exception then the
cleanup exception
also gets appended to $.)

If, say, IO:ir has
an error closing its file descriptor, a warning
message may be appropriate.  (Handily, $SIG can be
used to
trigger some other form of error reporting based on this
behaviour.
Hey, I wonder if $SIG could be used to emulate
Common Lisp's
continuable (non-unwinding) exceptions...)  Setting $! does
not seem an
appropriate way to report the error, because the main
program doesn't
realise that that's where the $! value has come from.  In
fact, it may
well not know that there's an IO:ir
object at all.  Also, of course,
$! often gets set in expected ways, where it's not at an
application level
erroneous to get an error response from a system call. 
That's analogous
to my cleanup code using eval {}, where it's not an error
for that inner
code to throw an exception.

-zefram