Don't attempt read from closed fd in dune_rpc_lwt

[gridbugs]

This fixes a bug where RPC clients built with dune_rpc_lwt would crash while disconnecting as they attempted to read from a channel whose underlying file descriptor had been closed.

A simple program that reproduces the bug:

let () =
  let init =
    Dune_rpc.V1.Initialize.create ~id:(Dune_rpc.V1.Id.make (Csexp.Atom "hello"))
  in
  let where = Dune_rpc_lwt.V1.Where.default ~build_dir:"_build" () in
  Lwt_main.run
    (let open Lwt.Syntax in
    let* input_channel, output_channel = Dune_rpc_lwt.V1.connect_chan where in
    Dune_rpc_lwt.V1.Client.connect (input_channel, output_channel) init
      ~f:(fun _client -> Lwt.return ()))

This program just connects to an RPC server and then immediately disconnects. Prior to this change it would crash while disconnection with the error:

Fatal error: exception Unix.Unix_error(Unix.EBADF, "check_descriptor", "")
Raised at Lwt_unix.check_descriptor in file "src/unix/lwt_unix.cppo.ml", line 379, characters 4-64
Called from Lwt_unix.blocking in file "src/unix/lwt_unix.cppo.ml", line 384, characters 2-21
Called from Lwt_unix.read_bigarray in file "src/unix/lwt_unix.cppo.ml", line 682, characters 4-15
Called from Lwt_io.perform_io in file "src/unix/lwt_io.ml", line 236, characters 10-35
Called from Lwt_io.Primitives.read_char in file "src/unix/lwt_io.ml", line 676, characters 6-15
Called from Lwt_io.Primitives.read_char_opt.(fun) in file "src/unix/lwt_io.ml", line 686, characters 17-29
Called from Lwt.Sequential_composition.catch in file "src/core/lwt.ml", line 2008, characters 16-20
Re-raised at Lwt.Miscellaneous.poll in file "src/core/lwt.ml", line 3077, characters 20-29
Called from Lwt_main.run.run_loop in file "src/unix/lwt_main.ml", line 31, characters 10-20
Called from Lwt_main.run in file "src/unix/lwt_main.ml", line 118, characters 8-13
Re-raised at Lwt_main.run in file "src/unix/lwt_main.ml", line 124, characters 4-13
Called from Dune__exe__Hello_rpc in file "hello_rpc.ml", line 6, characters 2-260

[rgrinberg]

I think I don't understand Lwt enough to understand why this is necessary. AFAIK, this is the line that does the closing:

        let write (_, o) = function
          | None -> Lwt_io.close o
          | Some csexps ->
            Lwt_list.iter_s
              (fun sexp -> Lwt_io.write o (Csexp.to_string sexp))
              csexps

Which should wake up any readers or writers with a Channel_closed exception and not an EBADF.

Any subsequent readers or writers would get the same exception.

[rgrinberg]

Given the above, I don't see how checking if the fd is closed in the beginning is enough. It could still be closed in the middle of a read in the same way.

[gridbugs]

Here's a minimal program that crashes with Fatal error: exception Unix.Unix_error(Unix.EBADF, "check_descriptor", ""):

let make_socket () =
  let open Lwt.Syntax in
  let+ sock_fd =
    let fd = Lwt_unix.socket Unix.PF_UNIX Unix.SOCK_STREAM 0 in
    let+ () = Lwt_unix.connect fd (Unix.ADDR_UNIX "mysocket") in
    fd
  in
  let fd mode = Lwt_io.of_fd sock_fd ~mode in
  (fd Input, fd Output)

let run () =
  let open Lwt.Syntax in
  let* i, o = make_socket () in
  let* () = Lwt_io.close o in
  let* (_ch : char) = Lwt_io.read_char i in
  Lwt.return_unit

(* run socket server with `nc -lkU mysocket` *)
let () = Lwt_main.run (run ())

Lwt's docs say:

exception Channel_closed of string
  (** Exception raised when a channel is closed. The parameter is a
      description of the channel. *)

It's not clear to me under what conditions Channel_closed will be raised. I don't know if lwt keeps track of the association between the input and output channels when they share an underlying socket.

In any case you are correct that we should be checking if file is open before each read. Will fix.

[gridbugs]

Updated to check that the channel is open before each read.

[rgrinberg]

The end result still doesn't look satisfying to me. I have no idea why Lwt_io.close isn't waking up existing reads/writes. Another idea would be to make the write end do a shutdown on the write end of the socket with Unix.shutdown and let the callers manage their own fd's.

In any case, at least this is an improvement that's unblocking you. We'll accept it as is.

[gridbugs]

Agreed that the workaround is unsatisfying. I create #7624 so we can improve it when we get time. Could also be a good starter issue I think.