Skip to content

Commit

Permalink
Don't assume perf is still alive after sending it SIGTERM
Browse files Browse the repository at this point in the history
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 <tbrindus@janestreet.com>
  • Loading branch information
Xyene committed Feb 19, 2024
1 parent bacf0a0 commit efe513c
Showing 1 changed file with 7 additions and 4 deletions.
11 changes: 7 additions & 4 deletions src/perf_tool_backend.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit efe513c

Please sign in to comment.