Don't assume perf is still alive after sending it SIGTERM

We caught this in practice with certain builds of `perf`. This code seems pretty fishy, and I'd like to look into it further later: it's not obvious to me why `waitpid` would raise if the PID no longer exists. I would have expected `perf` to still be an unreaped child at this point. But, we do have ~identical logic in ferrying ^C to `perf` in `src/trace.ml` too... Signed-off-by: Tudor Brindus <[email protected]>
janestreet · Feb 19, 2024 · efe513c · efe513c
1 parent bacf0a0
commit efe513c
Showing 1 changed file with 7 additions and 4 deletions.
diff --git a/src/perf_tool_backend.ml b/src/perf_tool_backend.ml
@@ -440,10 +440,13 @@ module Recording = struct
 
   let finish_recording t =
     Signal_unix.send_i Signal.term (`Pid t.pid);
-    (* This should usually be a signal exit, but we don't really care, if it didn't produce
-       a good perf.data file the next step will fail. *)
-    let%map (res : Core_unix.Exit_or_signal.t) = Async_unix.Unix.waitpid t.pid in
-    perf_exit_to_or_error res
+    (* This should usually be a signal exit, but we don't really care, if it didn't
+       produce a good perf.data file the next step will fail.
+
+       [Monitor.try_with] because [waitpid] raises if perf exited before we get here. *)
+    match%map.Deferred Monitor.try_with (fun () -> Async_unix.Unix.waitpid t.pid) with
+    | Ok res -> perf_exit_to_or_error res
+    | Error _exn -> Ok ()
   ;;
 end