mojo
mojo copied to clipboard
mojolicious
__SUB__ references wrong callback, we must use outer callback
Summary
__SUB__ references the wrong sub
Motivation
copy & paste from example was not working
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.
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.
Always squash your commits.
I am not familiar with github, i thought this will happen on accepting MR.
ping?
I guess i changed everything required, except the squash, how can i do this in a running MR?
ping?
I guess i changed everything required, except the squash, how can i do this in a running MR?
Interactive rebase and force push.
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, 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.
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.
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.
It's unfortunate this has not progressed. And now the CI is broken. Would have liked to have it in the next release.