Skip to content

Commit

Permalink
Fix interrupt bugs wrt bracket masking (#171)
Browse files Browse the repository at this point in the history
Currently when we check interrupt status, we are not considering the
bracket masking status. When we enter a masked state (`bracketCount > 0`),
it should be impossible for an interrupt to terminate evaluation, but
without the bracket check in CATCH, RESUME, and RELEASE, this is
violated.

*   In a nested async generalBracket, the wrong handler is enqueued.
Since we are in a masked state, it should be impossible for the `killed`
branch to be invoked. However, if an interrupt occurs it is currently
_always_ invoking `killed` when the branch is actually `completed`.
*   In a nested bracket, an interrupt will terminate evaluation of the
inner bracket even though we are in a masked state. This can also happen
with an inner `catch` in a masked state.

We need to always consider the masked state (`bracketCount > 0`) when we
discriminate the interrupt state.

Fixes #170
  • Loading branch information
natefaubion authored Mar 29, 2019
1 parent b5240af commit 390857f
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 7 deletions.
16 changes: 9 additions & 7 deletions src/Effect/Aff.js
Original file line number Diff line number Diff line change
Expand Up @@ -415,14 +415,14 @@ var Aff = function () {
attempts = attempts._2;

switch (attempt.tag) {
// We cannot recover from an interrupt. Otherwise we should
// We cannot recover from an unmasked interrupt. Otherwise we should
// continue stepping, or run the exception handler if an exception
// was raised.
case CATCH:
// We should compare the interrupt status as well because we
// only want it to apply if there has been an interrupt since
// enqueuing the catch.
if (interrupt && interrupt !== tmp) {
if (interrupt && interrupt !== tmp && bracketCount === 0) {
status = RETURN;
} else if (fail) {
status = CONTINUE;
Expand All @@ -431,11 +431,11 @@ var Aff = function () {
}
break;

// We cannot resume from an interrupt or exception.
// We cannot resume from an unmasked interrupt or exception.
case RESUME:
// As with Catch, we only want to ignore in the case of an
// interrupt since enqueing the item.
if (interrupt && interrupt !== tmp || fail) {
if (interrupt && interrupt !== tmp && bracketCount === 0 || fail) {
status = RETURN;
} else {
bhead = attempt._1;
Expand Down Expand Up @@ -468,19 +468,21 @@ var Aff = function () {
// Enqueue the appropriate handler. We increase the bracket count
// because it should not be cancelled.
case RELEASE:
bracketCount++;
attempts = new Aff(CONS, new Aff(FINALIZED, step, fail), attempts, interrupt);
status = CONTINUE;
// It has only been killed if the interrupt status has changed
// since we enqueued the item.
if (interrupt && interrupt !== tmp) {
// since we enqueued the item, and the bracket count is 0. If the
// bracket count is non-zero then we are in a masked state so it's
// impossible to be killed.
if (interrupt && interrupt !== tmp && bracketCount === 0) {
step = attempt._1.killed(util.fromLeft(interrupt))(attempt._2);
} else if (fail) {
step = attempt._1.failed(util.fromLeft(fail))(attempt._2);
} else {
step = attempt._1.completed(util.fromRight(step))(attempt._2);
}
fail = null;
bracketCount++;
break;

case FINALIZER:
Expand Down
57 changes: 57 additions & 0 deletions test/Test/Main.purs
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,39 @@ test_kill_bracket_nested = assert "kill/bracket/nested" do
, "foo/bar/run/release/bar/release"
]

test_kill_general_bracket_nested Aff Unit
test_kill_general_bracket_nested = assert "kill/bracket/general/nested" do
ref <- newRef []
let
action s = do
_ ← modifyRef ref (_ <> [ s ])
pure unit

bracketAction s acq =
generalBracket acq
{ killed: \_ _ → action (s <> "/killed")
, failed: \_ _ → action (s <> "/failed")
, completed: \_ _ → action (s <> "/completed")
}
(\_ → do
delay (Milliseconds 10.0)
action (s <> "/run"))
fiber ← forkAff do
bracketAction "outer" do
action "outer/acquire"
bracketAction "inner" do
action "inner/acquire"
delay (Milliseconds 10.0)
delay (Milliseconds 5.0)
killFiber (error "nope") fiber
readRef ref <#> eq
[ "outer/acquire"
, "inner/acquire"
, "inner/run"
, "inner/completed"
, "outer/killed"
]

test_kill_supervise Aff Unit
test_kill_supervise = assert "kill/supervise" do
ref ← newRef ""
Expand Down Expand Up @@ -667,6 +700,28 @@ test_regression_kill_sync_async = assert "regression/kill-sync-async" do
killFiber (error "Nope.") f1
pure true

test_regression_bracket_kill_mask Aff Unit
test_regression_bracket_kill_mask = assert "regression/kill-bracket-mask" do
ref ← newRef ""
let
action s = do
_ <- modifyRef ref (_ <> s)
pure unit
fiber ← forkAff do
bracket
do
action "a"
bracket
(pure unit)
(const (pure unit))
(\_ -> delay (Milliseconds 10.0))
action "b"
(const (pure unit))
(\_ -> delay (Milliseconds 10.0))
delay (Milliseconds 5.0)
killFiber (error "nope") fiber
readRef ref <#> eq "ab"

test_regression_kill_empty_supervisor Aff Unit
test_regression_kill_empty_supervisor = assert "regression/kill-empty-supervisor" do
f1 ← forkAff $ supervise $ delay $ Milliseconds 10.0
Expand Down Expand Up @@ -700,6 +755,7 @@ main = do
test_kill_canceler
test_kill_bracket
test_kill_bracket_nested
test_kill_general_bracket_nested
test_kill_supervise
test_kill_finalizer_catch
test_kill_finalizer_bracket
Expand All @@ -723,4 +779,5 @@ main = do
test_regression_par_apply_async_canceler
test_regression_bracket_catch_cleanup
test_regression_kill_sync_async
test_regression_bracket_kill_mask
test_regression_kill_empty_supervisor

0 comments on commit 390857f

Please sign in to comment.