From efe513c3a571524c15835bcf33ebe73ce2a5d6c6 Mon Sep 17 00:00:00 2001 From: Tudor Brindus Date: Sat, 17 Feb 2024 17:10:35 -0500 Subject: [PATCH] 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 --- src/perf_tool_backend.ml | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/perf_tool_backend.ml b/src/perf_tool_backend.ml index efdf1dbfe..6b92c9d25 100644 --- 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