Skip Menu |
 

This queue is for tickets about the Future-AsyncAwait CPAN distribution.

Report information
The Basics
Id: 129896
Status: resolved
Priority: 0/
Queue: Future-AsyncAwait

People
Owner: Nobody in particular
Requestors: leonerd-cpan [...] leonerd.org.uk
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: 0.29



Subject: Memory leak when `local $@` is abandoned
Download (untitled) / with headers
text/plain 353b
While investigating TODO: free saved slot type 43 I fixed that immediate complaint, but still it memory leaks: not ok 2 - abandoned async sub does not grow memory # Failed test 'abandoned async sub does not grow memory' # at t/81memory-growth.t line 44. # Lost 135168 bytes of memory over 10000 calls, average of 13.52 per call -- Paul Evans
Download (untitled) / with headers
text/plain 1.1k
Current investigation is on use Devel::MAT::Dumper; Devel::MAT::Dumper::dump("42-prior.pmat"); foreach(1..10000) { my $f1 = Future->new; my $fret = (async sub { local $@; await $f1 })->(); undef $fret; undef $f1; } Devel::MAT::Dumper::dump("42-subsequent.pmat"); which appears to be leaking UNDEF scalars: $ pmat-counts -S *.pmat 42-prior.pmat Kind Count (blessed) Bytes (blessed) ... SCALAR 18660 798.9 KiB ... UNDEF() 7938 186.1 KiB ------------------ ----- --------- --------- --------- (total) 27822 28 2.4 MiB 6.4 KiB 42-subsequent.pmat Kind Count (blessed) Bytes (blessed) ... SCALAR 28660(+10000) 1.0 MiB(+234.4 KiB) ... UNDEF() 17938(+10000) 420.4 KiB(+234.4 KiB) ------------------ ------------- --------- --------------------- --------- (total) 37822(+10000) 28 2.7 MiB(+234.4 KiB) 6.4 KiB -- Paul Evans
Download (untitled) / with headers
text/plain 955b
Ahah. Partial progress. Have to SvREFCNT_dec() both the saved and cur values === modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2019-06-24 15:12:03 +0000 +++ lib/Future/AsyncAwait.xs 2019-06-24 15:31:32 +0000 @@ -422,6 +422,7 @@ case SAVEt_SV: SvREFCNT_dec(saved->saved.sv); + SvREFCNT_dec(saved->cur.sv); break; case SAVEt_PADSV_AND_MORTALIZE: However, there's still a runaway SvREFCNT on $main::@ ... Saving saved->saved.sv was 0x55e45eb6c230 (SvREFCNT=10000) Freeing saved->saved.sv was 0x55e45eb6c230 (SvREFCNT=10000) Saving saved->saved.sv was 0x55e45eb6c230 (SvREFCNT=10001) Freeing saved->saved.sv was 0x55e45eb6c230 (SvREFCNT=10001) Saving saved->saved.sv was 0x55e45eb6c230 (SvREFCNT=10002) Freeing saved->saved.sv was 0x55e45eb6c230 (SvREFCNT=10002) t/42unresolved.t .. ok All tests successful. Investigation continues -- Paul Evans
Download (untitled) / with headers
text/plain 279b
Latest code doesn't leak nor exhibit runaway refcount on $@ itself (the ERRSV). But it still increases refcount on PL_errgv (i.e. the *@ glob), but only in the case where an inner eval{} block dies. If it returns or is abandoned, it works fine. This is most odd. -- Paul Evans
Download (untitled) / with headers
text/plain 365b
Weird. SvREFCNT(PL_errgv) increases by 1 the first time one of the tests is run, but is stable afterwards. Unsure quite what this is but I can imagine it being some sort of lazy creation of something somewhere. Easy fix is to run the test twice but only check refcount on the second run. This seems to work. Overall then I think this is now fixed. -- Paul Evans
Subject: rt129896.patch
Download rt129896.patch
text/x-diff 7.4k
=== modified file 'Build.PL' --- Build.PL 2019-01-12 20:06:45 +0000 +++ Build.PL 2019-06-25 19:35:33 +0000 @@ -27,6 +27,7 @@ }, test_requires => { 'Test::More' => '0.88', # done_testing + 'Test::Refcount' => '0.09', }, configure_requires => { 'Module::Build' => '0.4004', # test_requires === modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2019-06-17 15:58:33 +0000 +++ lib/Future/AsyncAwait.xs 2019-06-25 22:35:37 +0000 @@ -279,16 +279,54 @@ SuspendedFrame *frame; for(frame = state->frames; frame; frame = frame->next) { - if(frame->stacklen) { - int i; - for(i = 0; i < frame->stacklen; i++) - ret += DMD_ANNOTATE_SV(sv, frame->stack[i], "a suspended stack temporary"); - } - - if(frame->mortallen) { - int i; - for(i = 0; i < frame->mortallen; i++) - ret += DMD_ANNOTATE_SV(sv, frame->mortals[i], "a suspended mortal"); + int i; + + for(i = 0; i < frame->stacklen; i++) + ret += DMD_ANNOTATE_SV(sv, frame->stack[i], "a suspended stack temporary"); + + for(i = 0; i < frame->mortallen; i++) + ret += DMD_ANNOTATE_SV(sv, frame->mortals[i], "a suspended mortal"); + + for(i = 0; i < frame->savedlen; i++) { + struct Saved *saved = &frame->saved[i]; + switch(saved->type) { +#ifdef SAVEt_CLEARPADRANGE + case SAVEt_CLEARPADRANGE: +#endif + case SAVEt_CLEARSV: + case SAVEt_INT_SMALL: + case SAVEt_DESTRUCTOR_X: +#ifdef SAVEt_STRLEN + case SAVEt_STRLEN: +#endif + case SAVEt_SET_SVFLAGS: + /* Nothing interesting */ + break; + + case SAVEt_FREEPV: + /* This is interesting but a plain char* pointer so there's nothing + * we can do with it in Devel::MAT */ + break; + + case SAVEt_COMPPAD: + ret += DMD_ANNOTATE_SV(sv, saved->cur.ptr, "a suspended SAVEt_COMPPAD"); + break; + + case SAVEt_FREESV: + ret += DMD_ANNOTATE_SV(sv, saved->saved.sv, "a suspended SAVEt_FREESV"); + break; + + case SAVEt_SV: + ret += DMD_ANNOTATE_SV(sv, (SV *)saved->u.gv, "a suspended SAVEt_SV target GV"); + ret += DMD_ANNOTATE_SV(sv, saved->cur.sv, "a suspended SAVEt_SV current value"); + ret += DMD_ANNOTATE_SV(sv, saved->saved.sv, "a suspended SAVEt_SV saved value"); + break; + + case SAVEt_PADSV_AND_MORTALIZE: + ret += DMD_ANNOTATE_SV(sv, saved->cur.sv, "a suspended SAVEt_PADSV_AND_MORTALIZE current value"); + ret += DMD_ANNOTATE_SV(sv, saved->saved.sv, "a suspended SAVEt_PADSV_AND_MORTALIZE saved value"); + break; + } } } @@ -383,6 +421,12 @@ Safefree(saved->cur.ptr); break; + case SAVEt_SV: + SvREFCNT_dec(saved->u.gv); + SvREFCNT_dec(saved->saved.sv); + SvREFCNT_dec(saved->cur.sv); + break; + case SAVEt_PADSV_AND_MORTALIZE: SvREFCNT_dec(saved->cur.sv); break; @@ -635,7 +679,7 @@ saved->saved.sv = val; /* steal ownership */ /* restore it for now */ - GvSV(gv) = SvREFCNT_inc(val); + GvSV(gv) = val; break; } @@ -1226,7 +1270,7 @@ break; case SAVEt_SV: - save_pushptrptr(saved->u.gv, saved->saved.sv, SAVEt_SV); + save_pushptrptr(saved->u.gv, SvREFCNT_inc(saved->saved.sv), SAVEt_SV); SvREFCNT_dec(GvSV(saved->u.gv)); GvSV(saved->u.gv) = saved->cur.sv; === added file 't/15local-errsv.t' --- t/15local-errsv.t 1970-01-01 00:00:00 +0000 +++ t/15local-errsv.t 2019-06-25 22:35:06 +0000 @@ -0,0 +1,67 @@ +#!/usr/bin/perl + +use strict; +use warnings; + +use Test::More; +use Test::Refcount 0.09 import => [qw( is_refcount refcount )]; + +use Future; + +use Future::AsyncAwait; + +my $orig_cxstack_ix = Future::AsyncAwait::__cxstack_ix; + +my $errgv_ref = \*@; + +# $@ can be localized +# For some odd reason, SvREFCNT(PL_errgv) increases by 1 the first time +# this code is run, but is stable thereafter. So only test it the second +# time. Perhaps something somewhere needs to get lazily created? +foreach my $idx ( 1, 2 ) +{ + my $errsv_refcount = refcount(\$@); + my $errgv_refcount = refcount($errgv_ref); + + my $f1 = Future->new; + my $fret = (async sub { + local $@ = "inside"; + await $f1; + return $@; + })->(); + + $@ = "OUTSIDE"; + $f1->done; + is( scalar $fret->get, "inside", 'result from async sub with local $@' ); + + is_refcount( \$@, $errsv_refcount, '$@ refcount preserved' ); + is_refcount( $errgv_ref, $errgv_refcount, '*@ refcount preserved' ) if $idx > 1; +} + +# localised $@ plays nicely with eval{} +{ + my $errsv_refcount = refcount(\$@); + my $errgv_refcount = refcount($errgv_ref); + + my $f1 = Future->new; + my $fret = (async sub { + local $@ = "inside"; + await $f1; + eval { + die "oopsie\n"; + }; + return $@; + })->(); + + $@ = "OUTSIDE"; + $f1->done; + is( scalar $fret->get, "oopsie\n", 'result from eval { die }' ); + + is_refcount( \$@, $errsv_refcount, '$@ refcount preserved' ); + is_refcount( $errgv_ref, $errgv_refcount, '*@ refcount preserved' ); +} + +is( Future::AsyncAwait::__cxstack_ix, $orig_cxstack_ix, + 'cxstack_ix did not grow during the test' ); + +done_testing; === modified file 't/42unresolved.t' --- t/42unresolved.t 2019-04-26 12:55:27 +0000 +++ t/42unresolved.t 2019-06-25 22:05:51 +0000 @@ -4,6 +4,7 @@ use warnings; use Test::More; +use Test::Refcount 0.09 import => [qw( is_refcount refcount )]; use Future; @@ -12,6 +13,8 @@ my $orig_cxstack_ix = Future::AsyncAwait::__cxstack_ix; my $file = quotemeta __FILE__; +my $errgv_ref = \*@; + async sub identity { return await $_[0]; @@ -78,6 +81,22 @@ pass( "abandoned foreach loop does not crash" ); } +# abandoned local $@ +{ + my $errsv_refcount = refcount(\$@); + my $errgv_refcount = refcount($errgv_ref); + + my $f1 = Future->new; + my $fret = (async sub { local $@; await $f1 })->(); + + undef $fret; + undef $f1; + pass( "abandoned local \$@ does not crash" ); + + is_refcount( \$@, $errsv_refcount, '$@ refcount preserved' ); + is_refcount( $errgv_ref, $errgv_refcount, '*@ refcount preserved' ); +} + is( Future::AsyncAwait::__cxstack_ix, $orig_cxstack_ix, 'cxstack_ix did not grow during the test' ); === modified file 't/81memory-growth.t' --- t/81memory-growth.t 2019-02-02 17:19:16 +0000 +++ t/81memory-growth.t 2019-06-24 15:12:20 +0000 @@ -32,4 +32,16 @@ calls => 10000, 'async/await does not grow memory'; +sub abandoned +{ + my $f1 = Future->new; + my $fret = (async sub { local $@; await $f1 })->(); + undef $fret; + undef $f1; +} + +no_growth \&abandoned, + calls => 10000, + 'abandoned async sub does not grow memory'; + done_testing; === added file 't/82devel-mat-dumper-helper.t' --- t/82devel-mat-dumper-helper.t 1970-01-01 00:00:00 +0000 +++ t/82devel-mat-dumper-helper.t 2019-06-24 15:10:14 +0000 @@ -0,0 +1,30 @@ +#!/usr/bin/perl + +use strict; +use warnings; + +use Test::More; +BEGIN { + eval { require Devel::MAT; } or + plan skip_all => "No Devel::MAT"; + + require Devel::MAT::Dumper; +} + +use Future; + +use Future::AsyncAwait; + +my $f1 = Future->new; +my $fret = (async sub { local $@; await $f1 })->(); + +( my $file = __FILE__ ) =~ s/\.t$/.pmat/; +Devel::MAT::Dumper::dump( $file ); +END { unlink $file if -f $file } + +$f1->done; +$fret->get; + +pass( "did not crash" ); + +done_testing;
Resolved by 0.29 -- Paul Evans


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.