Cohttp-eio: "handle used after calling close" while reading body / Closing connection too soon?

[smondet]

This http-client code works when the content-length is “small” but starts
failing hard once it is a few kilobytes:

let response : Http.Response.t * Eio.Buf_read.t =
  Cohttp_eio.Client.call
    eio
    ?port
    ~headers:(Http.Header.of_list headers)
    ~host ~meth ?body resource
in
let body = Cohttp_eio.Client.read_fixed response in

The exception cannot be caught (it seems to be in the loop of the Luv backend):

Uncaught exception in run loop:
Exception: Invalid_argument("read_start: handle used after calling close!")
Raised at Stdlib.invalid_arg in file "stdlib.ml", line 30, characters 20-45
Called from Eio_luv.Low_level.Stream.read_into.(fun) in file "vendor/eio/lib_eio_luv/eio_luv.ml", line 512, characters 32-62
Called from Eio_luv.wakeup in file "vendor/eio/lib_eio_luv/eio_luv.ml", line 1179, characters 4-8
Called from Eio_luv.run2.(fun) in file "vendor/eio/lib_eio_luv/eio_luv.ml", line 1194, characters 10-40
Fatal error: exception Failure("Deadlock detected: no events scheduled but main function hasn't returned")
Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
Called from Stdlib__Fun.protect in file "fun.ml", line 33, characters 8-15
Re-raised at Stdlib__Fun.protect in file "fun.ml", line 38, characters 6-52
Called from Dune__exe__Backend_server_basics in file "test/backend_server_basics.ml", line 79, characters 2-1023

Looking at the code
client.ml:100:

• it seems that indeed Eio.Buf_read.of_flow ~initial_size ~max_size:max_int conn is lazy enough
• so when Cohttp_eio.Client.read_fixed gets called it still tries to pull from the socket (?)
• The doc of Eio.Net.with_tcp_connect says: “[conn] is closed after [f] returns (if it isn't already closed by then).”

let buf_write conn =
  let initial_size = 0x1000 in
  Buf_write.with_flow ~initial_size:0x1000 conn (fun writer ->
      let request = Http.Request.make ?meth ?version ~headers resource_path in
      let request = Http.Request.add_te_trailers request in
      write_request pipeline_requests request writer body;
      let reader =
        Eio.Buf_read.of_flow ~initial_size ~max_size:max_int conn
      in
      let response = response reader in
      (response, reader))
in
match conn with
| None ->
    let service =
      match port with Some p -> string_of_int p | None -> "80"
    in
    Eio.Net.with_tcp_connect ~host ~service env#net (fun conn ->
        Eio.traceln "using with_tcp_connect";
        buf_write conn)
| Some conn ->
    Eio.traceln "NOT using with_tcp_connect";
    buf_write conn

---

I tried to replace read_fixed with this to see what happens:

let body =
  let buf = snd response in
  let res = Buffer.create 42 in
  let rec go n =
    try
      Buffer.add_char res (Eio.Buf_read.any_char (* take_all *) buf);
      if n % 20 = 0 then traceln "not done reading: %d" n;
      go (n + 1)
    with _ ->
      traceln "done reading: %d" n;
      Buffer.contents res
  in
  go 0
in

It fails after saying +not done reading: 3960

[smondet]

Confirming that this ugly “Lazy.force” hack avoids the problem (but preserves the API :) ):

diff --git a/vendor/cohttp/cohttp-eio/src/client.ml b/vendor/cohttp/cohttp-eio/src/client.ml
index a9942f1e..fce2feaf 100644
--- a/vendor/cohttp/cohttp-eio/src/client.ml
+++ b/vendor/cohttp/cohttp-eio/src/client.ml
@@ -107,7 +107,7 @@ let call ?(pipeline_requests = false) ?meth ?version
           Eio.Buf_read.of_flow ~initial_size ~max_size:max_int conn
         in
         let response = response reader in
-        (response, reader))
+        (response, Buf_read.take_all reader |> Buf_read.of_string))
   in
   match conn with
   | None ->

[mseri]

Ping @bikallem

[bikallem]

/cc @talex5 @haesbaert It seems to be related to eio luv backend. Any ideas?

@smondet Can you replicate this in eio linux (uring) backend too?

[talex5]

It's because the conn argument to Client.get is now optional. That means that the connection can't outlive the call, which means you can't read the body.

Looks like this broke it: #958

[talex5]

However, we should make Eio not crash the event loop in this case!

[talex5]

One way to fix this would be to take a body parser argument and return the result of that, rather than leaving the connection open after the call returns.

[bikallem]

One way to fix this would be to take a body parser argument and return the result of that, rather than leaving the connection open after the call returns.

Do you mean like read the whole body like @smondet suggests?

[smondet]

@bikallem I think he means something more parametric like passing a function to handle the reader instead of returning it directly
(my Buf_read.take_all reader |> Buf_read.of_string was really just to show that if you read the whole thing while inside with_tcp_connect you don't get an error)

Maybe like this?

val Cohttp_eio.Client.call : ?pipeline_requests:bool ->
  ?meth:Http.Method.t ->
  ?version:Http.Version.t ->
  ?headers:Http.Header.t ->
  ?body:Cohttp_eio.Body.t ->
  ?conn:#Eio.Flow.two_way ->
  ?port:int ->
  ?on_body: (Eio.Buf_read.t -> 'a)
  < net : Eio.Net.t; .. > ->
  host:string -> string -> Http.Response.t * 'a

~on_body:Fn.id would reproduce the problem but at least it's documentable & more obivous

Or even

val Cohttp_eio.Client.with_call:
  ?pipeline_requests:bool ->
  ?meth:Http.Method.t ->
  ?version:Http.Version.t ->
  ?headers:Http.Header.t ->
  ?body:Cohttp_eio.Body.t ->
  ?conn:#Eio.Flow.two_way ->
  ?port:int ->
  < net : Eio.Net.t; .. > ->
  host:string -> string ->
  ((Http.Response.t * Eio.Buf_read.t) -> 'a) ->
   'a

because you may want to do something different with the body depending on the response?

[bikallem]

val Cohttp_eio.Client.with_call:
?pipeline_requests:bool ->
?meth:Http.Method.t ->
?version:Http.Version.t ->
?headers:Http.Header.t ->
?body:Cohttp_eio.Body.t ->
?conn:#Eio.Flow.two_way ->
?port:int ->
< net : Eio.Net.t; .. > ->
host:string -> string ->
((Http.Response.t * Eio.Buf_read.t) -> 'a) ->
'a

Ah, okay. Yes, this should probably solve the connection being alive issue.

[talex5]

The problem seems to be that we have one function (call) that is now trying to do two different things:

1. Take an existing connection and speak the HTTP protocol on it, OR
2. Take a network, make a connection, and parse the response.

I suggest reverting the changes to call (in #958), and instead adding a new function with the convenience behaviour.

[bikallem]

Just to give an update. I am working on a fix for this issue along with a few others.

[mseri]

I agree with your suggestion. Thanks for the update