Skip Menu |

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

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

People
Owner: Nobody in particular
Requestors: corion [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: 0.10
Fixed in: 0.11



Subject: Perl crash when using Future::AsyncAwait
Hello again, I'm finally trying out Future::AsyncAwait and I really like the primitives it supplies! However, I think I encountered a bug somewhere in Future::AsyncAwait, since Perl crashes for the attached program, while the (trivially modified) non-async/await version passes as expected. I have tested the program on Windows+Perl 5.26 and the Debian stock Perl (5.24), with the latest IO::Async, Future and Future::AsyncAwait and get different but consistent indicators of memory corruption in both cases. I've also tested the program with AnyEvent::Future and IO::Async but have only attached the IO::Async version, since I assume that you are more familiar with that and I don't think that the different backends are relevant. I suspect some problem in the parsing or stack/pad handling, since using a global variable instead of a lexical variable in global scope makes a difference in behaviour (but not final outcome, i.e. crash). But my debugging-fu is too weak to actually pinpoint where the problem would be. If you need any assistance in debugging, or more/different test runs, please tell me! Thanks for bringing the future to Perl, -max
Subject: await-lexical.t
#!perl -w use strict; use Test::More tests => 2; use Future; use Future::AsyncAwait; # We want to use/force the IO::Async backend for now # The same crash happens with Future::AnyEvent use IO::Async::Future; use IO::Async::Loop; use Data::Dumper; # This one won't work with a lexical variable(?!) # Most likely a bug in Future::AsyncAwait stack handling/pad handling?! my $limiter = 1; # This works/crashes differently: #our $limiter = 1; my $loop = IO::Async::Loop->new; my @results; async sub limit_test { my( $j ) = @_; warn "No more limiter for $j" unless $limiter; push @results, [$j, $limiter]; my $future = $loop->new_future; $loop->watch_time( after => 3, code => sub { $future->done( "Done" ) } ); await $future; # AnyEvent::Future->new_delay(after => 1); Future->done( $j ); }; Future->wait_all( limit_test( 1 ), limit_test( 2 ), )->get(); is 0+@results, 2, "We got two calls"; is_deeply \@results, [ [1,1], [2,1] ], 'The $limiter variable kept its value';
On Sat Jan 06 13:58:36 2018, CORION wrote: Show quoted text
> I'm finally trying out Future::AsyncAwait and I really like the > primitives it supplies! However, I think I encountered a bug somewhere > in Future::AsyncAwait, since Perl crashes for the attached program, > while the (trivially modified) non-async/await version passes as > expected.
Well it's certainly reproducable: $ perl -Mblib await-lexical.t 1..2 No more limiter for 2 at await-lexical.t line 26. Segmentation fault I expect gdb and/or valgrind will have something to say on the matter... -- Paul Evans
Ah now this is curious. I've cut it down to a more minimised test case (avoiding any of the IO::Async-related parts) which fails if it has two calls outstanding, but doesn't crash if just one is performed. (though the 'is' tests later fail, but that's expected). -- Paul Evans
Subject: 90rt124026.t
use strict; use warnings; use Test::More; use Future; use Future::AsyncAwait; my $limiter = 1; my @pending; my @results; async sub limit_test { my ( $j ) = @_; push @results, [ $j, $limiter ]; push @pending, my $future = Future->new; await $future; return $j; } my $f = Future->wait_all( limit_test( 1 ), limit_test( 2 ), ## does not crash if this line is removed ); ( shift @pending )->done while @pending; is 0+@results, 2, "We got two calls"; is_deeply \@results, [ [1,1], [2,1] ], 'The $limiter variable kept its value'; done_testing();
Now partly fixed. At least, it passes for its extended unit test, and my earlier-posted one, but yours still crashes. However, if you make one small edit to your test, then it now passes. Replace: my $future = $loop->new_future; $loop->watch_time( after => 3, code => sub { $future->done( "Done" ) } ); with my $future = $loop->delay_future( after => 3 ); I suspect the crash reason is due to that capture of $future in the inner sub. -- Paul Evans
Subject: rt124026.patch
=== modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2017-11-28 01:13:53 +0000 +++ lib/Future/AsyncAwait.xs 2018-01-07 14:19:52 +0000 @@ -385,8 +385,9 @@ for(i = 1; i <= pad_max; i++) { PADNAME *pname = (i <= padnames_max) ? padnames[i] : NULL; - /* Only the lexicals that have names; if there's no name then skip it. */ - if(!pname || !PadnameLEN(pname)) { + /* Only the lexicals that have names; if there's no name then skip it. + * Also skip the outer lexical captures*/ + if(!pname || !PadnameLEN(pname) || PadnameOUTER(pname)) { state->padslots[i-1] = NULL; continue; } === modified file 't/10pad.t' --- t/10pad.t 2017-08-13 17:11:20 +0000 +++ t/10pad.t 2018-01-07 14:19:52 +0000 @@ -48,19 +48,29 @@ } # outside +# Make sure to test this twice because of pad lexical sharing - see RT124026 { my $capture = "outer"; my $closure = async sub { + $capture .= "X"; await $_[0]; return $capture; }; my $f1 = Future->new; - my $fret = $closure->( $f1 ); + my $f2 = Future->new; + my $fret = Future->needs_all( + $closure->( $f1 ), + $closure->( $f2 ), + ); $f1->done; - is( scalar $fret->get, "outer", '$fret now ready after done for closure' ); + $f2->done; + + is_deeply( [ $fret->get ], [ "outerXX", "outerXX" ], + '$fret now ready after done for closure' + ); } # captured variables of nested subs
Subject: Re: [rt.cpan.org #124026] Perl crash when using Future::AsyncAwait
Date: Sun, 7 Jan 2018 16:27:00 +0100
To: bug-Future-AsyncAwait [...] rt.cpan.org
From: Max Maischein <corion [...] cpan.org>
Hello Paul, thanks for the quick reaction! I can confirm that your changes make my actual implementation (soon on Github) pass instead of crash now. Show quoted text
> At least, it passes for its extended unit test, and my earlier-posted one, but yours still crashes. > > However, if you make one small edit to your test, then it now passes. Replace: > > my $future = $loop->new_future; > $loop->watch_time( after => 3, code => sub { $future->done( "Done" ) } ); > > with > > my $future = $loop->delay_future( after => 3 ); > > I suspect the crash reason is due to that capture of $future in the inner sub. >
Hmm - yes, that sounds plausible... It seems that my original use case doesn't have this problem, but maybe it is just better hidden there... Have a nice Sunday, -max
Ahah. Further subtle complications with pad slots named '&' which honestly, my work on Devel::MAT should have made me think about. But anyhow, here is a patch that makes your original program pass. -- Paul Evans
Subject: rt124026.patch
=== modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2018-01-07 14:19:52 +0000 +++ lib/Future/AsyncAwait.xs 2018-01-07 15:49:25 +0000 @@ -281,6 +281,24 @@ croak("TODO: handle cx->blk_oldsaveix"); } +static bool padname_is_normal_lexical(PADNAME *pname) +{ + /* PAD slots without names are certainly not lexicals */ + if(!pname || !PadnameLEN(pname)) + return FALSE; + + /* Outer lexical captures are not lexicals */ + if(PadnameOUTER(pname)) + return FALSE; + + /* Protosubs for closures are not lexicals */ + if(PadnamePV(pname)[0] == '&') + return FALSE; + + /* anything left is a normal lexical */ + return TRUE; +} + #define suspendedstate_suspend(state, cv) MY_suspendedstate_suspend(aTHX_ state, cv) static void MY_suspendedstate_suspend(pTHX_ SuspendedState *state, CV *cv) { @@ -385,9 +403,7 @@ for(i = 1; i <= pad_max; i++) { PADNAME *pname = (i <= padnames_max) ? padnames[i] : NULL; - /* Only the lexicals that have names; if there's no name then skip it. - * Also skip the outer lexical captures*/ - if(!pname || !PadnameLEN(pname) || PadnameOUTER(pname)) { + if(!padname_is_normal_lexical(pname)) { state->padslots[i-1] = NULL; continue; } === added file 't/12closure.t' --- t/12closure.t 1970-01-01 00:00:00 +0000 +++ t/12closure.t 2018-01-07 15:04:33 +0000 @@ -0,0 +1,54 @@ +#!/usr/bin/perl + +use strict; +use warnings; + +use Test::More; + +use Future; + +use Future::AsyncAwait; + +# creating a closure inside an async sub + +# before await +{ + my $f1 = Future->new; + my $sub; + async sub closure_before + { + my $x; # just to create a real closure + $sub = sub { $x++; 123 }; + + await $f1; + } + + my $f = closure_before(); + + $f1->done( 45 ); + is( $f->get, 45, 'result of async sub' ); + is( $sub->(), 123, 'result of closure before' ); +} + +# after await +{ + my $f1 = Future->new; + my $sub; + async sub closure_after + { + my $ret = await $f1; + + my $x; # just to create a real closure + $sub = sub { $x++; 123 }; + + return $ret; + } + + my $f = closure_after(); + + $f1->done( 45 ); + is( $f->get, 45, 'result of async sub' ); + is( $sub->(), 123, 'result of closure after' ); +} + +done_testing;
Subject: Re: [rt.cpan.org #124026] Perl crash when using Future::AsyncAwait
Date: Sun, 7 Jan 2018 17:38:45 +0100
To: bug-Future-AsyncAwait [...] rt.cpan.org
From: Max Maischein <corion [...] cpan.org>
Am 07.01.2018 um 16:56 schrieb Paul Evans via RT: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=124026 > > > Ahah. Further subtle complications with pad slots named '&' which honestly, my work on Devel::MAT should have made me think about. But anyhow, here is a patch that makes your original program pass. >
Yep, that patch works for me on Windows too! Thank you very much for the quick solution! -max
Was released in 0.11 -- Paul Evans