eio_luv: add low-level process support #359
Conversation
There was a problem hiding this comment.
Thanks - looks good :-)
Because this is the low-level API is it okay to just return (int * int64) from the callback function as the exit status (see https://aantron.github.io/luv/luv/Luv/Process/index.html#val-spawn for an explanation)?
Yes, I think that's fine.
Should the low-level API use switches to protect against leaking a process spawn ?
Yes.
I've exposed Eio_luv.Low_level.Stream is that alright (I don't know if it was just because there was no need for it or if there is some other reason it wasn't exposed) ?
Probably just no need before.
lib_eio_luv/eio_luv.mli
Outdated
| @@ -114,6 +114,69 @@ module Low_level : sig | |||
| @param sw The handle is closed when [sw] is released, if not closed manually first. | |||
| @param close_unix if [true] (the default), calling [close] also closes [fd]. *) | |||
| end | |||
|
|
|||
| module Pipe : sig | |||
| type t | |||
There was a problem hiding this comment.
Does this need its own type? Perhaps we could just have this:
| type t | |
| type t = [`Stream of [`Pipe]] Handle.t |
There was a problem hiding this comment.
See comment about redirections below
lib_eio_luv/eio_luv.mli
Outdated
| type t | ||
| (** A process *) | ||
|
|
||
| type redirection |
There was a problem hiding this comment.
Is this to stop people creating the pipe using Luv directly? I think for the low-level API it's fine to expose the connection to Luv - they can already create other pipes that way if they want. i.e. we can just use Luv.Process.redirection in the mli.
There was a problem hiding this comment.
Yep that's right, also why it has a separate type. The problem I seemed to have is that things won't work if the pipe initialisation doesn't contain the loop i.e.Luv.Pipe.init ~loop:(get_loop ())
| let init ?for_handle_passing ~sw () = | ||
| Luv.Pipe.init ~loop:(get_loop ()) ?for_handle_passing () | ||
| |> or_raise | ||
| |> Handle.of_luv ~close_unix:true ~sw |
There was a problem hiding this comment.
I changed this to attach to the switch on init. We need to make sure it's freed even if to_handle isn't called, and we don't want to call it twice if to_handle is used twice.
I also exposed the type in the mli, removing the need for to_luv and to_handle. We already allow users to convert other Luv resources to handles, so might as well allow pipes too.
I opened #371 to expose get_loop.
| let parent_pipe = Handle.to_luv parent_pipe in | ||
| Luv.Process.to_parent_pipe ?readable_in_child ?writable_in_child ?overlapped ~fd ~parent_pipe () | ||
|
|
||
| let await_exit t = Promise.await t.status |
There was a problem hiding this comment.
I moved the remove_hook to spawn. It needs to be called even if the application doesn't wait.
| if not (Promise.is_resolved status) then ( | ||
| let h = Switch.on_release_cancellable sw (fun () -> | ||
| Luv.Process.kill proc Luv.Signal.sigkill |> or_raise; | ||
| ignore (Promise.await status) |
There was a problem hiding this comment.
Added a wait here just so we can be sure the process has completely finished stopping by the time the switch finishes.
|
Thanks! The changes in #371 plus these all look good to me :)) |
talex5
force-pushed
the
eio-luv-process
branch
from
9a5f9f3 to
1c50a3e
Compare
talex5
force-pushed
the
eio-luv-process
branch
from
1c50a3e to
de45321
Compare
|
Thanks! |
CHANGES: API changes: - Unify IO errors as `Eio.Io` (@talex5 ocaml-multicore/eio#378). This makes it easy to catch and log all IO errors if desired. The exception payload gives the type and can be used for matching specific errors. It also allows attaching extra information to exceptions, and various functions were updated to do this. - Add `Time.Mono` for monotonic clocks (@bikallem @talex5 ocaml-multicore/eio#338). Using the system clock for timeouts, etc can fail if the system time is changed during the wait. - Allow datagram sockets to be created without a source address (@bikallem @haesbaert ocaml-multicore/eio#360). The kernel will allocate an address in this case. You can also now control the `reuse_addr` and `reuse_port` options. - Add `File.stat` and improve `Path.load` (@haesbaert @talex5 ocaml-multicore/eio#339). `Path.load` now uses the file size as the initial buffer size. - Add `Eio_unix.pipe` (@patricoferris ocaml-multicore/eio#350). This replaces `Eio_linux.pipe`. - Avoid short reads from `getrandom(2)` (@haesbaert ocaml-multicore/eio#344). Guards against buggy user code that might not handle this correctly. - Rename `Flow.read` to `Flow.single_read` (@talex5 ocaml-multicore/eio#353). This is a low-level function and it is easy to use it incorrectly by ignoring the possibility of short reads. Bug fixes: - Eio_luv: Fix non-tail-recursive continue (@talex5 ocaml-multicore/eio#378). Affects the `Socket_of_fd` and `Socketpair` effects. - Eio_linux: UDP sockets were not created close-on-exec (@talex5 ocaml-multicore/eio#360). - Eio_linux: work around io_uring non-blocking bug (@haesbaert ocaml-multicore/eio#327 ocaml-multicore/eio#355). The proper fix should be in Linux 6.1. - `Eio_mock.Backend`: preserve backtraces from `main` (@talex5 ocaml-multicore/eio#349). - Don't lose backtrace in `Switch.run_internal` (@talex5 ocaml-multicore/eio#369). Documentation: - Use a proper HTTP response in the README example (@talex5 ocaml-multicore/eio#377). - Document that read_dir excludes "." and ".." (@talex5 ocaml-multicore/eio#379). - Warn about both operations succeeding in `Fiber.first` (@talex5 ocaml-multicore/eio#358, reported by @iitalics). - Update README for OCaml 5.0.0~beta2 (@talex5 ocaml-multicore/eio#375). Backend-specific changes: - Eio_luv: add low-level process support (@patricoferris ocaml-multicore/eio#359). A future release will add Eio_linux support and a cross-platform API for this. - Expose `Eio_luv.Low_level.Stream.write` (@patricoferris ocaml-multicore/eio#359). - Expose `Eio_luv.Low_level.get_loop` (@talex5 ocaml-multicore/eio#371). This is needed if you want to create resources directly and then use them with Eio_luv. - `Eio_linux.Low_level.openfile` is gone (@talex5 ocaml-multicore/eio#378). It was just left-over test code.
This PR adds low-level process support to Eio_luv as per the suggestion in #330. A few questions:
(int * int64)from the callback function as the exit status (see https://aantron.github.io/luv/luv/Luv/Process/index.html#val-spawn for an explanation) ?Eio_luv.Low_level.Streamis that alright (I don't know if it was just because there was no need for it or if there is some other reason it wasn't exposed) ?