Skip to content

Commit

Permalink
WorkerController shouldnt block on get_result after cancel
Browse files Browse the repository at this point in the history
Summary:
Right now, calling WorkerController.get_result after WorkerController.cancel
will block. Thus it is a leaky abstraction. So please please don't do that.

Thankfully, MultiThreadedCall doesn't do that.

This just cleans up this leaky abstraction (which makes the code easier
to read, for those trying to understand workerController.ml and worker.ml).

(In particular, while I was working on it, I found it very surprising that
cancellation worked at all without blocking).

Reviewed By: dabek

Differential Revision: D7537683

fbshipit-source-id: c9425d84fa5393e2a4ade04a10e70a7e26450d79
  • Loading branch information
alexchow authored and hhvm-bot committed Apr 13, 2018
1 parent c55caf9 commit cbac5c4
Showing 1 changed file with 7 additions and 2 deletions.
9 changes: 7 additions & 2 deletions hphp/hack/src/procs/workerController.ml
Expand Up @@ -145,6 +145,7 @@ and ('a, 'b) delayed = 'a * 'b worker_handle
and 'b worker_handle =
| Processing of 'b slave
| Cached of 'b
| Canceled
| Failed of exn

(** The Controller's slave has a Worker. The Worker is itself a single process
Expand All @@ -153,6 +154,7 @@ and 'b worker_handle =
and 'a slave = {

worker: worker; (* The associated worker *)

slave_pid: int; (* The actual slave pid *)

(* The file descriptor we might pass to select in order to
Expand Down Expand Up @@ -405,6 +407,7 @@ let get_result d =
match snd !d with
| Cached x -> x
| Failed exn -> raise exn
| Canceled -> raise End_of_file
| Processing s ->
with_worker_exn d s begin fun () ->
let res = s.result () in
Expand Down Expand Up @@ -443,7 +446,7 @@ let select ds additional_fds =
List.fold_right
~f:(fun d acc ->
match snd !d with
| Cached _ | Failed _ ->
| Cached _ | Canceled | Failed _ ->
{ acc with readys = d :: acc.readys }
| Processing s when List.mem ready_fds s.infd ->
{ acc with readys = d :: acc.readys }
Expand All @@ -456,6 +459,7 @@ let get_worker h =
match snd !h with
| Processing {worker; _} -> worker
| Cached _
| Canceled
| Failed _ -> invalid_arg "Worker.get_worker"

let get_job h = fst !h
Expand All @@ -478,7 +482,8 @@ let wait_for_cancel d =
| Processing s ->
with_worker_exn d s begin fun () ->
s.wait_for_cancel ();
s.worker.busy <- false
s.worker.busy <- false;
d := fst !d, Canceled
end
| _ -> ()

Expand Down

0 comments on commit cbac5c4

Please sign in to comment.