Skip to content

Commit

Permalink
Fix finalization after failure (#161)
Browse files Browse the repository at this point in the history
* Fix finalization after failure

It appears to have only worked coincidentally before. When the
interpreter enters into a finalization block, it puts a `Finalized` node
onto the attempt stack with the current `step`. However if there was
a failure, then step would be null. It "worked" before because there was
stale state, and `fail` could be preserved through finalization. This
bug was exposed by using `catchError` within the finalization block,
which resets the failure state to null. Thus when it resumes after
finalization, both `step` and `fail` are null, which is an invalid
state. The solution is to preserve `fail` in addition to `step` in
a `Finalized` node.

* Cleanup potentially stale state
  • Loading branch information
natefaubion authored Aug 24, 2018
1 parent c02ae17 commit 5450756
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 4 deletions.
9 changes: 5 additions & 4 deletions src/Effect/Aff.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,11 @@ var Aff = function () {
// the provided callback in `makeAff` more than once, but it may also be an
// async effect resuming after the fiber was already cancelled.
function run(localRunTick) {
var tmp, result, attempt, canceler;
var tmp, result, attempt;
while (true) {
tmp = null;
result = null;
attempt = null;
canceler = null;

switch (status) {
case STEP_BIND:
Expand Down Expand Up @@ -460,7 +459,7 @@ var Aff = function () {
// because it should not be cancelled.
case RELEASE:
bracketCount++;
attempts = new Aff(CONS, new Aff(FINALIZED, step), attempts, interrupt);
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.
Expand All @@ -471,11 +470,12 @@ var Aff = function () {
} else {
step = attempt._1.completed(util.fromRight(step))(attempt._2);
}
fail = null;
break;

case FINALIZER:
bracketCount++;
attempts = new Aff(CONS, new Aff(FINALIZED, step), attempts, interrupt);
attempts = new Aff(CONS, new Aff(FINALIZED, step, fail), attempts, interrupt);
status = CONTINUE;
step = attempt._1;
break;
Expand All @@ -484,6 +484,7 @@ var Aff = function () {
bracketCount--;
status = RETURN;
step = attempt._1;
fail = attempt._2;
break;
}
}
Expand Down
10 changes: 10 additions & 0 deletions test/Test/Main.purs
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,15 @@ test_regression_par_apply_async_canceler = assert "regression/par-apply-async-ca
val <- readRef ref
pure (val == "throwdone" && message err == "Nope.")

test_regression_bracket_catch_cleanup Aff Unit
test_regression_bracket_catch_cleanup = assert "regression/bracket-catch-cleanup" do
res :: Either Error Unit
try $ bracket
(pure unit)
(\_ catchError (pure unit) (const (pure unit)))
(\_ throwError (error "Nope."))
pure $ lmap message res == Left "Nope."

main Effect Unit
main = do
test_pure
Expand Down Expand Up @@ -697,3 +706,4 @@ main = do
test_parallel_stack
test_regression_return_fork
test_regression_par_apply_async_canceler
test_regression_bracket_catch_cleanup

0 comments on commit 5450756

Please sign in to comment.