Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supply eats all values from Channel, not allowing reuse across multiple react blocks #2646

Open
kbucheli opened this Issue Jan 25, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@kbucheli
Copy link

kbucheli commented Jan 25, 2019

The Problem

The data from a channel is not received anymore if the same react/whenever block is executed twice.

Expected Behavior

The second execution should receive the second item in the channel.

Actual Behavior

nothing is received

Steps to Reproduce

#!/usr/bin/env perl6

my $execute_max = 2;
my $timeout = 10;

my $channel = Channel.new;

$channel.send(1);
$channel.send(2);

my $worker = start {
    for (1..$execute_max) {
        my $timed-out = False;
        react {
            note "try to receive $channel ";
            whenever $channel {
                note "received from backfill-channel $_";
                done;
            }
            whenever Promise.in($timeout) {
                note "timed out";
                $timed-out = True;
                done;
            }
        }
        last if $timed-out;
    }
    note "finishing worker";
};

await($worker);
note "workers done, remaining in Channel: " ~ $channel.poll.gist;

outputs

try to receive Channel<139662271319664> 
received from backfill-channel 1
try to receive Channel<139662271319664> 
timed out
finishing worker
workers done, remaining in Channel: Nil

remark: if it would be possible to set a timeout on Channel.receive I would not have found the bug ;-)

Environment

  • Operating system: custom Linux
  • Compiler version (perl6 -v):
This is Rakudo version 2018.12 built on MoarVM version 2018.12
implementing Perl 6.d.
@lizmat

This comment has been minimized.

Copy link
Contributor

lizmat commented Jan 26, 2019

I don't think this is a bug. You mark the react block as done after receiving the first value from the channel. Thus starting a new react block (and probably shutting down the channel). I've golfed the issue down to:

my $timeout = 10;

my $channel = Channel.new;

$channel.send(1);
$channel.send(2);

react {
    note "try to receive $channel ";
    whenever $channel {
        note "received from backfill-channel $_";
        done;   # this appears to not have the intended effect
    }
    whenever Promise.in($timeout) {
        note "timed out";
        done;
    }
}
note "workers done, remaining in Channel: " ~ $channel.poll.gist;

If you remove the done from the whenever $channel block, the output becomes:

try to receive Channel<140248926202304> 
received from backfill-channel 1
received from backfill-channel 2
timed out
workers done, remaining in Channel: Nil

Now, according to documentation, if you would add a $channel.close after the $channel.send statements, the react block should automatically end after it received a "closing" signal on the channel. This however, does not seem to happen. And perhaps that is the underlying bug you're trying to fix?

@lizmat

This comment has been minimized.

Copy link
Contributor

lizmat commented Jan 26, 2019

A simple one liner appears to do what is documented:

my $c = Channel.new; $c.send($_) for ^2; $c.close; react { whenever $c { .say } }; say "done"
0
1
done

So looks it's some interaction with the timeout.

@lizmat

This comment has been minimized.

Copy link
Contributor

lizmat commented Jan 26, 2019

Please disregard previous comments: I think I understand what happens.

When you use a Channel $c in a whenever, you are in fact processing values coming in from a $c.Supply.tap. This supply grabs all of the values of the Channel and starts emitting them. This Supply however, is closed when the done is executed. So it ate all the values, but only produced one before being discarded. The next iteration in the loop will then create a new Supply, which will now wait for things to happen on a Channel that is not getting any new values, thus creating the described behaviour.

I think this could be fixed by mixing in the created Supply into the Channel object, so that it can continue with the Supply in the next iteration, and only close the Supply if the Channel it is associated with is also closed.

@jnthn Is that a solution to this problem? Or is there a better way?

@lizmat lizmat changed the title channel omitting items when used repeately in react/whenever Supply eats all values from Channel, not allowing reuse across multiple react blocks Jan 26, 2019

@lizmat

This comment has been minimized.

Copy link
Contributor

lizmat commented Jan 26, 2019

Alas, a naive way of keeping the Supply with the Channel does not fix this issue:

diff --git a/src/core/Channel.pm6 b/src/core/Channel.pm6
index 6e190f2..2b1b13a 100644
--- a/src/core/Channel.pm6
+++ b/src/core/Channel.pm6
@@ -12,6 +12,7 @@ my class Channel does Awaitable {
     # The queue of events moving through the channel.
     my class Queue is repr('ConcBlockingQueue') { }
     has $!queue;
+    has $!supply;

     # Promise that is triggered when all values are received, or an error is    
     # received and the channel is thus closed. 
@@ -115,7 +116,7 @@ my class Channel does Awaitable {
 
     method Capture(Channel:D:) { self.List.Capture }
     multi method Supply(Channel:D:) {
-        supply { 
+        $!supply //= supply {
             # Tap the async notification for new values supply.
             whenever $!async-notify.unsanitized-supply.schedule-on($*SCHEDULER)
                 my Mu \got = self.poll;
@lizmat

This comment has been minimized.

Copy link
Contributor

lizmat commented Jan 27, 2019

@jnthn: Why does Channel.Supply eat all of the values? Shouldn't that just hook into the async.notify and then receive? That should work with just a single Tap?

@kbucheli

This comment has been minimized.

Copy link
Author

kbucheli commented Jan 30, 2019

I created a unit test:

#!/usr/bin/env perl6

use Test;
plan 1;

my $channel = Channel.new;

$channel.send(1);
$channel.send(2);

react whenever $channel { done }

is $channel.poll, 2, 'second value should still be in the channel';
@jnthn

This comment has been minimized.

Copy link
Member

jnthn commented Jan 31, 2019

I'll be away for a few days, so won't be able to poke at it for a bit, but I'd perhaps try a patch in this direction:

diff --git a/src/core/Channel.pm6 b/src/core/Channel.pm6
index 6e190f2..9b58d4e 100644
--- a/src/core/Channel.pm6
+++ b/src/core/Channel.pm6
@@ -116,8 +116,11 @@ my class Channel does Awaitable {
     method Capture(Channel:D:) { self.List.Capture }
     multi method Supply(Channel:D:) {
         supply {
+            my $closed = False;
+
             # Tap the async notification for new values supply.
             whenever $!async-notify.unsanitized-supply.schedule-on($*SCHEDULER) {
+                done if $closed;
                 my Mu \got = self.poll;
                 if nqp::eqaddr(got, Nil) {
                     if $!closed_promise {
@@ -137,6 +140,7 @@ my class Channel does Awaitable {
             # not result in a notification, and so we'd not emit it on
             # the supply. This lost event can then cause a deadlock.
             loop {
+                done if $closed;
                 my Mu \got = self.poll;
                 last if nqp::eqaddr(got, Nil);
                 emit got;
@@ -147,6 +151,10 @@ my class Channel does Awaitable {
                     ?? done()
                     !! die $!closed_promise.cause
             }
+
+            CLOSE {
+                $closed = True;
+            }
         }
     }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.