Windows: process spawning support - #936
Conversation
On Windows,an anonymous pipe cant be made non-blocking, so this call always failed with an ENOTSOCK for me. We now use a pool thread for blocking reads and writes.
|
I'll need to back out the pipe changes here (see #929) , but the rest of the process spawning is still in reasonable shape. |
talex5
left a comment
There was a problem hiding this comment.
Just a quick review (I know it needs updating to remove the pipe stuff, so I've ignored those bits).
I think the logic for waiting for the child isn't quite right, and could probably be simplified.
The changes to nt_path look generally useful and could be split into another PR if you want to get that out of the way first.
| let read_all flow = | ||
| let b = Buffer.create 100 in | ||
| Eio.Flow.copy flow (Eio.Flow.buffer_sink b); | ||
| Buffer.contents b |
There was a problem hiding this comment.
This is just Eio.Flow.read_all.
| let check_status msg expected = function | ||
| | `Exited code when code = expected -> () | ||
| | status -> | ||
| Alcotest.failf "%s: expected exit %d, got %a" msg expected Process.pp_status status |
There was a problem hiding this comment.
Could Alcotest.of_pp help here? I'd expect something like:
let status = Alcotest.of_pp Eio.Process.pp_statusThat would also eliminate the need for a separate check_signalled below.
| check_status "findstr" 0 (Process.await child); | ||
| Alcotest.(check string) "roundtrip" "hello" (String.trim out) | ||
|
|
||
| (* A handle leaking into a sibling would delay its pipe's EOF *) |
There was a problem hiding this comment.
I don't see how this tests that. Even if another process inherits a pipe, it will just exit quickly anyway and the test will still pass. Maybe this comment belongs to the next test instead?
| Switch.run @@ fun sw -> | ||
| let r, w = Eio_unix.pipe sw in | ||
| let w_fd = Option.get (Eio_unix.Resource.fd_opt w) in | ||
| Eio_unix.Fd.use_exn "pipe" w_fd Unix.clear_close_on_exec; |
There was a problem hiding this comment.
If close_on_exec is clear, then inheriting the FD is the correct behaviour (at least, that's what the other backends do, and it's what I'd expect).
| ~fds:(std_fds @ [3, Eio_unix.Fd.stdin, `Blocking]) | ||
| ["cmd"; "/c"; "exit"; "0"])) | ||
|
|
||
| (* An unlisted descriptor is inherited, as on Unix. *) |
There was a problem hiding this comment.
How do we know this worked? exit doesn't try to use any of the FDs.
| match Promise.await t.exited with | ||
| | Ok code -> code | ||
| | Error ex -> raise ex |
There was a problem hiding this comment.
| match Promise.await t.exited with | |
| | Ok code -> code | |
| | Error ex -> raise ex | |
| Promise.await_exn t.exited |
|
|
||
| When [time] is reached, [k] is resumed. Cancelling [k] removes the entry from the timer. *) | ||
|
|
||
| val await_thread : t -> 'a Eio_utils.Suspended.t -> ?finished:(('a, exn) result -> unit) -> (unit -> 'a) -> exit |
There was a problem hiding this comment.
This await_thread thing seems really odd. It looks like it waits for the process unless cancelled. But if the spawning switch is cancelled, we want to signal the process and wait for it to exit, so there's no reason to do this.
Maybe something like this:
let watcher = Thread.create (fun h -> Promise.resolve t.exited (eio_process_wait h)) h;The you can just await t.exited, and signal the process if the switch ends. eio_posix has some suitable logic for this already (might not need reap on Windows, not sure whether calling Thread.join is necessary):
eio/lib_eio_posix/low_level.ml
Lines 681 to 695 in a8ae279
(it's slightly complicated because we decided that the switch ending should terminate the child, rather than waiting for it to finish)
The long awaited Windows process spawn after 2 unsuccessful other attempts that I didn't open a PR for! This is layered over #929 as we need pipes to work for tests.
This doesn't use Unix.create_process since I found it tricky to integrate:
Note that any signal sent will terminate the process. Should we actually parse the signal numbers and do something 'terminal' only on SIGKILL/TERM?
Another difference from Posix is that CreateProcess resolves the executable path itself if its blank, but I could replicate the PATH search that we do in Posix (Windows seems to prefer the search to be done by the call, I'm not sure).
We may need to restrict to OCaml 5.4 or higher since that's when @dra27 added caml_stat_char_array_to_utf16
Caveat: I am very inexperienced in these APIs so this is the results of much rooting around in MSDN, but I am happy to stand corrected on any of these decisions. This does progress the Forester test suite due to it spawning
xcopyquite significantly though! :-)