Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DESTROY not called on code reference objects #2028

Open
p5pRT opened this issue May 30, 2000 · 62 comments
Open

DESTROY not called on code reference objects #2028

p5pRT opened this issue May 30, 2000 · 62 comments

Comments

@p5pRT
Copy link

p5pRT commented May 30, 2000

Migrated from rt.perl.org#3306 (status was 'open')

Searchable as RT3306$

@p5pRT
Copy link
Author

p5pRT commented May 30, 2000

From abigail@arenanetworks.com

Created by abigail@arena-i.com

This might be related to my previous report.

Consider the following program​:

  #!/opt/perl/bin/perl -w

  use strict;

  sub new {
  my ($class, $code) = @​_;
  bless $code => $class;
  }

  DESTROY {
  print "In DESTROY...\n";
  }

  my $i = "Hello";
  my $exit = main -> new (sub {}); # 1)

  __END__

This will not print anything at all, indicating that the DESTROY method
isn't called when $exit goes out of scope - not even on program termination.

If we change 1) such that new() is called with a closure, for instance
by changing it to​:

  my $exit = main -> new (sub {$i}); # 1)

DESTROY is called as soon as $exit goes out of scope.

Abigail

Perl Info

Flags:
    category=core
    severity=high

This perlbug was built using Perl v5.6.0 - Thu Mar 23 19:51:19 EST 2000
It is being executed now by  Perl v5.6.0 - Fri Mar 24 17:24:48 EST 2000.

Site configuration information for perl v5.6.0:

Configured by abigail at Fri Mar 24 17:24:48 EST 2000.

Summary of my perl5 (revision 5.0 version 6 subversion 0) configuration:
  Platform:
    osname=solaris, osvers=2.7, archname=i86pc-solaris-64int
    uname='sunos newyork 5.7 generic_106542-07 i86pc i386 i86pc '
    config_args='-d -Dprefix=/opt/perl -Uinstallusrbinperl -Doptimize=-g -Duse64bitint'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef
    useperlio=undef d_sfio=undef uselargefiles=define 
    use64bitint=define use64bitall=undef uselongdouble=undef usesocks=undef
  Compiler:
    cc='cc', optimize='-g', gccversion=2.95.1 19990816 (release)
    cppflags='-DDEBUGGING -fno-strict-aliasing -I/usr/local/include'
    ccflags ='-DDEBUGGING -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    stdchar='char', d_stdstdio=define, usevfork=false
    intsize=4, longsize=4, ptrsize=4, doublesize=8
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, usemymalloc=y, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib '
    libpth=/usr/local/lib /lib /usr/lib /usr/ccs/lib
    libs=-lsocket -lnsl -lgdbm -ldb -ldl -lm -lc -lcrypt -lsec
    libc=/lib/libc.so, so=so, useshrplib=false, libperl=libperl.a
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags=' '
    cccdlflags='-fPIC', lddlflags='-G -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.6.0:
    /home/abigail/Perl
    /home/abigail/Sybase
    /opt/perl/lib/5.6.0/i86pc-solaris-64int
    /opt/perl/lib/5.6.0
    /opt/perl/lib/site_perl/5.6.0/i86pc-solaris-64int
    /opt/perl/lib/site_perl/5.6.0
    /opt/perl/lib/site_perl
    .


Environment for perl v5.6.0:
    HOME=/home/abigail
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH=/home/abigail/Lib:/usr/local/lib:/usr/lib:/lib:/usr/X11R6/lib
    LOGDIR (unset)
    PATH=/home/abigail/bin:/usr/local/bin:/usr/local/X11/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/games:/usr/ccs/bin:/usr/openwin/bin:/opt/Acrobat4/bin:/opt/cvs/bin:/opt/perl/bin
    PERL5LIB=/home/abigail/Perl:/home/abigail/Sybase
    PERLDIR=/opt/perl
    PERL_BADLANG (unset)
    SHELL=/usr/local/bin/bash


@p5pRT
Copy link
Author

p5pRT commented Oct 29, 2001

From @schwern

(From perlbug 20000530.008)

Consider the following program​:

\#\!/opt/perl/bin/perl \-w

use strict;

sub new \{
    my \($class\, $code\) = @​\_;
    bless $code => $class;
\}

DESTROY \{
    print "In DESTROY\.\.\.\\n";
\}

my $i    = "Hello";
my $exit = main \-> new \(sub \{\}\);   \# 1\)

\_\_END\_\_

This will not print anything at all, indicating that the DESTROY method
isn't called when $exit goes out of scope - not even on program termination.

If we change 1) such that new() is called with a closure, for instance
by changing it to​:

my $exit = main \-> new \(sub \{$i\}\);   \# 1\)

DESTROY is called as soon as $exit goes out of scope.

--

Michael G. Schwern <schwern@​pobox.com> http​://www.pobox.com/~schwern/
Perl6 Quality Assurance <perl-qa@​perl.org> Kwalitee Is Job One
I'm exploring my nipples.

@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2002

From @simoncozens

Created by simon@simon-cozens.org

This works​:

  { my $x = new Flow sub {print "Eat this, Hook​::Scope\n" }}

  package Flow;
  sub new { my $class = shift; bless {code => $_[0]}, $class }
  sub DESTROY { my $self = shift; goto &{$self->{code}};}

This doesn't​:

  { my $x = new Flow sub {print "Eat this, Hook​::Scope\n" }}

  package Flow;
  sub new { my $class = shift; bless $_[0], $class }
  sub DESTROY { my $self = shift; goto &$self;}

DESTROY is called in the first place but not in the second. This
isn't mentioned in the documentation, so either the documentation
or the code are wrong...

Perl Info

Flags:
    category=core
    severity=low

This perlbug was built using Perl v5.7.3 - Fri Jun 14 19:50:26 BST 2002
It is being executed now by  Perl v5.6.1 - Fri Jan 11 04:14:18 EST 2002.

Site configuration information for perl v5.6.1:

Configured by bod at Fri Jan 11 04:14:18 EST 2002.

Summary of my perl5 (revision 5.0 version 6 subversion 1) configuration:
  Platform:
    osname=linux, osvers=2.4.13, archname=i386-linux
    uname='linux duende 2.4.13 #1 wed oct 31 19:18:07 est 2001 i686 unknown '
    config_args='-Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=i386-linux -Dprefix=/usr -Dprivlib=/usr/share/perl/5.6.1 -Darchlib=/usr/lib/perl/5.6.1 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.6.1 -Dsitearch=/usr/local/lib/perl/5.6.1 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Uusesfio -Duseshrplib -Dlibperl=libperl.so.5.6.1 -Dd_dosuid -des'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef
    useperlio=undef d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
  Compiler:
    cc='cc', ccflags ='-DDEBIAN -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-DDEBIAN -fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='2.95.4  (Debian prerelease)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, usemymalloc=n, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lgdbm -ldb -ldl -lm -lc -lcrypt
    perllibs=-ldl -lm -lc -lcrypt
    libc=/lib/libc-2.2.4.so, so=so, useshrplib=true, libperl=libperl.so.5.6.1
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic'
    cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    DEVEL16693


@INC for perl v5.6.1:
    /usr/local/lib/perl/5.6.1
    /usr/local/share/perl/5.6.1
    /usr/lib/perl5
    /usr/share/perl5
    /usr/lib/perl/5.6.1
    /usr/share/perl/5.6.1
    /usr/local/lib/site_perl
    /usr/local/lib/perl/5.6.0
    /usr/local/share/perl/5.6.0
    .


Environment for perl v5.6.1:
    HOME=/home/simon
    LANG=en_GB
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/simon/bin:/bin:/usr/bin:/usr/local/bin:/usr/X11R6/bin:/usr/local/sbin:/usr/sbin:/sbin
    PERL_BADLANG (unset)
    SHELL=/usr/bin/zsh


@p5pRT
Copy link
Author

p5pRT commented Aug 17, 2003

From @iabyn

About a year ago, Simon Cozens reported a bug to the effect that

  { my $x = bless sub { }, 'X'; }
  sub X​::DESTROY { print "DESTROYED\n" }

Doesn't call the destructor.

Having looked at it, the reason is that the anon sub isn't a closure
(ie doesn't refer to any outer lexicals. Thus, the CV is shared rather
than cloned, so its refcnt is >1 when $x is freed.

Turning it into a closure makes it work​:

  my $y;
  { my $x = bless sub { $y }, 'X'; }
  sub X​::DESTROY { print "DESTROYED\n" }

Simon mentions that "either the documentation or the code are wrong".
Since it would be inefficient to fix, and since closureless anon subs
are more common that blessed coderefs (I would speculate), I think it
should be documented as a misfeature.

Dave.

--
In England there is a special word which means the last sunshine
of the summer. That word is "spring".

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @schwern

On Sun, Aug 17, 2003 at 11​:07​:15PM +0100, Dave Mitchell wrote​:

About a year ago, Simon Cozens reported a bug to the effect that

\{ my $x = bless sub \{ \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

Doesn't call the destructor.

Having looked at it, the reason is that the anon sub isn't a closure
(ie doesn't refer to any outer lexicals. Thus, the CV is shared rather
than cloned, so its refcnt is >1 when $x is freed.

Turning it into a closure makes it work​:

my $y;
\{ my $x = bless sub \{ $y \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

Simon mentions that "either the documentation or the code are wrong".
Since it would be inefficient to fix, and since closureless anon subs
are more common that blessed coderefs (I would speculate), I think it
should be documented as a misfeature.

Since it is unnecessarily surprising and inconsistent behavior, and since
the reason for it happening is purely for internal and not language reasons,
and since we don't want people to start relying on it, and since it would
be nice if someone could fix it sometime in the future despite predictions
that it will be inefficient to fix, it should be documented not as a feature
but as a bug.

Don't document bugs as features. You will hate yourself in the morning.

--
Michael G Schwern schwern@​pobox.com http​://www.pobox.com/~schwern/
Remember, any tool can be the right tool.
  -- Red Green

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @ysth

On Sun, Aug 17, 2003 at 05​:27​:51PM -0700, Michael G Schwern wrote​:

On Sun, Aug 17, 2003 at 11​:07​:15PM +0100, Dave Mitchell wrote​:

About a year ago, Simon Cozens reported a bug to the effect that

\{ my $x = bless sub \{ \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

Doesn't call the destructor.

Having looked at it, the reason is that the anon sub isn't a closure
(ie doesn't refer to any outer lexicals. Thus, the CV is shared rather
than cloned, so its refcnt is >1 when $x is freed.

Turning it into a closure makes it work​:

my $y;
\{ my $x = bless sub \{ $y \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

Simon mentions that "either the documentation or the code are wrong".
Since it would be inefficient to fix, and since closureless anon subs
are more common that blessed coderefs (I would speculate), I think it
should be documented as a misfeature.

Since it is unnecessarily surprising and inconsistent behavior, and since
the reason for it happening is purely for internal and not language reasons,
and since we don't want people to start relying on it, and since it would
be nice if someone could fix it sometime in the future despite predictions
that it will be inefficient to fix, it should be documented not as a feature
but as a bug.

Don't document bugs as features. You will hate yourself in the morning.

My initial reaction is to disagree. On second thought, I'm not so
sure. According to my initial reaction, sub {} is a kind of constant
(*unless* it is a closure). Viewed this way, it's perfectly
reasonable that an extra ref be hanging around. But on second
thought, if that is the case you shouldn't be able to bless it.
Either way, the issue is more than just whether it is destroyed. In
this code​:

perl -wle'sub cv { sub {} }; my $x = bless cv, 'X'; print ref cv'

either the bless should fail or the second call to cv should get a new
CV (thus printing CODE). And the latter sounds better to me (except
with something like sub :const { }).

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @simoncozens

Yitzchak Scott-Thoennes​:

my $y;
\{ my $x = bless sub \{ $y \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

Don't document bugs as features. You will hate yourself in the morning.

My initial reaction is to disagree. On second thought, I'm not so
sure.

My initial reaction is that calling a destructor on a sub that happens
to be a closure and not on one that doesn't is not just an unexpected
special case, but it's surely a bug. Why (and I ask this honestly, not
rhetorically) is it supposed to be so hard to fix?

--
  User​: In 1793 the french king was executed.
MegaHAL​: HA HA HA! CORRECT. ALTHOUGH, EXECUTED HAS MULTIPLE MEANINGS.

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @iabyn

On Sun, Aug 17, 2003 at 05​:27​:51PM -0700, Michael G Schwern wrote​:

On Sun, Aug 17, 2003 at 11​:07​:15PM +0100, Dave Mitchell wrote​:

Simon mentions that "either the documentation or the code are wrong".
Since it would be inefficient to fix, and since closureless anon subs
are more common that blessed coderefs (I would speculate), I think it
should be documented as a misfeature.
[snip]
Don't document bugs as features. You will hate yourself in the morning.

Note that I used the word 'misfeature' rather than 'feature'. So I was
suggesting to document it as "this may not work, and it may change
in future, so don't rely on it".

--
In my day, we used to edit the inodes by hand. With magnets.

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @iabyn

On Mon, Aug 18, 2003 at 05​:45​:29PM +0100, Simon Cozens wrote​:

My initial reaction is that calling a destructor on a sub that happens
to be a closure and not on one that doesn't is not just an unexpected
special case, but it's surely a bug. Why (and I ask this honestly, not
rhetorically) is it supposed to be so hard to fix?

Assuming that we wish to keep the optimisation of sharing non-closure anon
CVs, then the problem is that by the time you try to bless the coderef,
its already pointing to the shared CV, and you're buggered. By then, you
don't really have the option of unsharing it. At least I can't think of
a clean way.

--
A walk of a thousand miles begins with a single step...
then continues for another 1,999,999 or so.

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From kstar@cpan.org

On Aug 18, Dave Mitchell wrote​:

On Mon, Aug 18, 2003 at 05​:45​:29PM +0100, Simon Cozens wrote​:

My initial reaction is that calling a destructor on a sub that happens
to be a closure and not on one that doesn't is not just an unexpected
special case, but it's surely a bug. Why (and I ask this honestly, not
rhetorically) is it supposed to be so hard to fix?

Assuming that we wish to keep the optimisation of sharing non-closure anon
CVs, then the problem is that by the time you try to bless the coderef,
its already pointing to the shared CV, and you're buggered. By then, you
don't really have the option of unsharing it. At least I can't think of
a clean way.

  It seems very strange to me to say that an anonymous sub with
no private variables is "not a closure." Is not the (shorthand)
definition of a closure "a subroutine, associated with all its
bindings," even when "all" == 0?

  It's great that we have the optimization of not cloning CV's
unnecessarily. It's unfortunate that we have the misfeature of
Simon's no-DESTRUCT behavior. It's needlessly confusing to say
that anonymous subs that have no private bindings aren't closures.

  Remember the etymology of "closure." It's from set theory.
The empty set is a subset of *all* sets.

  - Kurt

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @chipdude

According to Simon Cozens​:

Yitzchak Scott-Thoennes​:

my $y;
\{ my $x = bless sub \{ $y \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

My initial reaction is that calling a destructor on a sub that happens
to be a closure and not on one that doesn't is not just an unexpected
special case, but it's surely a bug. Why (and I ask this honestly, not
rhetorically) is it supposed to be so hard to fix?

Because every value of

  $a = sub { print "hi" }

is the same CV. Printing $a will confirm this. The original owner of
the CV is the OP tree for the enclosing subroutine (or eval). Until
the owning OP tree goes away, the CV's refcount cannot fall to zero.
It's very much like

  sub hi { print "hi" }
  $a = \&hi;

The only difference (WRT GC) is that in the \&hi case the original
owner is the symbol table, which is visible and can be manipulated
from Perl.

In contrast, every value of

  my $foo;
  $b = sub { print $foo }

is a _new_ CV; printing $a will confirm this. Therefore there is no
'original' owner of each new CV; when $b (in this case) lets go, the
new CV can be GC'd (and thus DESTROYed). (BTW, there is a parent CV
from which all the closure CVs is cloned. This CV is totally hidden
from user code. It belongs to the OP tree.)
--
Chip Salzenberg - a.k.a. - <chip@​pobox.com>
"I wanted to play hopscotch with the impenetrable mystery of existence,
  but he stepped in a wormhole and had to go in early." // MST3K

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @nwc10

On Mon, Aug 18, 2003 at 05​:50​:16PM -0400, Chip Salzenberg wrote​:

According to Simon Cozens​:

Yitzchak Scott-Thoennes​:

my $y;
\{ my $x = bless sub \{ $y \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

My initial reaction is that calling a destructor on a sub that happens
to be a closure and not on one that doesn't is not just an unexpected
special case, but it's surely a bug. Why (and I ask this honestly, not
rhetorically) is it supposed to be so hard to fix?

Because every value of

$a = sub { print "hi" }

is the same CV. Printing $a will confirm this. The original owner of

Oh yes. Mmm. Arguably action at a distance​:

$ perl -le 'sub foo { return sub {}}; $a = foo; $b = foo; print ref $b; bless $a; print ref $b'
CODE
main

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @iabyn

On Mon, Aug 18, 2003 at 05​:49​:16PM -0400, Kurt Starsinic wrote​:

It seems very strange to me to say that an anonymous sub with

no private variables is "not a closure." Is not the (shorthand)
definition of a closure "a subroutine, associated with all its
bindings," even when "all" == 0?

It's great that we have the optimization of not cloning CV's

unnecessarily. It's unfortunate that we have the misfeature of
Simon's no-DESTRUCT behavior. It's needlessly confusing to say
that anonymous subs that have no private bindings aren't closures.

Remember the etymology of "closure\."  It's from set theory\.

The empty set is a subset of *all* sets.

Well, I've never been very conversant with set theory, so I've never
understood the etymology. I've always used it in the sense of something
which captures outside lexicals. Otherwise *every* sub is/has a closure,
and and the term seems rather to lose its usefulness.
Considering that most of the perl documentation still seems to imply
that only anon subs can be closures, I had assumed I was already
on the liberal wing as regards closure terminology :-)

Dave.

PS - I've been having some sendmail problems this afternoon, so replies to
some of my emails may have bounced or been silently queued, due the
hostname 'gizmo' leaking into sender addresses. Gack I hate sendmail.

--
Technology is dominated by two types of people​: those who understand what
they do not manage, and those who manage what they do not understand.

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2003

From @chipdude

According to Kurt Starsinic​:

Remember the etymology of "closure."

Etymology doesn't define usage, it merely illuminates its origins.
Were most everyone to say that X is not Y, then in a linguistic sense
it is reasonable to say that X is not Y, no matter the etymology of Y.

And yes that makes me a total descriptivist. But how can a Perl person
be otherwise, really?
--
Chip Salzenberg - a.k.a. - <chip@​pobox.com>
"I wanted to play hopscotch with the impenetrable mystery of existence,
  but he stepped in a wormhole and had to go in early." // MST3K

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2003

From @RandalSchwartz

"Kurt" == Kurt Starsinic <kstar@​cpan.org> writes​:

Kurt> It seems very strange to me to say that an anonymous sub with
Kurt> no private variables is "not a closure." Is not the (shorthand)
Kurt> definition of a closure "a subroutine, associated with all its
Kurt> bindings," even when "all" == 0?

Well, we need a term for that then. I've been adovcating "closure"
as when "all > 0", whether named or anonymous.

But if you have a better term (that doesn't take more than three
words) for the subroutines that generate a different CV when created
as opposed to those that don't, I'm all for it.

If "all subroutines are closures", then the term loses its usefulness
within the Perl world.

--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@​stonehenge.com> <URL​:http​://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2003

From sky@nanisky.com

On Monday, August 18, 2003, at 07​:12 pm, Dave Mitchell wrote​:

Assuming that we wish to keep the optimisation of sharing non-closure
anon
CVs, then the problem is that by the time you try to bless the coderef,
its already pointing to the shared CV, and you're buggered. By then,
you
don't really have the option of unsharing it. At least I can't think of
a clean way.

COW?

Arthur

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2003

From @iabyn

On Tue, Aug 19, 2003 at 05​:29​:09PM +0100, Arthur Bergman wrote​:

On Monday, August 18, 2003, at 07​:12 pm, Dave Mitchell wrote​:

Assuming that we wish to keep the optimisation of sharing non-closure
anon
CVs, then the problem is that by the time you try to bless the coderef,
its already pointing to the shared CV, and you're buggered. By then,
you
don't really have the option of unsharing it. At least I can't think of
a clean way.

COW?

AFAIKT, any COW scheme would break the following​:

  my $sub = sub { print "hello" };
  my $sub2 = $sub; # $sub and $sub1 point to the same CV
  bless $sub, 'Foo'; # $sub now points to a copy via COW
  print "not ok\n" unless $sub == $sub2;

--
The optimist believes that he lives in the best of all possible worlds.
As does the pessimist.

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2003

From kstar@cpan.org

On Aug 19, Randal L. Schwartz wrote​:

"Kurt" == Kurt Starsinic <kstar@​cpan.org> writes​:
Kurt> It seems very strange to me to say that an anonymous sub with
Kurt> no private variables is "not a closure." Is not the (shorthand)
Kurt> definition of a closure "a subroutine, associated with all its
Kurt> bindings," even when "all" == 0?

Well, we need a term for that then. I've been adovcating "closure"
as when "all > 0", whether named or anonymous.

But if you have a better term (that doesn't take more than three
words) for the subroutines that generate a different CV when created
as opposed to those that don't, I'm all for it.

  I suggest that it's a closure when the language supports
associating bindings to the subroutine. In Perl, this is always
and only the case for anonymous subroutines, whether or not the
number of private bindings > 0.

  So it seems to me that, in Perl, a named subroutine is never a
closure, and an anonymous subroutine is always a closure. The fact
that perl optimizes the special case of an anonymous subroutine with 0
private bindings is an implementation detail of interest to a tiny few.
This behavior is not guaranteed, and based on Simon's bug, I suspect
that it will change.

If "all subroutines are closures", then the term loses its usefulness
within the Perl world.

  I'm with you 100% there, Randal.

  - Kurt

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2003

From sky@nanisky.com

On Wednesday, August 20, 2003, at 04​:31 am, Kurt Starsinic wrote​:

So it seems to me that, in Perl, a named subroutine is never a
closure, and an anonymous subroutine is always a closure. The fact
that perl optimizes the special case of an anonymous subroutine with 0
private bindings is an implementation detail of interest to a tiny few.
This behavior is not guaranteed, and based on Simon's bug, I suspect
that it will change.

I would say an anonymous subroutine is just that, an anonymous
subroutine. It needs outside lexicals to be a closure.

However, I also think this issue is totally irrelevant since only a
select few are evil enough to do anything about it, and it is trivial
to have a wrapper object around it. Slowing down all anonymous
subroutines is just silly. Just document it, close the bug and get on
with life.

Arthur

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2003

From @iabyn

On Tue, Aug 19, 2003 at 11​:31​:48PM -0400, Kurt Starsinic wrote​:

I suggest that it's a closure when the language supports

associating bindings to the subroutine. In Perl, this is always
and only the case for anonymous subroutines, whether or not the
number of private bindings > 0.

So it seems to me that\, in Perl\, a named subroutine is never a

closure, and an anonymous subroutine is always a closure.

But named subs also capture their lexical state at creation time,
so I would call them closures too​:

X.pm​:
  package X;
  sub new { my $x = $_[1]; bless \$x }
  sub DESTROY { print "${$_[0]} destroyed\n" }

  my $x1 = X->new('x1');
  my $x2 = X->new('x2');
  sub f { $x2 }
p​:
  #!/usr/bin/perl -w

  print "in main\n";
  use lib '.';
  use X;

gizmo [d]$ ./p
x1 destroyed
in main
x2 destroyed
gizmo [d]$

Here, f captures $x2 and so stops it being destroyed at the end of
the compilation of X.pm. That is an important behaviour that deserves
a label. 'Closure' seems as good a label as any to me.

Similar comments aply to, eg

  {
  my $c = 0;
  sub inc { $c++ }
  sub dec { $c-- }
  }

--
Never do today what you can put off till tomorrow.

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2003

From @iabyn

On Wed, Aug 20, 2003 at 07​:12​:22PM +0100, Nick Ing-Simmons wrote​:

Dave Mitchell <davem@​fdgroup.com> writes​:

But named subs also capture their lexical state at creation time,

Things get confusing​:

sub harry
{
my $foo = shift;
sub fred { print "$foo\n" }
}

harry('Ouch');
harry('Weird');

fred();

Run it with -w and you get​:

Variable "$foo" will not stay shared at /tmp/p line 6.

Subs capture their lexical environment at creation time. When fred is
created, the instance of $foo it captures happens to be the first one.
A bit later, 'Ouch' is assigned to this instance. Even later, 'Weird' is
assigned to the second instance of $foo, but fred still only sees the
first instance.

D.

--
Justice is when you get what you deserve.
Law is when you get what you pay for.

@p5pRT
Copy link
Author

p5pRT commented Aug 21, 2003

From nick@ing-simmons.net

Dave Mitchell <davem@​fdgroup.com> writes​:

On Tue, Aug 19, 2003 at 11​:31​:48PM -0400, Kurt Starsinic wrote​:

I suggest that it's a closure when the language supports

associating bindings to the subroutine. In Perl, this is always
and only the case for anonymous subroutines, whether or not the
number of private bindings > 0.

So it seems to me that\, in Perl\, a named subroutine is never a

closure, and an anonymous subroutine is always a closure.

But named subs also capture their lexical state at creation time,

Things get confusing​:

sub harry
{
my $foo = shift;
sub fred { print "$foo\n" }
}

harry('Ouch');
harry('Weird');
 

fred();

so I would call them closures too​:

X.pm​:
package X;
sub new { my $x = $_[1]; bless \$x }
sub DESTROY { print "${$_[0]} destroyed\n" }

my $x1 = X->new('x1');
my $x2 = X->new('x2');
sub f { $x2 }
p​:
#!/usr/bin/perl -w

print "in main\n";
use lib '.';
use X;

gizmo [d]$ ./p
x1 destroyed
in main
x2 destroyed
gizmo [d]$

Here, f captures $x2 and so stops it being destroyed at the end of
the compilation of X.pm. That is an important behaviour that deserves
a label. 'Closure' seems as good a label as any to me.

Similar comments aply to, eg

{
my $c = 0;
sub inc { $c++ }
sub dec { $c-- }
}

@p5pRT
Copy link
Author

p5pRT commented Oct 17, 2004

From perl-5.8.0@ton.iguana.be

Created by perl-5.8.0@ton.iguana.be

perl -wle 'sub DESTROY { print "DESTROY" } my $a = bless sub {}; $a=undef'

The DESTROY never gets called.

Perl Info

Flags:
    category=core
    severity=low

This perlbug was built using Perl v5.8.5 - Sat Oct 16 01:07:18 CEST 2004
It is being executed now by  Perl v5.8.4 - Thu Jun  3 13:28:19 CEST 2004.

Site configuration information for perl v5.8.4:

Configured by ton at Thu Jun  3 13:28:19 CEST 2004.

Summary of my perl5 (revision 5 version 8 subversion 4) configuration:
  Platform:
    osname=linux, osvers=2.6.5, archname=i686-linux-64int-ld
    uname='linux quasar 2.6.5 #8 mon apr 5 05:41:20 cest 2004 i686 gnulinux '
    config_args=''
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=define use64bitall=undef uselongdouble=define
    usemymalloc=y, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -fomit-frame-pointer',
    cppflags='-fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='3.4.0 20031231 (experimental)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='long double', nvsize=12, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.3.2.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.3.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.8.4:
    /usr/lib/perl5/5.8.4/i686-linux-64int-ld
    /usr/lib/perl5/5.8.4
    /usr/lib/perl5/site_perl/5.8.4/i686-linux-64int-ld
    /usr/lib/perl5/site_perl/5.8.4
    /usr/lib/perl5/site_perl
    .


Environment for perl v5.8.4:
    HOME=/home/ton
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/ton/bin.Linux:/home/ton/bin:/home/ton/bin.SampleSetup:/opt/schily/bin:/usr/local/bin:/usr/local/sbin:/home/oracle/product/9.0.1/bin:/usr/local/ar/bin:/usr/games/bin:/usr/X11R6/bin:/usr/share/bin:/usr/bin:/usr/sbin:/bin:/sbin:.
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Oct 18, 2004

From @iabyn

On Sun, Oct 17, 2004 at 09​:10​:49PM -0000, perl-5. 8. 0 @​ ton. iguana. be wrote​:

perl -wle 'sub DESTROY { print "DESTROY" } my $a = bless sub {}; $a=undef'

The DESTROY never gets called.

This has been discussed before. When the anon sub isn't a closure, the
code CV is just shared, and so its refcnt never reaches zero.

eg

  $ perl -le'push @​a, sub {} for 1..5; print $_ for @​a'
  CODE(0x8102b48)
  CODE(0x8102b48)
  CODE(0x8102b48)
  CODE(0x8102b48)
  CODE(0x8102b48)
  $ perl -le'my $x;push @​a, sub {$x} for 1..5; print $_ for @​a'
  CODE(0x80f6370)
  CODE(0x8102c08)
  CODE(0x8102c50)
  CODE(0x8102c98)
  CODE(0x8102ce0)
  $

There doen't seem to be any way of fixing this without impacting the
performance of closureless anon sub creation.

Dave.

--
Spock (or Data) is fired from his high-ranking position for not being able
to understand the most basic nuances of about one in three sentences that
anyone says to him.
  -- Things That Never Happen in "Star Trek" #19

@p5pRT
Copy link
Author

p5pRT commented Oct 18, 2004

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Oct 18, 2004

From @nwc10

On Mon, Oct 18, 2004 at 05​:02​:13PM +0100, Dave Mitchell wrote​:

On Sun, Oct 17, 2004 at 09​:10​:49PM -0000, perl-5. 8. 0 @​ ton. iguana. be wrote​:

perl -wle 'sub DESTROY { print "DESTROY" } my $a = bless sub {}; $a=undef'

The DESTROY never gets called.

This has been discussed before. When the anon sub isn't a closure, the
code CV is just shared, and so its refcnt never reaches zero.

There doen't seem to be any way of fixing this without impacting the
performance of closureless anon sub creation.

Is it possible to retrospectively change a sub to be shared in the bless
operator?

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Oct 18, 2004

From mjtg@cam.ac.uk

Nicholas Clark <nick@​ccl4.org> wrote

Is it possible to retrospectively change a sub to be shared in the bless
operator?

Won't that make the reference point to a new CV? Will it break something
like

  my $a = sub { ... };
  my $b = $a;
  bless $b;
  print $a == $b;

Or is there an extra level of indirection?

I certainly think this bug ought to be fixed (and am slightly surprised that
I have never tripped over it).

Mike Guy

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2004

From @davidnicol

i don't think this matters.

When do you need to catch destruction of an anonymous coderef that is
not a closure?

Objects based on coderefs tend to be closures, so they can have their own
instance variables.

We could document the shared aspect in the documentation on DESTROY if
it's really that troubling

On Mon, 18 Oct 2004 18​:40​:03 +0100, Mike Guy <mjtg@​cam.ac.uk> wrote​:

I certainly think this bug ought to be fixed (and am slightly surprised that
I have never tripped over it).

Mike Guy

--
David L Nicol
transportation infrastructure technology contracting since 2002

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2004

From perl5-porters@ton.iguana.be

In article <934_64a2041018233558_d94e3@​mail.gmail.com>,
  David Nicol <davidnicol@​gmail.com> writes​:

i don't think this matters.

When do you need to catch destruction of an anonymous coderef that is
not a closure?

Objects based on coderefs tend to be closures, so they can have their own
instance variables.

We could document the shared aspect in the documentation on DESTROY if
it's really that troubling

The optimization for non-anonymous closures seems sensible, but it
should indeed be documented. If I'd known I could easily have worked
around the problem (funnily enough I only wanted to bless the sub to
check that I did proper cleanup of the object it got stored in, and I
solved it by not blessing the sub anymore, but putting a blessed something
inside the closure, exactly the kind of thing that would have solved the
original problem)

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2004

From @davidnicol

Here's i guess what you're asking for, if these lines were added to
perltoot.pod it
would make DESTROY even scarier. When is destruction deferred, currently?
the documentation says it is possible to stack up a bunch of DESTROY calls for
later, but doesn't it always happen immediately on refcount clearance,
in current
perls? The addition below gives an inappropriately detailed example for the
tutorial of the not-at-all case and no example for the deferred case.
Perldoc does
not appear to have a reference page for DESTROY. Maybe a section on
"Destroy gotchas"
belongs in perltie.pod, near the discussion of UNTIE, and referred to in the
introduction to DESTROY in tiescalar, with a "see below."

Imagine adding these lines to perltoot, and wince​:

  Perl's notion of the right time to call a destructor is not
well-defined currently, which is
  why your destructors should not rely on when they are called.
+ Generally, DESTROY is called when the reference count on a blessed
object drops
+ to zero. Sometimes this is deferred, and sometimes Perl's optimizer
will reuse a
+ reference, such as to an anonymous subroutine that is not a closure,
so the refcount
+ will never drop to zero, and DESTROY will not be called at all.

On Tue, 19 Oct 2004 08​:48​:17 +0000 (UTC), Ton Hospel
<perl5-porters@​ton.iguana.be> wrote​:

In article <934_64a2041018233558_d94e3@​mail.gmail.com>,
David Nicol <davidnicol@​gmail.com> writes​:

i don't think this matters.

When do you need to catch destruction of an anonymous coderef that is
not a closure?

Objects based on coderefs tend to be closures, so they can have their own
instance variables.

We could document the shared aspect in the documentation on DESTROY if
it's really that troubling

The optimization for non-anonymous closures seems sensible, but it
should indeed be documented. If I'd known I could easily have worked
around the problem (funnily enough I only wanted to bless the sub to
check that I did proper cleanup of the object it got stored in, and I
solved it by not blessing the sub anymore, but putting a blessed something
inside the closure, exactly the kind of thing that would have solved the
original problem)

--
David L Nicol
transportation infrastructure technology contracting since 2002

@p5pRT
Copy link
Author

p5pRT commented May 1, 2006

From @nothingmuch

On Mon, May 01, 2006 at 19​:49​:41 +0100, Dave Mitchell wrote​:

If bless just does a copy-on-write, then this breaks​:

$ perl588 -we '$a = sub {}; $b = $a; bless $a; print "not ok\n" if $a != $b'

That's technically copying the reference, but yeah.... Either way

but that's roughly what a CV is anyway​: it's an SV that contains pointers
to an op tree and a scratchpad.

Oh... I was assuming if it were that way then the optimization would
not be so substantial ;-)

In theory a CV could be a pointer to what a CV is now, and *that*
could contain a copy count, but the extra indirection is probably
going to be slower, overall, than copying that bit.

I still would like proper destructors for code refs - i have a
stupid module up on the CPAN for exactly that reason - it's a
wrapper object that overloads deref-as-code and allows you to
specify a custom destructor. It doesn't work when you have a bit of
code that takes either a code ref or an object, of course.

But ideally if you have a sub that returns a closure, and then some
day stops returning a closure but returns a regular sub with the
same behavior, then the user of that code reference would get
consistent blessing behavior.

Thanks for clarifying,

--
  Yuval Kogman <nothingmuch@​woobling.org>
http​://nothingmuch.woobling.org 0xEBD27418

@p5pRT
Copy link
Author

p5pRT commented May 1, 2006

From @iabyn

On Mon, May 01, 2006 at 09​:05​:30PM +0300, Yuval Kogman wrote​:

On Mon, May 01, 2006 at 19​:49​:41 +0100, Dave Mitchell wrote​:

but that's roughly what a CV is anyway​: it's an SV that contains pointers
to an op tree and a scratchpad.

Oh... I was assuming if it were that way then the optimization would
not be so substantial ;-)

I should imagine it's copying the scratchpad that takes most of the time.
Hmm, perhaps we could just share the scratchpads for non-closure anon
subs?

--
Justice is when you get what you deserve.
Law is when you get what you pay for.

@p5pRT
Copy link
Author

p5pRT commented May 1, 2006

From @nwc10

On Mon, May 01, 2006 at 06​:49​:41PM +0100, Dave Mitchell wrote​:

On Mon, May 01, 2006 at 08​:21​:24PM +0300, Yuval Kogman wrote​:

bless currently just updates the pointed-top CV.

Hence copy on right

If bless just does a copy-on-write, then this breaks​:

$ perl588 -we '$a = sub {}; $b = $a; bless $a; print "not ok\n" if $a != $b'

So the problem is that there are two pairs of two in​:

$ perl -lwe 'sub foo { my $q = sub {}; ($q, $q) } my @​a = foo(); my @​b = foo; print "@​a / @​b"'
CODE(0xfdd84) CODE(0xfdd84) / CODE(0xfdd84) CODE(0xfdd84)

but currently it's not clear which they are :-)

I like the idea of the real copies but shared scratchpads. I hope it's viable.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented May 2, 2006

From @nothingmuch

On Mon, May 01, 2006 at 20​:55​:21 +0100, Dave Mitchell wrote​:

Oh... I was assuming if it were that way then the optimization would
not be so substantial ;-)

I should imagine it's copying the scratchpad that takes most of the time.
Hmm, perhaps we could just share the scratchpads for non-closure anon
subs?

Why is it copied? I guessed (in my prior email) that it's not
captured vars, but just the sub proto and allocation info, right?
In general the only runtime writable part of the scratch pad are the
free variables... Perhaps closures should have the pad split up?

--
  Yuval Kogman <nothingmuch@​woobling.org>
http​://nothingmuch.woobling.org 0xEBD27418

@p5pRT
Copy link
Author

p5pRT commented May 4, 2006

From @iabyn

On Tue, May 02, 2006 at 03​:23​:16AM +0300, Yuval Kogman wrote​:

On Mon, May 01, 2006 at 20​:55​:21 +0100, Dave Mitchell wrote​:

Oh... I was assuming if it were that way then the optimization would
not be so substantial ;-)

I should imagine it's copying the scratchpad that takes most of the time.
Hmm, perhaps we could just share the scratchpads for non-closure anon
subs?

Why is it copied? I guessed (in my prior email) that it's not
captured vars, but just the sub proto and allocation info, right?
In general the only runtime writable part of the scratch pad are the
free variables... Perhaps closures should have the pad split up?

Every sub has its own scratchpad. The scratchpad contains pointers to​:
* lexical vars
* captured outer lexical vars
* tmp SVs for OPs
* for threaded builds​: assorted SVs that have been migrated from the OP
  tree since that is shared between threads.

The only exception to this are closureless anon subs which share the
scratchpad.

The problem with splitting off the closure vars from the scratchpad in
order to avoid copying the scratchpad each time, is that it just
generalises the problems we have currently with sharing scratchpads of
closureless anon subs into a problem for all anon subs.

--
My get-up-and-go just got up and went.

@p5pRT
Copy link
Author

p5pRT commented May 6, 2006

From @iabyn

On Mon, May 01, 2006 at 07​:55​:21PM +0100, Dave Mitchell wrote​:

I should imagine it's copying the scratchpad that takes most of the time.
Hmm, perhaps we could just share the scratchpads for non-closure anon
subs?

I've knocked up a proof-of-concept patch (it fails loads of tests, but
works for basic code).

The timings I get are as follows​:

orig perl (share CV and scratchpad)​:

  0m2.292s sub {} for 1..10_000_000

hacked perl (copy CV, but share scratchpad)​:

  0m6.564s sub {} for 1..10_000_000

orig perl, but full closure (CV and scratchpad copied)​:

  0m18.229s my $x; sub {$x} for 1..10_000_000

However, there's a problem with copying the CV while sharing the pad​:
If one of the sub calls another copy of the sub, then they share the
same set of lexicals and temps. The following contrived example shows
how the inner call corrupts the outer sub's $x​:

  push @​s, sub { my $x=1; $s[0] && &{pop @​s}; print "x=$x\n" } for 1,2;
  &{pop @​s};

which gives​:

  x=1
  x=

whereas in normal perl it gives

  x=1
  x=1

this is becuase with the CV being shared, they share CvDEPTH and thus
when one sub calls the other, CvDEPTH gets incremented and so they use
different levels in the pad.

--
But Pity stayed his hand. "It's a pity I've run out of bullets",
he thought. - "Bored of the Rings"

@p5pRT
Copy link
Author

p5pRT commented Sep 28, 2011

@rspier - Status changed from 'open' to 'deleted'

@p5pRT p5pRT closed this as completed Sep 28, 2011
@p5pRT
Copy link
Author

p5pRT commented Dec 25, 2011

@ikegami - Status changed from 'deleted' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Dec 25, 2011

From @ikegami

Ticket wasn't reopened after being mistakenly reported as spam. Fixed.

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2011

From @dolmen

Le Dim. Ao�. 17 15​:07​:59 2003, davem a �crit�​:

About a year ago, Simon Cozens reported a bug to the effect that

\{ my $x = bless sub \{ \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

Doesn't call the destructor.

Having looked at it, the reason is that the anon sub isn't a closure
(ie doesn't refer to any outer lexicals. Thus, the CV is shared rather
than cloned, so its refcnt is >1 when $x is freed.

What we are seeing here is that "sub {}" is a kind of shared constant.
Just like '"foo"' or '1'.

Blessing multiple times the same value multiple time can only lead to
unexpected behavior and I don't a single case where blessing (and
re-blessing) the same non-closure sub would be useful. In general bless
should only succeed on real not-shared references, which in the CODE
case are closures.

So I think that the proper fix for this issue would be that bless die
for non-closure subs as it does in the other constants cases (where it
dies with "Can't bless non-reference value"). The error message would be
"Can't bless non closure sub".
If the user wanted a unique object the workaround (transforming the sub
in a real closure) is easy and obvious as has already been shown in this
ticket.

To mitigate issues with existing code (but I would be interested to see
such usage), we could just warn on the first bless and die only on
re-blessing. But my preference still goes to "die always".

Olivier.

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2011

From [Unknown Contact. See original ticket]

Le Dim. Ao�. 17 15​:07​:59 2003, davem a �crit�​:

About a year ago, Simon Cozens reported a bug to the effect that

\{ my $x = bless sub \{ \}\, 'X'; \}
sub X&#8203;::DESTROY \{ print "DESTROYED\\n" \}

Doesn't call the destructor.

Having looked at it, the reason is that the anon sub isn't a closure
(ie doesn't refer to any outer lexicals. Thus, the CV is shared rather
than cloned, so its refcnt is >1 when $x is freed.

What we are seeing here is that "sub {}" is a kind of shared constant.
Just like '"foo"' or '1'.

Blessing multiple times the same value multiple time can only lead to
unexpected behavior and I don't a single case where blessing (and
re-blessing) the same non-closure sub would be useful. In general bless
should only succeed on real not-shared references, which in the CODE
case are closures.

So I think that the proper fix for this issue would be that bless die
for non-closure subs as it does in the other constants cases (where it
dies with "Can't bless non-reference value"). The error message would be
"Can't bless non closure sub".
If the user wanted a unique object the workaround (transforming the sub
in a real closure) is easy and obvious as has already been shown in this
ticket.

To mitigate issues with existing code (but I would be interested to see
such usage), we could just warn on the first bless and die only on
re-blessing. But my preference still goes to "die always".

Olivier.

@p5pRT
Copy link
Author

p5pRT commented Jan 3, 2012

From zefram@fysh.org

Olivier Mengu?? via RT wrote​:

Blessing multiple times the same value multiple time can only lead to
unexpected behavior

No, it has perfectly well-defined behaviour, and can reasonably be
used deliberately.

So I think that the proper fix for this issue would be that bless die
for non-closure subs as it does in the other constants cases (where it
dies with "Can't bless non-reference value").

I get "Modification of a read-only value attempted" if I try, say,
bless(\1, "Foo"). Note that it's the thing the reference points at,
not the reference itself, that gets blessed, so bless(1, "Foo"), which
is what gets "Can't bless non-reference value", is nonsensical rather
than just not permitted.

It would be vaguely sensible for non-closure subs to be marked readonly
and so be unblessable, but it seems equally sensible for closures too.
It should be possible to bless subs somehow, though, so we can't very well
have all (non-closure) subs readonly to start with. It's also way too
late to change the default behaviour. If you want a sub to be readonly
that can be arranged (Internals​::SetReadOnly from the CPAN version of
Internals), and you can sensibly have a :ro attribute to express it.

It's not sensible to change the existing behaviour of blessing subs.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2014

From @FGasper

Created by @FGasper

Consider the following​:

===================
package Finally;

use strict;

sub new {
  my ( $class, $todo_cr ) = @​_;
  return bless $todo_cr, $class;
}

sub DESTROY {
  my ($self) = @​_;
  return $self->();
}

#----------------------------------------------------------------------

package main;

use strict;

sub finally1 {
  my $foo;
  my $finally = Finally->new( sub { print 'haha' } );
}

sub finally2 {
  my $foo;
  my $finally = Finally->new( sub { print 'hoho'; $foo = 1 } );
}

finally1();
finally2();

Notice that only "hoho" prints, not "haha", even though both coderefs
are blessed and should fire a DESTROY handler.

Workaround is just to wrap the coderef in an arrayref...but, still a bug
that should be fixed.

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.14.4:

Configured by cPanel at Thu Jul 10 15:51:11 CDT 2014.

Summary of my perl5 (revision 5 version 14 subversion 4) configuration:
   
  Platform:
    osname=linux, osvers=2.6.32-358.23.2.el6.i686, archname=i386-linux-64int
    uname='linux rpmb-32-centos-6 2.6.32-358.23.2.el6.i686 #1 smp wed oct 16 17:21:31 utc 2013 i686 i686 i386 gnulinux '
    config_args='-des -Dusedevel -Darchname=i386-linux-64int -Dcc=/usr/bin/gcc -Dcpp=/usr/bin/cpp -DDEBUGGING=none -Doptimize=-Os -Dusemymalloc=n -Duseshrplib -Duselargefiles=yes -Duseposix=true -Dhint=recommended -Duseperlio=yes -Dccflags=-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -DAPPLLIB_EXP="/usr/local/cpanel" -Dcppflags=-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -Dldflags=-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -Dprefix=/usr/local/cpanel/3rdparty/perl/514 -Dsiteprefix=/opt/cpanel/perl5/514 -Dsitebin=/opt/cpanel/perl5/514/bin -Dsitelib=/opt/cpanel/perl5/514/site_lib -Dusevendorprefix=true -Dvendorbin=/usr/local/cpanel/3rdparty/perl/514/bin -Dvendorprefix=/usr/local/cpanel/3rdparty/perl/514/lib/perl5 -Dvendorlib=/usr/local/cpanel/3rdparty/perl/514/lib/perl5/cpanel_lib -Dprivlib=/usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.4 -Dman1dir=none -Dman3dir=none -Dscriptdir=/usr/local/cpanel/3rdparty/perl/514/bin -Dscriptdirexp=/usr/local/cpanel/3rdparty/perl/514/bin -Dsiteman1dir=none -Dsiteman3dir=none -Dinstallman1dir=none -Dversiononly=no -Dinstallusrbinperl=no -Dcf_by=cPanel -Dmyhostname=localhost -Dperladmin=root@localhost -Dcf_email=support@cpanel.net -Di_dbm=/usr/local/cpanel/3rdparty/include -Di_gdbm=/usr/local/cpanel/3rdparty/include -Di_ndbm=/usr/local/cpanel/3rdparty/include -Ud_dosuid -Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks -Uuselongdouble -Ui_db -Aldflags=-L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -lgdbm -Dlocincpth=/usr/local/cpanel/3rdparty/perl/514/include /usr/local/cpanel/3rdparty/include /usr/local/include  -Duse64bitint -Uuse64bitall -Acflags=-fPIC -DPIC -m32 -I/usr/local/cpanel/3rdparty/perl/514/include -I/usr/local/cpanel/3rdparty/include -Dlibpth=/usr/local/cpanel/3rdparty/perl/514/lib /usr/local/cpanel/3rdparty/lib /usr/local/lib /lib /usr/lib '
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='/usr/bin/gcc', ccflags ='-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -DAPPLLIB_EXP="/usr/local/cpanel" -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-Os',
    cppflags='-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -DAPPLLIB_EXP="/usr/local/cpanel" -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.4.7 20120313 (Red Hat 4.4.7-3)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='/usr/bin/gcc', ldflags ='-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -lgdbm -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.12.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.12'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.4/i386-linux-64int/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -Os -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -L/usr/local/lib -fstack-protector'

Locally applied patches:
    cPanel patches
    cPanel INC path changes
    Disabled some unstable tests on a kvm build server
    Cherry pick of 49498ca from p5p (RT 113514)
    Remove improper use of each() in B::walksymtable (5cc8528c90)


@INC for perl 5.14.4:
    /usr/local/cpanel
    /usr/local/cpanel
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/cpanel_lib/i386-linux-64int
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/cpanel_lib
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.4/i386-linux-64int
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.4
    /opt/cpanel/perl5/514/site_lib/i386-linux-64int
    /opt/cpanel/perl5/514/site_lib
    .


Environment for perl 5.14.4:
    HOME=/root
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/root/.opt/bin:/usr/local/cpanel/3rdparty/perl/514/bin:/usr/local/cpanel/3rdparty/bin:/usr/local/cpanel/3rdparty/node/bin:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
    PERL5LIB=/usr/local/cpanel
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2014

From @cpansprout

On Mon Aug 18 18​:29​:12 2014, felipe@​felipegasper.com wrote​:

This is a bug report for perl from felipe@​felipegasper.com,
generated with the help of perlbug 1.39 running under perl 5.14.4.

-----------------------------------------------------------------
[Please describe your issue here]

Consider the following​:

===================
package Finally;

use strict;

sub new {
my ( $class, $todo_cr ) = @​_;
return bless $todo_cr, $class;
}

sub DESTROY {
my ($self) = @​_;
return $self->();
}

#----------------------------------------------------------------------

package main;

use strict;

sub finally1 {
my $foo;
my $finally = Finally->new( sub { print 'haha' } );
}

sub finally2 {
my $foo;
my $finally = Finally->new( sub { print 'hoho'; $foo = 1 } );
}

finally1();
finally2();

Notice that only "hoho" prints, not "haha", even though both coderefs
are blessed and should fire a DESTROY handler.

Workaround is just to wrap the coderef in an arrayref...but, still a
bug
that should be fixed.

This is a known longstanding bug (#3306) that is not easy to fix without severe performance impact.

(I’m merging this with that ticket.)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Oct 30, 2014

From @cpansprout

Ah! I found it!

This is the ticket I was looking for. I discussed this same issue in <20141028053445.4478.qmail@​lists-nntp.develooper.com>. The ideas that I thought were my own have already been suggested and discussed here. :-)

On Sat May 06 15​:55​:35 2006, davem@​iabyn.com wrote​:

On Mon, May 01, 2006 at 07​:55​:21PM +0100, Dave Mitchell wrote​:

I should imagine it's copying the scratchpad that takes most of the time.
Hmm, perhaps we could just share the scratchpads for non-closure anon
subs?

I've knocked up a proof-of-concept patch (it fails loads of tests, but
works for basic code).

The timings I get are as follows​:

orig perl (share CV and scratchpad)​:

0m2\.292s    sub \{\} for 1\.\.10\_000\_000

hacked perl (copy CV, but share scratchpad)​:

0m6\.564s    sub \{\} for 1\.\.10\_000\_000

orig perl, but full closure (CV and scratchpad copied)​:

0m18\.229s    my $x; sub \{$x\} for 1\.\.10\_000\_000

With my hacked-up perl on the sprout/padlist-sharing branch (which fails about 20 test scripts), I get​:

orig perl (share CV and scratchpad)​:

0m0.980s sub {} for 1..10_000_000

hacked perl (copy CV, but share scratchpad)​:

0m3.130s sub {} for 1..10_000_000

orig perl, but full (dis)closure (CV and scratchpad copied)​:

0m9.695s my $x; sub {$x} for 1..10_000_000

So sub{} that returns a new sub each time is only a third of the speed.

Is that acceptable?

I think the most compelling reason for fixing this is that the behaviour is different under the debugger. If one’s code is not working, but it just magically does the right thing in the debugger, that’s scary action at a distance, making these sorts of problems hard to track down.

However, there's a problem with copying the CV while sharing the pad​:
If one of the sub calls another copy of the sub, then they share the
same set of lexicals and temps. The following contrived example shows
how the inner call corrupts the outer sub's $x​:

push @&#8203;s\, sub \{ my $x=1; $s\[0\] && &\{pop @&#8203;s\}; print "x=$x\\n" \} for 1\,2;
&\{pop @&#8203;s\};

which gives​:

x=1
x=

whereas in normal perl it gives

x=1
x=1

this is becuase with the CV being shared, they share CvDEPTH and thus
when one sub calls the other, CvDEPTH gets incremented and so they use
different levels in the pad.

That problem I don’t have because I followed your suggestion of putting CvDEPTH in the padlist.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Nov 3, 2014

From @iabyn

On Wed, Oct 29, 2014 at 09​:38​:43PM -0700, Father Chrysostomos via RT wrote​:

Ah! I found it!

This is the ticket I was looking for. I discussed this same issue in
<20141028053445.4478.qmail@​lists-nntp.develooper.com>. The ideas that I
thought were my own have already been suggested and discussed here. :-)

"There is nothing new under the sun" :-)

On Sat May 06 15​:55​:35 2006, davem@​iabyn.com wrote​:

On Mon, May 01, 2006 at 07​:55​:21PM +0100, Dave Mitchell wrote​:

I should imagine it's copying the scratchpad that takes most of the time.
Hmm, perhaps we could just share the scratchpads for non-closure anon
subs?

I've knocked up a proof-of-concept patch (it fails loads of tests, but
works for basic code).
With my hacked-up perl on the sprout/padlist-sharing branch (which fails about 20 test scripts), I get​:

orig perl (share CV and scratchpad)​:

0m0.980s sub {} for 1..10_000_000

hacked perl (copy CV, but share scratchpad)​:

0m3.130s sub {} for 1..10_000_000

orig perl, but full (dis)closure (CV and scratchpad copied)​:

0m9.695s my $x; sub {$x} for 1..10_000_000

So sub{} that returns a new sub each time is only a third of the speed.

Is that acceptable?

Well, 'sub {} for 1..10_000_000' is a bit of a contrived benchmark; in the
real world, one might typically expect a newly created anon sub to be
stored somewhere, and to be called at least once; and for that sub to do a
minimal amount of work. In that case, the extra work done might make the
savings in clone time mere noise. Consider this benchmark​:

  my $r = { foo => 1 };
  for (1..10_000_000) {
  #sub {};
  my $x = sub {$_[0]{foo} };
  my $y = $x->($r);
  }

which represents a fairly minimal non-closure anon sub that does
something accessor-ish. Running with just the sub {} line takes 0.614s.
Running instead with the $x and $y lines takes 3.339s. Assuming the times 3
slowdown you got with your hacked perl and I got with my hacked perl, that
would add 0.678s for the sub cloning, which would change the 'real world'
code

  from​: 3.339s
  to​: 4.017s

a slowdown of 20%.

I think this would be at the margins of acceptability.

I think the most compelling reason for fixing this is that the behaviour
is different under the debugger. If one’s code is not working, but it
just magically does the right thing in the debugger, that’s scary action
at a distance, making these sorts of problems hard to track down.

I think the fact that blessing one sub blesses them all is a fairly
serious bug too.

--
If life gives you lemons, you'll probably develop a citric acid allergy.

@p5pRT
Copy link
Author

p5pRT commented Nov 3, 2014

From siracusa@gmail.com

On Mon, Nov 3, 2014 at 7​:36 AM, Dave Mitchell <davem@​iabyn.com> wrote​:

On Wed, Oct 29, 2014 at 09​:38​:43PM -0700, Father Chrysostomos via RT wrote​:

Ah! I found it!

This is the ticket I was looking for. I discussed this same issue in
<20141028053445.4478.qmail@​lists-nntp.develooper.com>. The ideas that I
thought were my own have already been suggested and discussed here. :-)

"There is nothing new under the sun" :-)

On Sat May 06 15​:55​:35 2006, davem@​iabyn.com wrote​:

On Mon, May 01, 2006 at 07​:55​:21PM +0100, Dave Mitchell wrote​:

I should imagine it's copying the scratchpad that takes most of the
time.
Hmm, perhaps we could just share the scratchpads for non-closure anon
subs?

I've knocked up a proof-of-concept patch (it fails loads of tests, but
works for basic code).
With my hacked-up perl on the sprout/padlist-sharing branch (which fails
about 20 test scripts), I get​:

orig perl (share CV and scratchpad)​:

0m0.980s sub {} for 1..10_000_000

hacked perl (copy CV, but share scratchpad)​:

0m3.130s sub {} for 1..10_000_000

orig perl, but full (dis)closure (CV and scratchpad copied)​:

0m9.695s my $x; sub {$x} for 1..10_000_000

So sub{} that returns a new sub each time is only a third of the speed.

Is that acceptable?

Well, 'sub {} for 1..10_000_000' is a bit of a contrived benchmark; in the
real world, one might typically expect a newly created anon sub to be
stored somewhere, and to be called at least once; and for that sub to do a
minimal amount of work. In that case, the extra work done might make the
savings in clone time mere noise. Consider this benchmark​:

my $r = \{ foo => 1 \};
for \(1\.\.10\_000\_000\) \{
    \#sub \{\};
    my $x = sub \{$\_\[0\]\{foo\} \};
    my $y = $x\->\($r\);
\}

which represents a fairly minimal non-closure anon sub that does
something accessor-ish. Running with just the sub {} line takes 0.614s.
Running instead with the $x and $y lines takes 3.339s. Assuming the times 3
slowdown you got with your hacked perl and I got with my hacked perl, that
would add 0.678s for the sub cloning, which would change the 'real world'
code

from&#8203;:  3\.339s
to&#8203;:    4\.017s

a slowdown of 20%.

I think this would be at the margins of acceptability.

I think the most compelling reason for fixing this is that the behaviour
is different under the debugger. If one’s code is not working, but it
just magically does the right thing in the debugger, that’s scary action
at a distance, making these sorts of problems hard to track down.

I think the fact that blessing one sub blesses them all is a fairly
serious bug too.

--
If life gives you lemons, you'll probably develop a citric acid allergy.

@demerphq
Copy link
Collaborator

demerphq commented Feb 11, 2023

What I do see however is that just about every luminary in the perl community that I know of has commented on it, and/or been bitten by the consequences of this. As have I.

@iabyn suggested that if we did fix this it would slow down construction of anonymous subs by about 20%.

Someone else suggested we make it so that bless detects it is blessing an anonymous sub that is not enclosing vars and refuse to bless it. ysth pointed out this is problematic, as people often do bless such subs.

Multiple people suggested we document it an move on.

I think that most people see code like

sub {}

as a constructor for a new sub. However apparently when the sub is not a closure that is not what it does, and instead it ends up being closer to this code:

do { state $sub= sub { ... }; $sub }

I think we should resolve these tickets once and for all. As far as I can tell we have three options:

  1. Document it and be done with it.
  2. Make bless warn when a non-closure anonymous sub is blessed into two different namespaces.
  3. Make non-closure and anonymous subs behave the same and accept the slow down we get.

My personal feeling is that the best option is 3, I am not convinced that a 20% slowdown in constructing anonymous non-closure subs is a cost anyone should care about. I do not understand the optree well enough yet to make a patch myself. It looks like @cpansprout produced one which had pretty good performance, which is still in the repo (at least in mine) as sprout/padlist-sharing.

But i could live with 2. I don't think 1 is a good idea. I think most people would bump into this issue /then/ read the docs and find it.

I also kinda wonder if we could "think outside of the box" here, and maybe make a "light" wrapper around true SVt_CODE (maybe via SVt_PVLV or something like it) so that such subs get an extra wrapper, which is what gets blessed. Thus we could keep the optimization but be able to bless the wrapper independently.

It really feels to me like we can and should solve this.

@demerphq
Copy link
Collaborator

BTW, I pushed yves/sprout-padlist-sharing as a rebase of sprout/padlist-sharing.

It doesnt pass test, but then i guess that is expected. I would sure love a patch that does fix this (IE make it so anonymous closures and non-closures are treated the same) so we could benchmark it and assess what the impact might be of fixing it.

@demerphq
Copy link
Collaborator

This hasnt been resolved, so I am reopening.

@demerphq demerphq reopened this Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants