Skip to content

Commit

Permalink
Fix eager side effects in Memo.exec (#4283)
Browse files Browse the repository at this point in the history
Calling [Memo.exec] and then not running the resulting [Fiber.t]
brings the memoization framework into an inconsistent internal
state. This PR makes [Memo.exec] less eager to perform side
effects, delaying them until the returned [Fiber.t] is actually run.

Signed-off-by: Andrey Mokhov <[email protected]>
  • Loading branch information
snowleopard authored Feb 25, 2021
1 parent f2e0079 commit f839fc1
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 7 deletions.
11 changes: 6 additions & 5 deletions src/memo/memo.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1078,11 +1078,12 @@ end = struct
| Needs_work { sample_attempt = _; work } -> work)

let exec_dep_node dep_node =
match consider_dep_node dep_node with
| Ok res ->
let* res = res in
Value.get_async_exn res.value
| Error cycle_error -> raise (Cycle_error.E cycle_error)
Fiber.of_thunk (fun () ->
match consider_dep_node dep_node with
| Ok res ->
let* res = res in
Value.get_async_exn res.value
| Error cycle_error -> raise (Cycle_error.E cycle_error))

let exec_dep_node_internal = consider_dep_node

Expand Down
11 changes: 9 additions & 2 deletions test/expect-tests/memo/memoize_tests.ml
Original file line number Diff line number Diff line change
Expand Up @@ -704,12 +704,12 @@ let%expect_test "diamond with non-uniform cutoff structure" =
[%expect
{|
Evaluating count_runs: 2
Starting evaluating yes_cutoff
Evaluated yes_cutoff: 1
Starting evaluating no_cutoff
Evaluated no_cutoff: 1
Starting evaluating after_no_cutoff
Evaluated after_no_cutoff: 2
Starting evaluating yes_cutoff
Evaluated yes_cutoff: 1
f 0 = Ok 4
|}];
print_result summit 1;
Expand Down Expand Up @@ -806,6 +806,13 @@ let%expect_test "dynamic cycles with non-uniform cutoff structure" =
~from:cycle_creator_yes_cutoff
in
Fdecl.set summit_fdecl summit_yes_cutoff;
(* Calling [Memo.exec] and then not running the resulting [Fiber.t] used to
bring the memoization framework into an inconsistent internal state, due to
the eager execution of some internal side effects. That further manifested
in deadlocks and reappearance of zombie computations. The problem has now
been fixed and so the line below is just a no-op. *)
let _ = Memo.exec cycle_creator_no_cutoff () in
[%expect {| |}];
print_result summit_no_cutoff 0;
[%expect
{|
Expand Down

0 comments on commit f839fc1

Please sign in to comment.