From 390857f9143e1a52f7403d05b14c9ca79d356737 Mon Sep 17 00:00:00 2001 From: Nathan Faubion Date: Fri, 29 Mar 2019 09:10:51 -0700 Subject: [PATCH] Fix interrupt bugs wrt bracket masking (#171) 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 --- src/Effect/Aff.js | 16 +++++++------ test/Test/Main.purs | 57 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/Effect/Aff.js b/src/Effect/Aff.js index 63a38e7..190216e 100644 --- a/src/Effect/Aff.js +++ b/src/Effect/Aff.js @@ -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; @@ -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; @@ -468,12 +468,13 @@ 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); @@ -481,6 +482,7 @@ var Aff = function () { step = attempt._1.completed(util.fromRight(step))(attempt._2); } fail = null; + bracketCount++; break; case FINALIZER: diff --git a/test/Test/Main.purs b/test/Test/Main.purs index 451c6cc..f175429 100644 --- a/test/Test/Main.purs +++ b/test/Test/Main.purs @@ -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 "" @@ -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 @@ -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 @@ -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