Proposals to get rid of the obscure Lwt_io.Channel_closed("input") exception when a Lwt_process times-out

[kit-ty-kate]

Lwt_main.run (Lwt_process.with_process_in ~timeout:1. ("", [|"sleep"; "2"|]) (fun proc -> Lwt_io.read proc#stdout))

currently (lwt 5.5.0) raises

Exception: Lwt_io.Channel_closed "input"

Worse,

Lwt_main.run (Lwt_process.with_process_in ~timeout:1. ("", [|"sleep"; "2"|]) (fun _ -> Lwt.return ()))

Does not fail at all.

I think it would be less confusing to have the functions from Lwt_process which takes ~timeout fail with a new Lwt_process.Timeout exception.
If you prefer not to change the behaviour, maybe either add a new ?fail_on_timeout:bool argument, or changing the type of ?timeout from float to something like [`Ignore of float | `Fail of float] would be enough.

EDIT: Personally I would prefer the second proposal as it would make users aware of the issue/choice in a more systematic manner.

I just spent days trying to debug why my code would fail with an obscure lwt exception all of a sudden and I wish for no-one to go through that pain ever again.

[Et7f3]

I think a lighter proposal is to create a ProcessTimedOut exception and just throw it. You get the information you wanted without modifying the type.

first thought: NodeJs API style but don't know how to type

In case we give:

Lwt_main.run (Lwt_process.with_process_in ~timeout:(`Ignore 1.) ("", [|"sleep"; "2"|]) (fun proc -> Lwt_io.read proc#stdout))

What will be in proc#stdout if the process is closed in a write/has not flushed all stdout ?
ignore in a timeout mean that process is still closed but we consider as successfully exited ? so maybe it will require to add another parameter to force users to take care of handling as they fit. So I propose:

module Lwt = struct
type 'a t = 'a ref
end
type process_in = int

type ('handler, 'return) onTimeout =
  | Catch: float -> (process_in -> exn -> 'return Lwt.t, 'return) onTimeout
  | Fail: float -> (process_in -> 'return Lwt.t, 'return) onTimeout
  | NoTimeout: (process_in -> 'return Lwt.t, 'return) onTimeout (* for the default value *)

let with_process_in: type return. ?timeout:('handler, return) onTimeout -> 'handler -> return Lwt.t = fun  ?(timeout=NoTimeout) f ->
  match timeout with
    NoTimeout -> f 4
  | Fail _t -> f 3 (Invalid_argument "timeout")
  | Catch _ -> f 3

[raphael-proust]

I'm not sure all the functions in this module should return promises which are rejected by an exception. I think it might be interesting for users to still recover the status of the killed process. So here are some alternatives:

• Use an exception. Too bad about your processes' statuses. (I'd rather use Lwt_unix.Timeout to avoid introducing a new exception with the same name.)
• Use ?fail_on_timeout:bool to choose the behaviour. (I'm not a fan because the semantics is complex.)
• Use an exception, but the exception carries a promise of the status: Process_timeout of Unix.process_status Lwt.t. (I'm not a fan because it ties together two orthogonal control-flow mechanisms.)
• Use a sum type for return with KilledBecauseOfTimeout of Unix.process_status Lwt.t and ExitedOnItsOwn of Unix.process_status. It's not ideal because of the promise in a promise and also because of the backwards-incompatible change. It'll require significant changes in users code. Even those who don't use timeout.
• Use the type system to encode the return type in the type of the ?timeout parameter. I'm not sure how feasible this is if we want to keep timeout as an actual option which would be necessary in order to keep backwards compatibility. I actually don't think it's possible?

It's a bit annoying to change the API for the users who don't set a timeout. And so the exception might turn out to be the best solution there.

[Et7f3]

Use the type system to encode the return type in the type of the ?timeout parameter. I'm not sure how feasible this is if we want to keep timeout as an actual option which would be necessary in order to keep backwards compatibility. I actually don't think it's possible?

See printf it use the first argument a GADT to encode the rest of argument https://github.com/ocaml/ocaml/blob/1546e1f86f87485637719c86d92f32df9292997f/stdlib/camlinternalFormatBasics.ml#L525
So it might be possible but the prototype will not be nice I think.

I think keeping optional will we the hard part if possible.

[Et7f3]

I managed to get their https://gist.github.com/Et7f3/3b4f09ddf5ac1eacf0f4a360d8154d32 this mean we either need to make non optional or force in case of NoTimeout to return unit Lwt.t
or use other solution

[raphael-proust]

That gets us half of the way there… but it still breaks backwards compatibility.

Another possible approach is to deprecate the use of timeout and recommend Lwt_unix.with_timeout instead. The function with_timeout will cancel the promise if the timeout is reached. So we just need to make sure the promise is cancelable and that the canceling triggers the closing of the process. I thin we just need to add wrap_in_cancelable inside of the Lwt_process.make_with.

[kit-ty-kate]

Another possible approach is to deprecate the use of timeout and recommend Lwt_unix.with_timeout instead. The function with_timeout will cancel the promise if the timeout is reached. So we just need to make sure the promise is cancelable and that the canceling triggers the closing of the process. I thin we just need to add wrap_in_cancelable inside of the Lwt_process.make_with.

I haven’t tried yet but I like this approach very much. I don’t think deprecating a parameter is allowed in the OCaml syntax (at least not automatically using [@deprecated]) but simply removing them next update (5.6.0) would be ok i think.

I am very much in favour of that solution.

[raphael-proust]

We might break a few packages by making this breaking change, but we should be able to propose patches to either use with_timeout or add version constraints (in case the different error-management style is incompatible with the rest of the code).