The bug and the fix:
GH #21489 [1] fixed a bug introduced back in v5.17.1-26-ga0d2bbd5c4
which broke some cases of the special behaviour of eval in package DB,
where such an eval works in the lexical scope of the first non-DB
stack frame.
This can be used by the debugger to display or modify lexicals in the
code the debugger is controlling and is a long standing and documented
feature of Perl.
The change in v5.17.1-26-ga0d2bbd5c4 was to make only closures that
contain eval keep a reference to their outer scope, e.g.
my $y; # used to make the outer subs closures
my $no_outer = sub { $y; my $x; sub { ++$x; } }->();
my $has_outer = sub { $y; my $x; sub { eval ""; ++$x; } }->();
so $has_outer keeps a reference to the outer anonymous sub, while
$no_outer does not.
This allows the outer sub of $no_outer to be released before its inner
sub.
This also means v5.17.1-26-ga0d2bbd5c4 broke eval in package DB in
some cases, for code like:
use strict;
package DB { sub myeval { eval shift or die } }
my $y = 1; # something true so eval in myeval above is true
my $has_outer = sub { $y; my $x; sub { DB::myeval '$y'; eval "" ++$x; } }->();
$has_outer->(): # has an outside pointer and can resolve $y
my $no_outer = sub { $y; my $x; sub { DB::myeval '$y'; ++$x; } }->();
$no_outer->(): # dies since we can't see $y
In the $no_outer case the debugger can't see the grandparent scopes
lexicals, since the chain to the outer scopes is broken.
The fix in GH #21489 added tests for eval in package DB and reverted
v5.17.1-26-ga0d2bbd5c4, so both cases above produce the same results,
as they did prior in 5.16 and earlier.
The problem remaining:
Unfortunately this means that for the non-eval cases the lifetime of
the outer sub is extended, and in the case of #21524 [2], this results
in a reference loop in t/race_asyncs.t and some other Promise::ES6
tests.
It happens that these tests check the lifetime of the objects it
creates and so the tests fail, but if other code on CPAN or private
code uses similar constructs, they may silently[3] and continually
accumulate unreleased objects.
I can easily provide a fix specific to Promise::ES6 by ensuring the
subs don't close over so much (or anything), but this reduces the
locality of reference of the code and makes it harder to read.
It also doesn't fix any other potential code silently accumulating
objects.
Potential fixes:
In a comment in #21489 [4] Dave suggested using something like the
existing CvWEAKOUTSIDE() flag to make the outside links weak,
I don't think we can use CvWEAKOUTSIDE() as it is, since (I think)
that depends on the outside specifically holding the inner CV in some
way, but let's say we implement something similar, possibly just using
a weak reference.
If we make the weak outside unconditional, we'll have the possibility
of a `DB::eval` implementation failing to find outer lexicals where it
could find them before 386907f (the revert from #21489) if the outside
is released before the inside CV.
If we do re-implement CvHASEVAL() we have some of the same problem as
before 386907f: eval in the closure controls the lifetime of the
outside sub. This is compatible with pre-386907f but I think it is a
wart, since adding a C<eval "..."> may make your code start leaking.
The problem I see with leaving it as with 386907f is we have a silent
change in behaviour unless the user is specifically testing for it,
which Promise::ES6 does.
I don't know what to do here.
Tony
[1] https://github.com/Perl/perl5/pull/21489
[2] https://github.com/Perl/perl5/issues/21524
[3] if we still used spinning rust in general we'd hear the swapping
from the leaked objects eventually.
[4] https://github.com/Perl/perl5/pull/21489#issuecomment-1725443594
GH #21489 [1] fixed a bug introduced back in v5.17.1-26-ga0d2bbd5c4
which broke some cases of the special behaviour of eval in package DB,
where such an eval works in the lexical scope of the first non-DB
stack frame.
This can be used by the debugger to display or modify lexicals in the
code the debugger is controlling and is a long standing and documented
feature of Perl.
The change in v5.17.1-26-ga0d2bbd5c4 was to make only closures that
contain eval keep a reference to their outer scope, e.g.
my $y; # used to make the outer subs closures
my $no_outer = sub { $y; my $x; sub { ++$x; } }->();
my $has_outer = sub { $y; my $x; sub { eval ""; ++$x; } }->();
so $has_outer keeps a reference to the outer anonymous sub, while
$no_outer does not.
This allows the outer sub of $no_outer to be released before its inner
sub.
This also means v5.17.1-26-ga0d2bbd5c4 broke eval in package DB in
some cases, for code like:
use strict;
package DB { sub myeval { eval shift or die } }
my $y = 1; # something true so eval in myeval above is true
my $has_outer = sub { $y; my $x; sub { DB::myeval '$y'; eval "" ++$x; } }->();
$has_outer->(): # has an outside pointer and can resolve $y
my $no_outer = sub { $y; my $x; sub { DB::myeval '$y'; ++$x; } }->();
$no_outer->(): # dies since we can't see $y
In the $no_outer case the debugger can't see the grandparent scopes
lexicals, since the chain to the outer scopes is broken.
The fix in GH #21489 added tests for eval in package DB and reverted
v5.17.1-26-ga0d2bbd5c4, so both cases above produce the same results,
as they did prior in 5.16 and earlier.
The problem remaining:
Unfortunately this means that for the non-eval cases the lifetime of
the outer sub is extended, and in the case of #21524 [2], this results
in a reference loop in t/race_asyncs.t and some other Promise::ES6
tests.
It happens that these tests check the lifetime of the objects it
creates and so the tests fail, but if other code on CPAN or private
code uses similar constructs, they may silently[3] and continually
accumulate unreleased objects.
I can easily provide a fix specific to Promise::ES6 by ensuring the
subs don't close over so much (or anything), but this reduces the
locality of reference of the code and makes it harder to read.
It also doesn't fix any other potential code silently accumulating
objects.
Potential fixes:
In a comment in #21489 [4] Dave suggested using something like the
existing CvWEAKOUTSIDE() flag to make the outside links weak,
I don't think we can use CvWEAKOUTSIDE() as it is, since (I think)
that depends on the outside specifically holding the inner CV in some
way, but let's say we implement something similar, possibly just using
a weak reference.
If we make the weak outside unconditional, we'll have the possibility
of a `DB::eval` implementation failing to find outer lexicals where it
could find them before 386907f (the revert from #21489) if the outside
is released before the inside CV.
If we do re-implement CvHASEVAL() we have some of the same problem as
before 386907f: eval in the closure controls the lifetime of the
outside sub. This is compatible with pre-386907f but I think it is a
wart, since adding a C<eval "..."> may make your code start leaking.
The problem I see with leaving it as with 386907f is we have a silent
change in behaviour unless the user is specifically testing for it,
which Promise::ES6 does.
I don't know what to do here.
Tony
[1] https://github.com/Perl/perl5/pull/21489
[2] https://github.com/Perl/perl5/issues/21524
[3] if we still used spinning rust in general we'd hear the swapping
from the leaked objects eventually.
[4] https://github.com/Perl/perl5/pull/21489#issuecomment-1725443594