Skip Menu |
Report information
Id: 124387
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: gabor [at] szabgab.com
pushtaev.vm [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)



Subject: AUTOLOAD is not called when DESTROY is missing in 5.20.1 - but this is not documented
To: perlbug [...] perl.org
Date: Sat, 25 Apr 2015 08:16:00 +0200
From: Gabor Szabo <gabor [...] szabgab.com>
Download (untitled) / with headers
text/plain 768b
In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY is not defined, the AUTOLOAD would be executed when the object goes out of scope and one would have had to add an empty DESTROY to eliminate the problem caused by this.

In 5.20.1 this does not seem to be the case. In 5.20.1 it seems if AUTOLOAD won't be called when the object goes out of scope.

However I think the documentation in perlobj still indicates the old behavior
and I could not find any indication in the perldelta docs that would imply this change.

So I wonder if this was an unplanned change in perl,
a lack of documentation, or something else.

The issue was discussed here as well:

http://www.perlmonks.org/?node_id=1124504
To: perl5-porters [...] perl.org
Date: Sat, 25 Apr 2015 11:25:34 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #124387] AUTOLOAD is not called when DESTROY is missing in 5.20.1 - but this is not documented
Download (untitled) / with headers
text/plain 2.5k
On Fri, Apr 24, 2015 at 11:16:23PM -0700, Gabor Szabo wrote: Show quoted text
> # New Ticket Created by Gabor Szabo > # Please include the string: [perl #124387] > # in the subject line of all future correspondence about this issue. > # <URL: https://rt.perl.org/Ticket/Display.html?id=124387 > > > > In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY > is not defined, the AUTOLOAD would be executed when the object goes out of > scope and one would have had to add an empty DESTROY to eliminate the > problem caused by this. > > In 5.20.1 this does not seem to be the case. In 5.20.1 it seems if AUTOLOAD > won't be called when the object goes out of scope. > > However I think the documentation in perlobj still indicates the old > behavior > http://perldoc.perl.org/perlobj.html#Destructors > and I could not find any indication in the perldelta docs that would imply > this change. > > So I wonder if this was an unplanned change in perl, > a lack of documentation, or something else. > > The issue was discussed here as well: > > http://www.perlmonks.org/?node_id=1124504
The change bisects to this, which leads to the suspicion that it was unintended. commit 8c34e50dccefe2a0539ba2339a2889bb841986c2 Author: Father Chrysostomos <sprout@cpan.org> AuthorDate: Fri Nov 16 10:00:50 2012 -0800 Commit: Father Chrysostomos <sprout@cpan.org> CommitDate: Sat Nov 17 10:13:43 2012 -0800 [perl #114864] Don’t use amt for DESTROY DESTROY has been cached in overload tables since perl-5.6.0-2080-g32251b2, making it 4 times faster than before (over- load tables are faster than method lookup). But it slows down symbol lookup on stashes with overload tables, because overload tables use magic, and SvRMAGICAL results in calls to mg_find on every hash lookup. By reusing SvSTASH(stash) to cache the DESTROY method (if the stash is unblessed, of course, as most stashes are), we can avoid making all destroyable stashes magical and also speed up DESTROY lookup slightly more. The results: • 10% increase in stash lookup speed after destructors. That was just testing $Foo::{x}. Other stash lookups will have other overheads that make the difference less impressive. • 5% increase in DESTROY lookup speed. I was using an empty DESTROY method to test this, so, again, real DESTROY methods will have more overhead and less speedup. -- "There's something wrong with our bloody ships today, Chatfield." -- Admiral Beatty at the Battle of Jutland, 31st May 1916.
Subject: Re: [perl #124387] AUTOLOAD is not called when DESTROY is missing in 5.20.1 - but this is not documented
Date: Tue, 26 May 2015 08:01:26 -0400
From: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
* Dave Mitchell <davem@iabyn.com> [2015-04-25T06:25:34] Show quoted text
> On Fri, Apr 24, 2015 at 11:16:23PM -0700, Gabor Szabo wrote:
> > In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY > > is not defined, the AUTOLOAD would be executed when the object goes out of > > scope and one would have had to add an empty DESTROY to eliminate the > > problem caused by this. > > [...]
> The change bisects to this, which leads to the suspicion that it was > unintended. > > commit 8c34e50dccefe2a0539ba2339a2889bb841986c2
That does look unintended. I don't see discussion of the change on p5p or changes to tests, either. I know I've added special casing to AUTOLOADs to skip DESTROY, which makes me feel certain that people have used AUTOLOAD to handle DESTROY. Sure, it's been broken for three years now, but unless there is a compelling reason against it, I think we should restore this behavior and add a regression test. The downside is breaking code only ever written or tested on 5.18.0, or later. In other words, we'd be fixing a regression introduced before the earliest supported version of perl. Still, I think it should be fixed. -- rjbs
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

Subject: Re: [perl #124387] AUTOLOAD is not called when DESTROY is missing in 5.20.1 - but this is not documented
To: perl5 porters <perl5-porters [...] perl.org>
CC: "bugs-bitbucket [...] rt.perl.org" <bugs-bitbucket [...] rt.perl.org>
From: Eric Brine <ikegami [...] adaelis.com>
Date: Tue, 26 May 2015 17:04:28 -0400
Download (untitled) / with headers
text/plain 787b
On Sat, Apr 25, 2015 at 2:16 AM, Gabor Szabo <perlbug-followup@perl.org> wrote:
Show quoted text
# New Ticket Created by  Gabor Szabo
# Please include the string:  [perl #124387]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt.perl.org/Ticket/Display.html?id=124387 >


In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY
is not defined, the AUTOLOAD would be executed when the object goes out of
scope and one would have had to add an empty DESTROY to eliminate the
problem caused by this.
Show quoted text

In 5.20.1 this does not seem to be the case. In 5.20.1 it seems if AUTOLOAD
won't be called when the object goes out of scope.

Ow! The bug isn't the documentation; it's that AUTOLOAD isn't being called when it should be!

Subject: AUTOLOAD is not called instead of DESTROY
Download (untitled) / with headers
text/plain 928b
The perldoc says: If you define an AUTOLOAD in your class, then Perl will call your AUTOLOAD to handle the DESTROY method. You can prevent this by defining an empty DESTROY, like we did in the autoloading example. You can also check the value of $AUTOLOAD and return without doing anything when called to handle DESTROY . However, my AUTOLOAD is not called in some versions of Perl (including the current stable 5.22.0). Here is what I tried: # 5.8.8 perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"' D # 5.14.2 perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"' D # 5.18.2 perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"' # nothing # 5.20.2 perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"' # nothing # 5.22.0 perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"' # nothing I'm not sure whether it's a documentation issue or it should not behave this way at all.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 576b
On Tue Oct 20 02:25:43 2015, pushtaev.vm@gmail.com wrote: Show quoted text
> The perldoc says: > > If you define an AUTOLOAD in your class, then Perl will call your > AUTOLOAD to handle the DESTROY method. You can prevent this by > defining an empty DESTROY, like we did in the autoloading example. You > can also check the value of $AUTOLOAD and return without doing > anything when called to handle DESTROY . > > However, my AUTOLOAD is not called in some versions of Perl (including > the current stable 5.22.0).
Looks like this changed in 8c34e50dccefe2a0539ba2339a2889bb841986c2. Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 937b
Hello, thanks for you reply! I googled that commmit identifier you provided and found this: https://rt.perl.org/Ticket/Display.html?id=124387 Looks like it has already been reported and isn't fixed yet. Is there a way I mark this ticket as duplicate or something? -- Vadim Pushtaev On Tue Oct 20 18:42:26 2015, tonyc wrote: Show quoted text
> On Tue Oct 20 02:25:43 2015, pushtaev.vm@gmail.com wrote:
> > The perldoc says: > > > > If you define an AUTOLOAD in your class, then Perl will call your > > AUTOLOAD to handle the DESTROY method. You can prevent this by > > defining an empty DESTROY, like we did in the autoloading example. You > > can also check the value of $AUTOLOAD and return without doing > > anything when called to handle DESTROY . > > > > However, my AUTOLOAD is not called in some versions of Perl (including > > the current stable 5.22.0).
> > Looks like this changed in 8c34e50dccefe2a0539ba2339a2889bb841986c2. > > Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 380b
On Thu Oct 22 01:56:47 2015, pushtaev.vm@gmail.com wrote: Show quoted text
> Hello, thanks for you reply! > > I googled that commmit identifier you provided and found this: > https://rt.perl.org/Ticket/Display.html?id=124387 > > Looks like it has already been reported and isn't fixed yet. > > Is there a way I mark this ticket as duplicate or something?
Thanks, I've merged the tickets. Tony
Subject: Re: [perl #126407] AUTOLOAD is not called instead of DESTROY
Date: Thu, 22 Oct 2015 12:41:43 -0500
From: Todd Rinaldo <toddr [...] cpanel.net>
To: perlbug-followup [...] perl.org
CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.8k

Show quoted text
On Oct 20, 2015, at 8:42 PM, Tony Cook via RT <perlbug-followup@perl.org> wrote:

On Tue Oct 20 02:25:43 2015, pushtaev.vm@gmail.com wrote:
The perldoc says:

If you define an AUTOLOAD in your class, then Perl will call your
AUTOLOAD to handle the DESTROY method. You can prevent this by
defining an empty DESTROY, like we did in the autoloading example. You
can also check the value of $AUTOLOAD and return without doing
anything when called to handle DESTROY .

However, my AUTOLOAD is not called in some versions of Perl (including
the current stable 5.22.0).

Looks like this changed in 8c34e50dccefe2a0539ba2339a2889bb841986c2.


That makes this sound related to https://rt.perl.org/Ticket/Display.html?id=126410. Which is the third time (overload changes also broke B::C in bee7c5743f) this week I've been fighting with these sets of changes introduced in 5.17. 

As I look at the commits, I'm forced to ask why we would want these speedup improvements when the commit messages say:

10% increase in stash lookup ... Other stash lookups will have other overheads that make the difference less impressive.

So basically it's a 10% speedup on trivial cases. Which were probably billisecond improvements to begin with?

I promise I'm not trying to trivialize the effort that was put in. It seems like we changed some fundamental things about how Perl checks and uses some of the magic. However in doing so made the code potentially more complex and introduced several bugs we're now starting to notice.

It's possible I'm misunderstanding these changes and they're not as related as they seem. However if they are, I'm wondering if it's worth reverting the improvements to fix these problems? I admit that at this point it may be more complex than some other approach.

Todd.

Download smime.p7s
application/pkcs7-signature 3.4k

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 619b
On Fri Apr 24 23:16:23 2015, gabor@szabgab.com wrote: Show quoted text
> In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY > is not defined, the AUTOLOAD would be executed when the object goes out of > scope and one would have had to add an empty DESTROY to eliminate the > problem caused by this.
The attached, along with the two patches on #126410, fixes this for me. I needed to change one of the existing tests in op/method.t to allow for DESTROY being called on the stdio objects. I don't think these patches can be applied as-is to maint, though a different patch could fix this problem in maint. Tony
Subject: 0003-perl-124387-TODO-test-for-AUTOLOAD-on-DESTROY.patch
From 8e91b8fcc8f1c911ccd80dc5bef1639a8a51d2f4 Mon Sep 17 00:00:00 2001 From: Tony Cook <tony@develop-help.com> Date: Tue, 19 Jan 2016 11:39:48 +1100 Subject: [perl #124387] TODO test for AUTOLOAD on DESTROY --- t/op/method.t | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/t/op/method.t b/t/op/method.t index 0d7f254..bdac5bb 100644 --- a/t/op/method.t +++ b/t/op/method.t @@ -13,7 +13,7 @@ BEGIN { use strict; no warnings 'once'; -plan(tests => 148); +plan(tests => 149); @A::ISA = 'B'; @B::ISA = 'C'; @@ -460,6 +460,17 @@ is $kalled, 1, 'calling a class method via a magic variable'; { bless {}, "NoSub"; } } +{ + # [perl #124387] + local $::TODO = "AUTOLOAD not being called for DESTROY"; + my $autoloaded; + package AutoloadDestroy; + sub AUTOLOAD { $autoloaded = 1 } + package main; + bless {}, "AutoloadDestroy"; + ok($autoloaded, "AUTOLOAD called for DESTROY"); +} + eval { () = 3; new {} }; like $@, qr/^Can't call method "new" without a package or object reference/, -- 2.1.4
Subject: 0004-perl-124387-call-AUTOLOAD-when-DESTROY-isn-t-defined.patch
From 4e3b4ec09e4002c0b74eca141e662b1403d65aaa Mon Sep 17 00:00:00 2001 From: Tony Cook <tony@develop-help.com> Date: Tue, 19 Jan 2016 11:42:21 +1100 Subject: [perl #124387] call AUTOLOAD when DESTROY isn't defined --- sv.c | 3 ++- t/op/method.t | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/sv.c b/sv.c index 0a5ffe8..9c4fed6 100644 --- a/sv.c +++ b/sv.c @@ -6738,6 +6738,7 @@ S_curse(pTHX_ SV * const sv, const bool check_refcnt) { if (HvNAME(stash)) { CV* destructor = NULL; struct mro_meta *meta; + assert (SvOOK(stash)); DEBUG_o( deb(aTHX_ "Looking for DESTROY method for %s\n", @@ -6753,7 +6754,7 @@ S_curse(pTHX_ SV * const sv, const bool check_refcnt) { } else { GV * const gv = - gv_fetchmeth_autoload(stash, "DESTROY", 7, 0); + gv_fetchmethod_pvn_flags(stash, "DESTROY", 7, GV_AUTOLOAD); if (gv) destructor = GvCV(gv); HvMROMETA(stash)->destroy_gen = PL_sub_generation; HvMROMETA(stash)->destroy = destructor; diff --git a/t/op/method.t b/t/op/method.t index bdac5bb..d11a703 100644 --- a/t/op/method.t +++ b/t/op/method.t @@ -361,6 +361,7 @@ for my $meth (['Bar', 'Foo::Bar'], { fresh_perl_is(<<EOT, package UNIVERSAL; sub AUTOLOAD { my \$c = shift; print "\$c \$AUTOLOAD\\n" } +sub DESTROY {} # prevent AUTOLOAD being called on DESTROY package Xyz; package main; Foo->$meth->[0](); EOT @@ -462,7 +463,6 @@ is $kalled, 1, 'calling a class method via a magic variable'; { # [perl #124387] - local $::TODO = "AUTOLOAD not being called for DESTROY"; my $autoloaded; package AutoloadDestroy; sub AUTOLOAD { $autoloaded = 1 } -- 2.1.4
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 672b
On Mon Jan 18 16:53:50 2016, tonyc wrote: Show quoted text
> On Fri Apr 24 23:16:23 2015, gabor@szabgab.com wrote:
> > In earlier versions of perl if AUTOLOAD is defined in a module but > > DESTROY > > is not defined, the AUTOLOAD would be executed when the object goes > > out of > > scope and one would have had to add an empty DESTROY to eliminate the > > problem caused by this.
> > The attached, along with the two patches on #126410, fixes this for > me. > > I needed to change one of the existing tests in op/method.t to allow > for DESTROY being called on the stdio objects.
Applied as f05081b8ef229712c83a6f19b217489cf5e5ab25 and 000814da477053c7627c7ace5ca3ce3d4c4aad08. Tony
Download (untitled) / with headers
text/plain 252b
Thank you for submitting this report. You have helped make Perl better. With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org