Skip to content

Commit

Permalink
Fix an add/remove race in Supplier::Preserving
Browse files Browse the repository at this point in the history
The tap list was vulnerable to a situation where the `tap(...)` call
could result in a sequence of actions that would `close` that tap,
but the tap had not been added to the tap list yet. This could result
in us having a taplist containing `Mu`, and `Mu.emit` would blow up
(the call meaning to target a TapListEntry`. Even if the `Mu` insertion
were to be avoided, we'd still end up with the closed tap being put in
the list.

Resolve this fully by introducing a pair of flags to indicate if we
have added or removed the entry. Only really do the removal if we've
added it, and only really do the addition if we didn't (in spirit)
remove it.

Fixes flapping in S17-supply/supplier-preserving.t.
  • Loading branch information
jnthn committed Nov 2, 2018
1 parent 15c16e3 commit 68d79e7
Showing 1 changed file with 24 additions and 12 deletions.
36 changes: 24 additions & 12 deletions src/core/Supply.pm6
Expand Up @@ -1660,25 +1660,37 @@ my class Supplier::Preserving is Supplier {
method tap(&emit, &done, &quit, &tap) {
my $tle := TapListEntry.new(:&emit, :&done, :&quit);
my int $replay = 0;
# Since we run `tap` before adding, there's a small chance of
# a tap removal attempt happening for the add attempt. We use
# these two flags to handle that case. This is safe since we
# only ever access them under lock.
my $added := False;
my $removed := False;
my $t = Tap.new({
$!lock.protect({
my Mu $update := nqp::list();
for nqp::hllize($!tappers) -> \entry {
nqp::push($update, entry) unless entry =:= $tle;
if $added {
my Mu $update := nqp::list();
for nqp::hllize($!tappers) -> \entry {
nqp::push($update, entry) unless entry =:= $tle;
}
$!replay-done = 0 if nqp::elems($update) == 0;
$!tappers := $update;
}
$!replay-done = 0 if nqp::elems($update) == 0;
$!tappers := $update;
$removed := True;
});
});
tap($t);
$!lock.protect({
my Mu $update := nqp::isconcrete($!tappers)
?? nqp::clone($!tappers)
!! nqp::list();
nqp::push($update, $tle);
$replay = 1 if nqp::elems($update) == 1;
self!replay($tle) if $replay;
$!tappers := $update;
unless $removed {
my Mu $update := nqp::isconcrete($!tappers)
?? nqp::clone($!tappers)
!! nqp::list();
nqp::push($update, $tle);
$replay = 1 if nqp::elems($update) == 1;
self!replay($tle) if $replay;
$!tappers := $update;
}
$added := True;
});
$t
}
Expand Down

0 comments on commit 68d79e7

Please sign in to comment.