__SUB__ references wrong callback, we must use outer callback

[Sadrak]

Summary

__SUB__ references the wrong sub

Motivation

copy & paste from example was not working

[jhthorsen]

Pretty sure that introduces a circular reference, so 👎 from me.

[Sadrak]

Pretty sure that introduces a circular reference, so from me.

No, this is a common workflow for working with callbacks calling itself. No circular reference.

[Sadrak]

No circular reference.

OK, i misunderstood your plea.

There is one circular reference. Variable $fetch itself used in callback, produce one circular reference, but not for every call.

[kraih]

Always squash your commits.

[Sadrak]

Always squash your commits.

I am not familiar with github, i thought this will happen on accepting MR.

[Sadrak]

ping?

I guess i changed everything required, except the squash, how can i do this in a running MR?

[marcusramberg]

ping?

I guess i changed everything required, except the squash, how can i do this in a running MR?

Interactive rebase and force push.

[Sadrak]

ping?
I guess i changed everything required, except the squash, how can i do this in a running MR?

Interactive rebase and force push.

nice & easy, thanks for the hint!

done

[Sadrak]

@jhthorsen did i get an approval now? 🙏

[kraih]

Inconsistent comment format.

[Sadrak]

done?

[kraih]

How is this cleaner than my $fetch; $fetch = sub {?

[Sadrak]

ok, i am lost.

I simple wanted to fix a bug in the documentation. My initial fix was not accepted, because of a newly introduced memleak.

The optimized non-memleak variant has now the problem it looks to similar to the memleak-variant?

Can someone please help me and tell me what exactly is required to fix this bug? I wanted a minimalistic change not a complete rewrite of the example.

Next try would be to rename the inner $fetch to something else, can someone please suggest a name, i am tired of trying to help ...

[kraih]

Then just don't use __SUB__ in an example where it serves no purpose.

[kraih]

If you don't want to figure out the correct solution, please open an issue instead of a PR in the future.

[Sadrak]

Then just don't use __SUB__ in an example where it serves no purpose.

@jhthorsen can you please confirm the difference between my $fetch ; $fetch = sub { ... and my $fetch = sub { ... my $fetch = __SUB__; ...

In my understanding the later (current MR) will not leak a reference after the $promise->wait;

[kathackeray]

@Sadrak

can you please confirm the difference between my $fetch ; $fetch = sub { ... and my $fetch = sub { ... my $fetch = __SUB__; ...

If I understand correctly, they are looking for:

my $fetch; $fetch = sub {

  # Stop if there are no more URLs
  return unless my $url = shift @urls;

  # Fetch the next title
  $ua->get($url => sub ($ua, $tx) {
    print "$url: ", $tx->result->dom->at('title')->text, "\n";

    # Next request
    $fetch->();
    $promise->resolve if --$count == 0;
  });
  $count++;
};

$fetch must be declared first, and then assigned to, so that it is available within the function. If it was assigned during the declaration my $fetch = sub { ... then $fetch would not be available within the body of the function.

[Sadrak]

pretty sure this solution get a downvote from @jhthorsen (that was my first try).

Pretty sure that introduces a circular reference, so -1 from me.

And he is absolut right.

So to resume: The documentation is broken. I tried to figure this out, but can't get a proper/accepted solution. Next step is what? I guess closing and ignoring? :-/ That hurt in my developer heart ...

[kraih]

I lost track of the history of this discussion and have no idea what @jhthorsen was referring to.

[Sadrak]

using my $fetch ; $fetch = sub { ...; $fetch->(); ...; } will leak one reference. $fetch won't cleaned up after the scope due to a circular reference.

Alternatively before calling $promise->resolve we have to call undef $fetch manually to break the circular reference.

Or use the current MR with my $fetch = __SUB__ as a solution to this problem.

[kathackeray]

In my testing so far, my $fetch; $fetch = is flagged as circular by Devel::Cycle. But then so is $ua. Declaring my $fetch = __SUB__ within the sub does remove that circular reference, but $ua remains unless it is passed in as an argument, e.g.

  my $fetch = sub ($ua) {
    return unless my $url = shift @urls;

    my $fetch = __SUB__;
    printf "refcount: %s\n", refcount($fetch);

    $ua->get(
      $url => sub ($ua, $tx) {
        print "$url: ", $tx->result->json->{result}, "\n";

        $fetch->($ua);
        $promise->resolve if --$count == 0;
      }
    );

    $count++;
  };

  $fetch->($ua) for 1 .. 5;
  $promise->wait;

I'm still testing for alternatives, and also to understand why the above example finishes with a high refcount, whereas the version with my $fetch; $fetch = finishes with a refcount of 1.

[Sadrak]

ping?

[Sadrak]

ping?

[kathackeray]

@Sadrak, I've toyed with some alternative approaches that avoid storing the __SUB__ for nested scopes altogether.

This example uses a single function for both the "worker" and the callback.

my $fetch = sub ($ua, $tx = undef) {
  if ($tx) {    # $tx exists if sub is responding to a callback
    say $tx->req->url;
    __SUB__->($ua);
    $promise->resolve if --$count == 0;
  }

  return unless my $url = shift @urls;

  $ua->get($url => __SUB__);

  $count++;
};

$fetch->($ua) for 1 .. 8;
$promise->wait;

The next example does away with the explicitly managed promise, and instead piggybacks on the get_p functionality.

sub ($tx = undef) {
  say $tx->req->url if $tx;    # $tx exists if responding as a callback

  return unless my $url = shift @urls;
  $ua->get_p($url)->then(__SUB__);
  }
  ->() for 1 .. 8;

Mojo::IOLoop->start unless Mojo::IOLoop->is_running;

I wonder whether one of these would sit better with the reviewers.

[Sadrak]

I was thinking about the new approaches for some time and they are cool, but this is a documentation and this rise the complexity in my eyes a lot. The first one with some additional comments could be ok, the second one is dope.

[kathackeray]

Yea, I do agree they're a little more difficult to follow, especially without comments.

The sticking point for this issue passing review seems to be the assignment:

• @jhthorsen vetoed my fetch; $fetch = sub { on account of a circular reference
• @kraih seemed to veto my $fetch = __SUB__; on account of it being unnecessary (preferring the approach @jhthorsen vetoed)

In testing I've been unable to isolate tangible problems with either approach. But as the new approaches I've put forward avoid assignment of the sub altogether, I figured we might persuade the reviewers to let one of those pass (heavily commented), if not one of your originals.

I agree with you that it is important to have a correct example in the docs for this. Especially as it has been tricky to settle on the correct approach even amongst the maintainers.

[kraih]

It's unfortunate this has not progressed. And now the CI is broken. Would have liked to have it in the next release.