Skip Menu |
 

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

Report information
The Basics
Id: 129215
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:
  • 0.23
  • 0.25
Fixed in: 0.26



Subject: Erroneous foreach(LIST) items
Download (untitled) / with headers
text/plain 1012b
Example by Tom Molesworth: $ perl -v This is perl 5, version 28, subversion 1 (v5.28.1) built for x86_64-linux-gnu-thread-multi(with 61 registered patches, see perl -V for more detail) ... $ perl -Mblib -e'use Future::AsyncAwait; use IO::Async::Loop; my $loop = IO::Async::Loop->new; (async sub { for my $k (qw(one two three four)) { print "$k\n"; await $loop->delay_future(after => 0.01); print "$k\n" } })->()->get' one one 0 1 one one two two three three four four Bizarre copy of CODE in eval {block} exit at -e line 1. I don't think this is a repeat of RT128619 as it doesn't involve stack temporaries, all the values here are const. It also fails on pre-5.24 perls, in a somewhat similar way: $ perl5.22.2 -Mblib -e'use Future::AsyncAwait; use IO::Async::Loop; my $loop = IO::Async::Loop->new; (async sub { for my $k (qw(one two three four)) { print "$k\n"; await $loop->delay_future(after => 0.01); print "$k\n" } })->()->get' one one 0 1 1 1 one one two two three three four four -- Paul Evans
Download (untitled) / with headers
text/plain 674b
Most awkwardly, this appears to actually require IO::Async to break. I tried rewriting the example using just Future::IO, to make a smaller-dependency test case, but that passes: # RT#129215 SKIP: { skip "Future::IO not available", 1 unless eval { require Future::IO; }; my $out = ""; (async sub { foreach my $k (qw( one two three four )) { $out .= "$k\n"; await Future::IO->sleep(0.01); $out .= "$k\n"; } })->()->get; is( $out, "one\none\ntwo\ntwo\nthree\nthree\nfour\nfour\n", 'Output from sleepy foreach(LIST)' ); } But breaks as above if replaced with the $loop->delay_future() call. -- Paul Evans
Download (untitled) / with headers
text/plain 469b
This from perlguts might also be relevant: Most callers of "cx_pushblock" simply set the new args stack floor to the top of the previous stack frame, but for "CXt_LOOP_LIST" it stores the items being iterated over on the stack, and so sets "blk_oldsp" to the top of these items instead. Note that, contrary to its name, "blk_oldsp" doesn't always represent the value to restore "PL_stack_sp" to on scope exit. -- Paul Evans
Download (untitled) / with headers
text/plain 1.2k
I think there's still multiple bugs here. The one above might be one. Another is that I'm not accounting for the change in stack height between original and resumed functions. Almost every bit of perl context in the context stack stores either SV pointers directly, or operates implicitly relative to PL_stack_sp at the time, so is immune to minor stack height differences. Not so the fields in CXt_LOOP_LIST. These fields are all I32 values that store the _absolute_ stack index, i.e. the index of PL_stack_base[ix] at which items are found: cx->blk_oldsp == just beyond the list of items cx->blk_loop.state_u.stack.basesp == 1 before the first item in the list ((I've no idea why it's 1 before...)) cx->blk_loop.state_u.stack.ix == current iteration item When the `async sub` is first suspended, the stack includes the arguments it was called with, as well as the height of all its callers. When we resume, the stack height might be quite different. These absolute indices will need adjusting. At suspend time we'll have to measure them relative to some point we can ensure we put back in place later on. At that point of execution, I *think* PL_stack_sp should be stable enough that we can measure them backwards, down from the top. -- Paul Evans
Download (untitled) / with headers
text/plain 466b
Show quoted text
> At suspend time we'll have to measure them relative to some point we > can ensure we put back in place later on. At that point of execution, > I *think* PL_stack_sp should be stable enough that we can measure them > backwards, down from the top.
Attached patch does this. Tests now pass on 5.24+, but still fail on the previous versions of perl - which don't use CXt_LOOP_LIST. I suspect they'll have some other, analogous bug in their handling. -- Paul Evans
Subject: rt129215.patch
Download rt129215.patch
text/x-diff 3.5k
=== modified file 'lib/Future/AsyncAwait.xs' --- lib/Future/AsyncAwait.xs 2019-04-18 16:16:19 +0000 +++ lib/Future/AsyncAwait.xs 2019-04-18 16:22:03 +0000 @@ -126,6 +126,10 @@ } eval; struct block_loop loop; } el; + + /* for debugging purposes */ + SV *loop_list_first_item; + #ifdef HAVE_ITERVAR SV *itervar; #endif @@ -749,6 +753,25 @@ */ if(frame->stacklen || frame->marklen) panic("ARGH CXt_LOOP_LIST frame tried to steal stack values or marks"); + + /* The various fields in the context structure store absolute stack + * heights as offsets from PL_stack_base directly. When we get + * resumed the stack will probably not be the same absolute height + * at this point, so we'll have to store them relative to something + * fixed. + * We'll adjust them to be upside-down, counting -backwards- from + * the current stack height. + */ + I32 height = PL_stack_sp - PL_stack_base; + + if(cx->blk_oldsp != height) + panic("ARGH suspending CXt_LOOP_LIST frame with blk_oldsp != stack height"); + + /* First item is at [1] oddly, not [0] */ + frame->loop_list_first_item = PL_stack_base[cx->blk_loop.state_u.stack.basesp+1]; + + frame->el.loop.state_u.stack.basesp = height - frame->el.loop.state_u.stack.basesp; + frame->el.loop.state_u.stack.ix = height - frame->el.loop.state_u.stack.ix; break; #endif } @@ -1225,6 +1248,33 @@ Safefree(frame->mortals); frame->mortals = NULL; } + + switch(frame->type) { +#if HAVE_PERL_VERSION(5, 24, 0) + case CXt_LOOP_LIST: { + I32 height = PL_stack_sp - PL_stack_base; + + cx->blk_loop.state_u.stack.basesp = height - cx->blk_loop.state_u.stack.basesp; + cx->blk_loop.state_u.stack.ix = height - cx->blk_loop.state_u.stack.ix; + + /* For consistency; check that the first SV in the list is in the right + * place. If so we presume the others are + */ + if(PL_stack_base[cx->blk_loop.state_u.stack.basesp+1] == frame->loop_list_first_item) + break; + + /* First item is at [1] oddly, not [0] */ + fprintf(stderr, "F:AA: consistency check resume LOOP_LIST with first=%p:", + frame->loop_list_first_item); + debug_sv_summary(frame->loop_list_first_item); + fprintf(stderr, " stackitem=%p:", PL_stack_base[frame->el.loop.state_u.stack.basesp + 1]); + debug_sv_summary(PL_stack_base[frame->el.loop.state_u.stack.basesp]); + fprintf(stderr, "\n"); + panic("ARGH CXt_LOOP_LIST consistency check failed"); + break; + } +#endif + } } #define suspendedstate_resume(state, cv) MY_suspendedstate_resume(aTHX_ state, cv) === modified file 't/22context-foreach.t' --- t/22context-foreach.t 2018-01-20 14:09:54 +0000 +++ t/22context-foreach.t 2019-04-18 16:22:03 +0000 @@ -119,6 +119,24 @@ is( scalar $fret->get, "awaited twice", '$fret now ready after foreach with two awaits' ); } +# RT#129215 +{ + my @F = map { Future->new } 0, 1, 2; + my $fret = (async sub { + foreach my $idx ( 0, 1, 2 ) { + await $F[$idx]; + } + return "OK"; + })->(); + + # Arrange for the stack to be at different heights on each resume + my $tmp = do { 1 + ( $F[0]->done // 0 ) }; + $tmp = [ 2, 3, [ $F[1]->done ] ]; + $tmp = 4 * ( 6 + ( $F[2]->done // 0 ) ); + + is( scalar $fret->get, "OK", '$fret now ready after differing stack resumes' ); +} + # TODO: # This ought to be a compiletime check. That's hard right now so for now # it's a runtime check
This plus another fix for pre-5.24 was released in 0.24 -- Paul Evans
Download (untitled) / with headers
text/plain 186b
It's back. Turns out to be the cause of both https://rt.cpan.org/Ticket/Display.html?id=129319 https://rt.cpan.org/Ticket/Display.html?id=129320 in different ways. -- Paul Evans
Download (untitled) / with headers
text/plain 280b
On Fri Apr 26 10:02:33 2019, PEVANS wrote: Show quoted text
Those two are fixed, I think we can close this one again now. -- 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.