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.

Sign up for GitHub

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

__SUB__ references wrong callback, we must use outer callback #1783

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

__SUB__ references wrong callback, we must use outer callback #1783

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/Mojolicious/Guides/Cookbook.pod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1413,12 +1413,15 @@ to process requests in smaller batches.
# Stop if there are no more URLs
return unless my $url = shift @urls;

# Save __SUB__ for later use in another callback
my $fetch = __SUB__;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

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