Bug #130957 for Future-AsyncAwait: Long-running async subs seem to accumulate on

Sun Nov 10 12:25:13 2019 TEAM [...] cpan.org - Ticket created

Subject:

Long-running async subs seem to accumulate on_cancel callbacks

Attached an example script - when suspending and resuming, Future instances are accumulated with a suspiciously large number of callbacks queued via ->on_cancel. This looks like an unfortunate side-effect of the cancellation handling from a few versions back?

Subject:

await-leak.pl

#!/usr/bin/env perl use strict; use warnings; use IO::Async::Loop; use Future::AsyncAwait; use IO::Async::Timer::Periodic; use Devel::MAT::Dumper; my $loop = IO::Async::Loop->new; my @pending; my $count = 0; $loop->add( my $timer = IO::Async::Timer::Periodic->new( interval => 0.0001, on_tick => sub { if($count > 1000) { Devel::MAT::Dumper::dump('out.pmat'); exit 0; } my @f = splice @pending; $_->done for @f; } ) ); $timer->start; (async sub { while(1) { ++$count; my $f = $loop->new_future; push @pending, $f; await $f; } })->()->get;

Sun Nov 10 12:32:54 2019 TEAM [...] cpan.org - Correspondence added

Turns out that this is quite easy to reproduce without IO::Async loaded - simpler example and potential test script attached.

Subject:

await-leak-plain.pl

#!/usr/bin/env perl use strict; use warnings; use Future::AsyncAwait; use Devel::MAT::Dumper; my @pending; my $count = 0; (async sub { while(1) { ++$count; my $f = Future->new; push @pending, $f; await $f; } })->()->retain; while(1) { my @f = splice @pending; $_->done for @f; if($count > 1000) { Devel::MAT::Dumper::dump('out.pmat'); exit 0; } }

Subject:

await-leak.t

#!/usr/bin/env perl use strict; use warnings; use Future::AsyncAwait; use Test::More; use Test::Refcount; use Scalar::Util qw(weaken); my @pending; my $count = 0; (async sub { while(1) { ++$count; my $f = Future->new; push @pending, $f; await $f; } })->()->retain; for(1..10) { my @f = splice @pending; for(@f) { $_->done; is_oneref($_); weaken($_); is($_, undef); } } done_testing;

Sun Nov 10 13:01:39 2019 TEAM [...] cpan.org - Correspondence added

Looks like the issue is that `$x->on_cancel($y)` is keeping a strong ref on `$y` - even when `$y` is marked as ready, it won't be a candidate for destruction until `$x` is marked as ready. Updating `on_cancel` in Future.pm to clear out referenced futures when they are marked as ready causes the test case to pass: sub on_cancel { my $self = shift; my ( $code ) = @_; my $is_future = blessed( $code ) && $code->isa( "Future" ); $is_future or _callable( $code ) or Carp::croak "Expected \$code to be callable or a Future in ->on_cancel"; $self->{ready} and return $self; if($is_future) { use Scalar::Util qw(weaken refaddr); use List::UtilsBy qw(extract_by); weaken(my $weak_self = $self); $code->on_ready(sub { my $self = $weak_self or return; my $f = shift; extract_by { refaddr($_) == refaddr($f) } @{$self->{on_cancel}}; }); } push @{ $self->{on_cancel} }, $code; return $self; }

Mon Nov 11 09:29:55 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Sun Nov 10 13:01:39 2019, TEAM wrote: Show quoted text

> Looks like the issue is that `$x->on_cancel($y)` is keeping a strong > ref on `$y` - even when `$y` is marked as ready, it won't be a > candidate for destruction until `$x` is marked as ready.

That sounds a good description of what's going on. Show quoted text

> Updating `on_cancel` in Future.pm to clear out referenced futures when > they are marked as ready causes the test case to pass:

I found a somewhat better(?) way to basically do that. Each Future now stores a (weakened) list of SCALAR refs to other places where it is referenced; scalars it should undef when it is marked as ready. The `on_cancel` method then stores a ref in the target $code future's list. This ensures that two pending futures form a strong ref in the cancellation chain, but once the latter one becomes resolved the former one can let go of it. -- Paul Evans

Subject:

rt130957.patch

=== modified file 'lib/Future.pm' --- lib/Future.pm 2019-06-13 13:41:38 +0000 +++ lib/Future.pm 2019-11-11 14:27:22 +0000 @@ -377,6 +377,8 @@ } delete $self->{on_cancel}; + $_ and undef $$_ for @{ $self->{undef_on_ready} }; + my $callbacks = delete $self->{callbacks} or return; my $cancelled = $self->{cancelled}; @@ -716,6 +718,10 @@ $self->{ready} and return $self; push @{ $self->{on_cancel} }, $code; + if( $is_future ) { + push @{ $code->{undef_on_ready} }, \$self->{on_cancel}[-1]; + weaken $code->{undef_on_ready}[-1]; + } return $self; } @@ -1037,6 +1043,7 @@ $self->{cancelled}++; foreach my $code ( reverse @{ $self->{on_cancel} || [] } ) { + defined $code or next; my $is_future = blessed( $code ) && $code->isa( "Future" ); $is_future ? $code->cancel : $code->( $self ); === modified file 't/02cancel.t' --- t/02cancel.t 2017-11-28 17:32:24 +0000 +++ t/02cancel.t 2019-11-11 14:27:22 +0000 @@ -84,8 +84,18 @@ { my $f1 = Future->new; my $f2 = Future->new; + my $f3 = Future->new; $f1->on_cancel( $f2 ); + $f1->on_cancel( $f3 ); + + is_oneref( $f1, '$f1 has refcount 1 after on_cancel chaining' ); + is_refcount( $f2, 2, '$f2 has refcount 2 after on_cancel chaining' ); + is_refcount( $f3, 2, '$f3 has refcount 2 after on_cancel chaining' ); + + $f3->done; + is_oneref( $f3, '$f3 has refcount 1 after done in cancel chain' ); + my $cancelled; $f2->on_cancel( sub { $cancelled++ } );

Mon Nov 11 09:29:56 2019 The RT System itself - Status changed from 'new' to 'open'

Mon Nov 11 09:30:05 2019 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'patched'

Tue Nov 12 06:22:25 2019 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Future 0.42 fixes this one. -- Paul Evans

Tue Nov 12 06:22:26 2019 leonerd-cpan [...] leonerd.org.uk - Status changed from 'patched' to 'resolved'

Bug #130957 for Future-AsyncAwait: Long-running async subs seem to accumulate on_cancel callbacks