diff --git a/lib/dev.ml b/lib/dev.ml index 64d0cd5..9057471 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -2410,13 +2410,39 @@ let serve t fd = | r -> r | exception e when not (fatal e) -> reply in - Wire.send fd annotated; - if op = Some "close" then true else go () + (* The write is guarded and the guard is not decoration: an [exception] + case on a [match] covers the scrutinee only, so before this an [EPIPE] + here went straight past the two below and out of the accept loop, + ending the session. An editor that left before its reply arrived is a + closed connection and nothing more, which is what [false] says. *) + (match Wire.send fd annotated with + | () -> if op = Some "close" then true else go () + | exception Unix.Unix_error _ -> false) | exception Wire.Closed -> false | exception Unix.Unix_error _ -> false in go () +(* An editor that goes away between its request and the reply to it leaves this + process writing into a socket with no reader, and a write into a socket with + no reader is SIGPIPE — whose default action is to kill the process. In the + two-process daemon that would lose the session; in the merged build it kills + the program, the compiler and the listener together, and leaves the socket + file behind for the next client to get ECONNREFUSED on. That is not a + theoretical shape: `M-x flan-dev' reconnects a dead connection, Emacs tears + the old process down when it does, and a reply already on its way out lands + in the gap. + + Ignored rather than handled, because [serve] already treats a failed write + as a closed connection: with the signal out of the way [Wire.send] raises + [EPIPE] like any other [Unix_error], the connection is dropped, and the + accept loop goes back to waiting for the next one. The program's own + listener in flan_agent.c reached the same conclusion from the other side and + passes MSG_NOSIGNAL; this is that decision for the half written in OCaml. *) +let ignore_sigpipe () = + try Sys.set_signal Sys.sigpipe Sys.Signal_ignore + with Invalid_argument _ -> () + (* [accept] would block past the program's own exit, so it is waited on with a timeout and the child checked each time round: a daemon whose program has finished has nothing left to do, and an editor waiting on it would wait @@ -2511,6 +2537,7 @@ let two_process ?(debug = false) ~file ~sock () = out = Buffer.create 4096; n = 0; gen = 0; owners = Hashtbl.create 32; host_ll; host_exe = exe; finished = false } in + ignore_sigpipe (); (try Unix.unlink sock with Unix.Unix_error _ -> ()); let ls = Unix.socket Unix.PF_UNIX Unix.SOCK_STREAM 0 in Unix.bind ls (Unix.ADDR_UNIX sock); @@ -2925,6 +2952,7 @@ let merged_setup () = out = Buffer.create 4096; n = 0; gen = 0; owners = Hashtbl.create 32; host_ll; host_exe = exe; finished = false } in + ignore_sigpipe (); (try Unix.unlink sock with Unix.Unix_error _ -> ()); let ls = Unix.socket Unix.PF_UNIX Unix.SOCK_STREAM 0 in Unix.bind ls (Unix.ADDR_UNIX sock); diff --git a/test/test_emacs.ml b/test/test_emacs.ml index 9b7f378..c578f33 100644 --- a/test/test_emacs.ml +++ b/test/test_emacs.ml @@ -63,9 +63,14 @@ let () = else if not (have "clang" && have "llc") then print_endline "emacs: skipped (no clang or llc on PATH)" else begin - let sock = tmp "dev.sock" and out = tmp "prog.out" in - List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ sock; out ]; + let sock = tmp "dev.sock" and out = tmp "prog.out" and err = tmp "dev.err" in + List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ sock; out; err ]; let fd = Unix.openfile out [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 in + (* The daemon's stderr to a file rather than inherited: it carries the + "ready on" line and any OCaml backtrace, and a failure here wants both + quoted next to the client's complaint rather than interleaved with + whatever else dune was running. *) + let efd = Unix.openfile err [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600 in let flan = "../bin/main.exe" in (* Absolute, because the client starts its own daemon at the end of the run and does it from the program's directory rather than from this one. *) @@ -80,9 +85,10 @@ let () = let pid = Unix.create_process flan [| flan; "dev"; "programs/dev-repl.flan"; "-s"; sock |] - Unix.stdin fd Unix.stderr + Unix.stdin fd efd in Unix.close fd; + Unix.close efd; (* Thirty seconds and not the 8s default: this waits on an llc-and-link of the whole program, ~0.5s warm and 2s cold, worst measured at 6.8s with this suite's other binaries running beside it. The watchdog bounds the @@ -113,20 +119,61 @@ let () = in (* The client disconnects at the end, which is what ends the daemon. If it did not get that far — because it failed — nothing else will, so it is - stopped here rather than left waiting on a socket nobody will use. *) + stopped here rather than left waiting on a socket nobody will use. + + The wait *status* is kept, and not thrown away as it used to be, because + it is the one fact that tells the two failure shapes apart. A daemon + that exited on its own runs its cleanup and unlinks the socket, and the + client then says "nothing is listening"; a daemon killed by a signal + never gets that far, leaves the socket file behind, and the client gets + an ECONNREFUSED it cannot explain. Two diagnoses of this flake have + already been wrong for want of this line. *) + let status = ref None in if not (await ~ms:3000 (fun () -> match Unix.waitpid [ Unix.WNOHANG ] pid with | 0, _ -> false - | _ -> true + | _, st -> status := Some st; true | exception Unix.Unix_error _ -> true)) then begin (try Unix.kill pid Sys.sigterm with Unix.Unix_error _ -> ()); (try ignore (Unix.waitpid [] pid) with Unix.Unix_error _ -> ()) end; - List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ sock; out; buf ]; + let how = + match !status with + | Some (Unix.WEXITED n) -> Printf.sprintf "exited with status %d" n + | Some (Unix.WSIGNALED n) -> + Printf.sprintf "was killed by signal %d — nothing ran its cleanup, so \ + the socket file is stale rather than gone" n + | Some (Unix.WSTOPPED n) -> Printf.sprintf "stopped on signal %d" n + | None -> "was still running when the client finished, and was terminated" + in + (* Read before the removal below, because on a failure this is the evidence + and the removal is what destroyed it last time. *) + let tail_of path = + match open_in_bin path with + | ic -> + let n = in_channel_length ic in + let want = min n 4000 in + seek_in ic (n - want); + let s = really_input_string ic want in + close_in ic; + s + | exception Sys_error _ -> "" + in + let sock_left = Sys.file_exists sock in + let prog_out = if code = 0 then "" else tail_of out in + let daemon_err = if code = 0 then "" else tail_of err in + List.iter + (fun f -> try Sys.remove f with Sys_error _ -> ()) + [ sock; out; err; buf ]; if code = 0 then print_endline "emacs: all tests passed" else begin Printf.printf "\nemacs client exited %d\n" code; + Printf.printf "the daemon %s\n" how; + Printf.printf "%s was %s after the run\n" sock + (if sock_left then "still there" else "gone"); + Printf.printf "\n-- the program's own output, last 4k --\n%s\n" prog_out; + Printf.printf "\n-- the daemon's stderr, last 4k --\n%s\n" daemon_err; exit 1 end end