Lower-level support for long-running cohttp-async connections

[brendanlong]

This adds Cohttp_async.Client.Connection which allows you to open
a connection and re-use it for multiple requests.

Unlike callv, this makes it easy to handle errors and correlate
requests and responses.

[brendanlong]

I'm opened this as a PR to give an example of the interface I'm looking for, but I'm open for suggestions if there's a better way to do this.

Cohttp_async.Client.callv doesn't work for me because I need arbitrary-length connections (basically trying to keep a connection open until the server closes it), but callv doesn't give me any way to catch the error when the server closes the connection (the pipe just locks up).

One piece I'm still trying to figure out is if there's any way to detect that the server has disconnected before sending subsequent requests. The Reader.close_finished and Writer.consumer_left events don't seem to trigger until after I made another request.

[brendanlong]

(I'm also happy to match any style guidelines but I'm not sure what they are for this project)

[brendanlong]

In the internal project I'm working on, I'm attempting a request, catching the "Connection closed by remote host" exception and then retrying once:

  let rec loop ~is_first_request =
    Monitor.try_with (fun () ->
        let%bind connection =
          let key = Scheme_host_port.of_uri target in
          match Hashtbl.find t.open_connections key with
          | Some connection when not (Cohttp_async.Client.Connection.is_closed connection)
            -> return connection
          | _ ->
            let%bind ssl_config = ssl_config ?hostname:(Uri.host target) () in
            let%map connection =
              Cohttp_async.Client.Connection.connect ~ssl_config target
            in
            Hashtbl.set t.open_connections ~key ~data:connection;
            connection
        in
        Cohttp_async.Client.Connection.request ~body connection req)
    >>= function
    | Ok response -> return response
    | Error e ->
      (* We have no way of detecting if the remote side closed a connection before we were able to make a request,
         so always make a request and retry once if the connection was closed *)
      let remote_connection_closed =
        match Monitor.extract_exn e with
        (* This exception is thrown by Cohttp if the other end hangs up *)
        | Failure msg when is_first_request && msg = "Connection closed by remote host" ->
          true
          (* This exception is thrown if multiple requests are queued when the connection is closed *)
        | e when Exn.to_string e |> String.is_substring ~substring:"throttle aborted job"
          -> true
        | _ -> false
      in
      if remote_connection_closed
      then (
        Log.Global.info
          "Remote connection to %s was closed, opening a new one"
          (Uri.to_string target);
        loop ~is_first_request:false)
      else (
        Log.Global.error "%s" (Exn.to_string e);
        Exn.reraisef
          e
          !"Failed to connect to '%s' with request: %{sexp:Request.t}"
          (Uri.to_string uri)
          req
          ())
  in
  loop ~is_first_request:true

[brendanlong]

I currently have code similar to this inlined in my Blue_http client but I would rather upstream this piece if I can since I need pieces of Cohttp_async that aren't publish (Cohttp_async.Client.Net): https://github.com/brendanlong/blue-http/blob/master/src/connection.ml

[mseri]

Sorry for the delay on this PR.
Gosh, I wish there was a more elegant and robust way than pattern matching on the error message string. Maybe this could become easier once we move to conduit 3.0 (see the on_handler_error function in #692), or maybe not.

[mseri]

@avsm I have never used async, I don't feel in the position to properly review this change. Can you recommend somebody more familiar with it for the review?

[brendanlong]

The "connection closed by remote host" exception is throw by Cohttp, so we could replace that with a more explicit (not-Failure) exception if you want to: https://github.com/mirage/ocaml-cohttp/blob/master/cohttp-async/src/client.ml#L53

[mseri]

If you want to rebase this on master we can try to have it merged before cohttp-3 is merged, or have it ready for a bugfix chottp 3.1 release at least

[brendanlong]

Ok I rebased this

[mseri]

Thanks. Yesterday I spent some time to understand what you were doing. It took a while to wrap my head around async but I believe the patch is sound. Is there any test worth to add?

Besides that. Do you still need to catch the "Connection closed by remote host" exception in the same way when using the new cohttp on the master branch?

[mseri]

In #692 there was this suggestion concerning the issue:

do you think that the new conduit and this revamp of cohttp can solve the issue pointed out here: #704?

From my perspective, the ability to use something else (defined by the user) than a simple socket such as, in this case, a pool of socket/connection is possible - but it's not well documented and it's not the part of conduit to propose such implementation. The initial issue should be fixed in a different path where the user should implement its own protocol, register it into conduit and re-extract/de-abstract it as well in callv.

So it will not fix the initial issue (where conduit provides something simpler/easier to maintain) but it unlocks the ability to inject an another (new) kind of connection (with another impl. of connect/recv/send/close) into cohttp independently than conduit and cohttp.

So for me, we should keep #704 and may be rethink it in the scope of conduit3.0.0.

The reason I am asking is that exposing the Connection module is somewhat diverging (again) the interfaces between cohttp-async and cohttp-lwt. If a solution using the new capabilities of conduit is possible, that would make it reusable everywhere (so I ping again @dinosaure -- sorry, today you must have received a million emails from me)

[dinosaure]

So, I did a deep look into the proposed PR and it seems that the initial error is mostly about the current proposed API of callv. It's hard for me to find a homogenized and safe way to be able to send multiple request into the same connection. My initial idea is to copy the { ic; oc; } - and re-use it with Net.connect_uri with a special ctx which will pass the same { ic; oc; } - but on this way, it will be more complex and may be less understandable that this proposed PR.

The best for me is to accept the PR as is if @brendanlong really needs it 😕 .

[mseri]

Thanks for looking into it. I have actually privately heard from other people that would benefit from having this.

@brendanlong if you rebase one last time and add an entry in the CHANGES file I am going to merge

[mseri]

Merging, I need to edit the changes file anyways