Test suite fails with perl 5.39.4+

[eserte]

See subject, and see http://matrix.cpantesters.org/?dist=Promise-ES6+0.28 for an overview.
Feel free to open a BBC if you think that bleadperl does not behave correctly here.

[FGasper]

@eserte It looks like something changed (broke?) handling of DESTROY.

I may not have the bandwidth to diagnose the issue myself, unfortunately.

[eserte]

Ah, it's already known: Perl/perl5#21524

[tonycoz]

The issue is that a change around 5.18 broke the documented behaviour of eval EXPR in package DB by not maintaining the OUTSIDE links for each closure.

This means that with the way the failing Promise::ES6 test works you can end up with a reference loop between the closures and (I think) the @resolves array.

This can be avoided in the test by not making the outer scopes visible, for example

--- a/t/race_async.t
+++ b/t/race_async.t
@@ -13,6 +13,32 @@ use PromiseTest;
 
 use Promise::ES6;
 
+sub _bind {
+    my ($sub, @args) = @_;
+
+    return sub {
+        $sub->(@args, @_);
+    };
+}
+
+sub ready1 {
+    my ($eventer, $resolve) = @_;
+
+    if ($eventer->has_happened('ready1') && !$eventer->has_happened('resolved1')) {
+        $resolve->(1);
+        $eventer->happen('resolved1');
+    }
+}
+
+sub ready2 {
+    my ($eventer, $resolve) = @_;
+
+    if ($eventer->has_happened('ready2') && !$eventer->has_happened('resolved2')) {
+        $resolve->(2);
+        $eventer->happen('resolved2');
+    }
+}
+
 {
     my $eventer = Eventer->new();

though there may be prettier fixes.

I asked on the list to see if anyone else had any ideas but didn't receive any solutions.

[akarelas]

Tests don't pass on perl v5.40.0 either.

[FGasper]

I haven't taken the time to dig into this and am unlikely to do so. (Too many other priorities, sorry.)

I could:

• just skip this test in 5.39+. (Easy but undesirable long-term.)
• wait for a PR that fixes the problem, assumedly using @tonycoz's suggested approach.

Any other ideas?

[tonycoz]

I've made PRs #15 and #16 with possible solutions.

[akarelas]

@tonycoz I noticed all your changes were in the test scripts. Does that mean that I should install and use Promise::ES6 on perl v5.40 if I need it, skipping tests, and it should work perfectly?

[FGasper]

@tonycoz Thank you for your PRs.

I hesitate, though, to merge them because they seem to solve the problem in ways that no pure-Perl programmer (i.e., someone who doesn’t grok Perl internals) would reasonably understand. Are the “OUTSIDE links for each closure” an implementation detail of Perl?

Is that correct? If so, would a fix to the interpreter be possible so that pure-Perl code won’t encounter this problem?

[tonycoz]

I hesitate, though, to merge them because they seem to solve the problem in ways that no pure-Perl programmer (i.e., someone who doesn’t grok Perl internals) would reasonably understand. Are the “OUTSIDE links for each closure” an implementation detail of Perl?

Is that correct? If so, would a fix to the interpreter be possible so that pure-Perl code won’t encounter this problem?

The problem is a reference loop.

5.18 broke the reference loop allowing your test to pass, but broke the documented eval "" in package DB behaviour that the debugger depends on, by removing the reference held by subs containing no normal eval on their containing scope.

5.39.4 fixed that documented behaviour, but that means all subs, not just those with evals, need to keep a reference to their containing scope.

This meant that the reference loop which your test documented as fixed in 5.18 was reinstated in 5.39.4.

I spent a lot of time trying to find a solution, including asking on the p5p list, but didn't receive any feedback with a solution.

So I went with the fix to the documented behaviour.

[FGasper]

This makes sense; thank you for clarifying.For posterity, though: where is it documented that subs retain a reference to their containing scope?-FGOn Aug 19, 2024, at 01:23, Tony Cook ***@***.***> wrote: I hesitate, though, to merge them because they seem to solve the problem in ways that no pure-Perl programmer (i.e., someone who doesn’t grok Perl internals) would reasonably understand. Are the “OUTSIDE links for each closure” an implementation detail of Perl? Is that correct? If so, would a fix to the interpreter be possible so that pure-Perl code won’t encounter this problem? The problem is a reference loop. 5.18 broke the reference loop allowing your test to pass, but broke the documented eval "" in package DB behaviour that the debugger depends on, by removing the reference held by subs containing no normal eval on their containing scope. 5.39.4 fixed that documented behaviour, but that means all subs, not just those with evals, need to keep a reference to their containing scope. This meant that the reference loop which your test documented as fixed in 5.18 was reinstated in 5.39.4. I spent a lot of time trying to find a solution, including asking on the p5p list, but didn't receive any feedback with a solution. So I went with the fix to the documented behaviour. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

[jkeenan]

This makes sense; thank you for clarifying. For posterity, though: where is it documented that subs retain a reference to their containing scope?

It appears that @FGasper is still waiting for an answer to a question that was garbled in the GH interface. @tonycoz, can you clarify? (I'd like to see if we can close Perl/perl5#21524.) Thanks.

[tonycoz]

The PSC said:

* We took another brief look at the closure memory leak which is now            
  <https://github.com/Perl/perl5/issues/22547> and resolved to prepare          
  to put down a consensus next time we meet.                                    

so I'm waiting on that.

[toddr]

@tonycoz any update?

[toddr]

It looks like discussions are still in progress to revert the change to perl. It still means this module is going to be broken on 5.40.0 but I'm trying the patch to see if it resolves this completely Perl/perl5#22635