Skip Menu |

This queue is for tickets about the IO-Async CPAN distribution.

Report information
The Basics
Id: 101774
Status: resolved
Priority: 0/
Queue: IO-Async

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

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



Subject: ->read_until / ->read_exactly / ->read_* Future is already done
Attached script raises an exception due to future being marked ->done after completion. More details to follow... cheers, Tom
Subject: read_until.t
use strict; use warnings; use Test::More; use Test::Fatal; use IO::Async::Loop; use IO::Async::Stream; my $loop = IO::Async::Loop->new; my $listener = $loop->listen( addr => { family => 'inet', socktype => 'stream', ip => '127.0.0.1', port => 0, }, on_stream => sub { my ($stream) = @_; $stream->configure( on_read => sub { my ($stream, $buf, $eof) = @_; $stream->debug_printf("Had line: %s", $1) while $$buf =~ s/^(.+)\x0D\x0A//; 0 } ); $loop->add($stream); $stream->write('lalalalalalalalala'); } )->get; my $port = $listener->read_handle->sockport; $loop->connect( addr => { family => 'inet', socktype => 'stream', ip => '127.0.0.1', port => $port, }, )->then(sub { my ($sock) = @_; my $stream = IO::Async::Stream->new( handle => $sock, on_read => sub { 0 } ); $loop->add($stream); Future->done($stream); })->then(sub { my ($stream) = @_; $stream->read_exactly(4)->then(sub { $stream->read_exactly(4) }) })->get
Confirmed, repeatable: $ PERL_FUTURE_DEBUG=1 perl -Mblib rt101774.pl IO::Async::Future=HASH(0x2872e58) is already done at /home/leo/src/perl/IO-Async/blib/lib/IO/Async/Stream.pm line 1139 and cannot be ->done at /home/leo/src/perl/IO-Async/blib/lib/IO/Async/Stream.pm line 1139. -- Paul Evans
Smaller test case -- Paul Evans
Subject: rt101774.pl
#!/usr/bin/perl use strict; use warnings; use IO::Async::Test; use Test::More; use IO::Async::Loop; use IO::Async::Stream; my $loop = IO::Async::Loop->new_builtin; testing_loop( $loop ); sub mkhandles { my ( $rd, $wr ) = IO::Async::OS->pipepair or die "Cannot pipe() - $!"; # Need handles in nonblocking mode $rd->blocking( 0 ); $wr->blocking( 0 ); return ( $rd, $wr ); } # RT101774 { my ( $rd, $wr ) = mkhandles; my $stream = IO::Async::Stream->new( read_handle => $rd, on_read => sub { 0 }, ); $loop->add( $stream ); $wr->syswrite( "lalaLALA" ); my $f = $stream->read_exactly( 4 )->then( sub { $stream->read_exactly( 4 ); }); wait_for { $f->is_ready }; is( scalar $f->get, "LALA", 'chained ->read_exactly' ); $loop->remove( $stream ); } done_testing;
Patch attached. While it solves the issue, I can't help thinking there's a conceptually-neater solution that doesn't involve this seemingly-arbitrary flag just to break the cycle in this way. Maybe we can find a neater way sometime. In the meantime, it does at least seem to work -- Paul Evans
Subject: rt101774.patch
=== modified file 'lib/IO/Async/Stream.pm' --- lib/IO/Async/Stream.pm 2014-10-17 16:54:33 +0000 +++ lib/IO/Async/Stream.pm 2015-01-27 22:59:07 +0000 @@ -897,6 +897,8 @@ my $self = shift; my ( $eof ) = @_; + local $self->{flushing_read} = 1; + my $readqueue = $self->{readqueue}; my $ret; @@ -1040,6 +1042,7 @@ push @{ $self->{readqueue} }, Reader( $on_read, $args{future} ); # TODO: Should this always defer? + return if $self->{flushing_read}; 1 while length $self->{readbuff} and $self->_flush_one_read( 0 ); }
Looks good to me. I much prefer this approach over a loop->later deferral. Offhand I can't think of any cases where it's valid to recurse into ->_flush_one_read, so I'd be tempted to move the flag check into that sub as well. Probably the same for _flush_one_write. As such, it doesn't seem that bad conceptually - same principle as never recursing into the event loop itself, I guess. Only concern I have is the possibility of something skipping the _flush_one_read because one was already active, then never actually getting around to the _flush_one_read. Basically, this should be fine: 1 while $self->_flush_one_read($eof); but there seems to be potential for breakage with: $self->_flush_one_read($eof); That makes me think that perhaps the '1 while ...' construct should be moved into a method, but the while condition varies - ->push_on_read checks for length $self->{readbuff} as well. Will revisit the fmap/read_until combo to see if this also fixes the concurrent ->read_until behaviour (I'm expecting that to Just Work with this patch applied). cheers, Tom On Tue Jan 27 23:00:43 2015, PEVANS wrote: Show quoted text
> Patch attached. > > While it solves the issue, I can't help thinking there's a > conceptually-neater solution that doesn't involve this seemingly- > arbitrary flag just to break the cycle in this way. Maybe we can find > a neater way sometime. > > In the meantime, it does at least seem to work
From: larodi [...] gmail.com
applying the patch results in the following warnings in IO::Async::Stream : Use of uninitialized value $len in numeric ge (>=) at /home/tsm/tsm/bin/../local/lib/perl5/IO/Async/Stream.pm line 1141. Use of uninitialized value $len in substr at /home/tsm/tsm/bin/../local/lib/perl5/IO/Async/Stream.pm line 1142. На ср яну 28 01:17:02 2015, TEAM написа: Show quoted text
> Looks good to me. I much prefer this approach over a loop->later > deferral. > > Offhand I can't think of any cases where it's valid to recurse into > ->_flush_one_read, so I'd be tempted to move the flag check into that > sub as well. Probably the same for _flush_one_write. As such, it > doesn't seem that bad conceptually - same principle as never recursing > into the event loop itself, I guess. > > Only concern I have is the possibility of something skipping the > _flush_one_read because one was already active, then never actually > getting around to the _flush_one_read. Basically, this should be fine: > > 1 while $self->_flush_one_read($eof); > > but there seems to be potential for breakage with: > > $self->_flush_one_read($eof); > > That makes me think that perhaps the '1 while ...' construct should be > moved into a method, but the while condition varies - ->push_on_read > checks for length $self->{readbuff} as well. > > Will revisit the fmap/read_until combo to see if this also fixes the > concurrent ->read_until behaviour (I'm expecting that to Just Work > with this patch applied). > > cheers, > > Tom > > On Tue Jan 27 23:00:43 2015, PEVANS wrote:
> > Patch attached. > > > > While it solves the issue, I can't help thinking there's a > > conceptually-neater solution that doesn't involve this seemingly- > > arbitrary flag just to break the cycle in this way. Maybe we can find > > a neater way sometime. > > > > In the meantime, it does at least seem to work
Released in 0.65 -- Paul Evans