Eio backend using stock parsing, serialization and signatures from Cohttp.

[mefyl]

Context

While trying out cohttp-eio, I found that all requests handler where blocking forever when trying to exhaust request or response bodies. After some digging, it appeared that - if I'm not mistaken - the current implementation simply forwards the raw socket as the body, not limiting the content length and potentially exposing chunked encoding, which is not the behavior of the other cohttp APIs.

While digging around, I found this PR discussion suggesting that cohttp-eio should provide an interface as close as other cohttp APIs instead, and I gave it a shot.

Implementation

This implementation relies mostly around providing a Cohttp.S.IO implementation suitable for eio, and relies mostly on the generic parser and serialization from the root Http and Cohttp libraries to do the heavy lifting - like cohttp-async does and cohttp-lwt partly does. This provides a working client and server with relatively little code and could IMO provide maximum consistency between the different cohttp APIs.

I have also experimented with generating a client compliant with a generic signature from a minimal request interface. I'm confident a generic client signature could be extracted into Cohttp, parameterized by its Cohttp.S.IO, and all APIs could generate a client compliant with that interface quite easily using this process - similarly to what cohttp-lwt does with its Cohttp_lwt.S.Client and Cohttp_lwt.S.Server.

Apart from this, the main difference is that request and response bodies are Eio.Flow.{source,sink} that entirely abstract the content-length and encoding. Iow, one can eagerly consume a whole body, and the flow will dry up at some point when everything has been consumed. The user never has to worry about content encoding, like in the async and lwt versions.

Status

This PR is more of an RFC regarding this approach before going any further. It rewrites the content of cohttp-eio/ but could perfectly live next to it, I just had no better idea baring cohttp-eio-2/.

Clients and servers are fully functional, but not heavily tested or very featureful - connection keepalive, multiple domains, ... I suppose it would be good to extract a common interface before making opinionated decisions over the interface. @talex5 suggests it should mimic the cohttp-lwt API, and I'd be happy to take a stab at it.

All in all, eager for any feedback and to possibly push this further if the approach is deemed beneficial.

[mefyl]

I actually went ahead and :

• Extracted generic signatures for clients and servers from cohttp-lwt to cohttp
• Constrained Cohttp_lwt.S.Server and Cohttp_lwt.S.Client to these interfaces.
• Migrated my previous work in cohttp-eio to honor these interfaces.
• Added Cohttp.Client.Make () to factor some clients boilerplate generation from a minimal set of functions (ie. generate get, put etc. from call) and used that in both lwt and eio.

IMO these interfaces could really use some facelift, but for now I strictly adhered to them to separate concerns. While cohttp-lwt was touched, it should just be code being moved around and it should behave exactly the same.

[talex5]

I only skimmed this, but seems reasonable. Some minor suggestions in-line.

[adatario]

I very much like these changes!

This should make ocaml-websocket work with cohttp-eio. Some WIP experiments: https://github.com/adatario/eio-ws/blob/main/src/main.ml#L37-L99.

The only change required would be to expose the IO module (adatario@41f5f10). Maybe that could be added to this PR?

[talex5]

It looks like this PR is also missing TLS support. e.g.

open Eio.Std

let net =
  let net = Eio_mock.Net.make "mocknet" in
  Eio_mock.Net.on_getaddrinfo net [`Return [`Tcp (Eio.Net.Ipaddr.V4.loopback, 443)]];
  let conn = Eio_mock.Flow.make "connection" in
  Eio_mock.Net.on_connect net [`Return conn];
  net

let () =
  Eio_main.run @@ fun env ->
  Mirage_crypto_rng_eio.run (module Mirage_crypto_rng.Fortuna) env @@ fun () ->
  let client = Cohttp_eio.Client.make net in
  Switch.run @@ fun sw ->
  let resp, _body = Cohttp_eio.Client.get client ~sw (Uri.of_string "https://www.example.org") in
  traceln "status: %S" (Cohttp.Code.string_of_status resp.status);
  ()

outputs the request in plain text:

+mocknet: getaddrinfo ~service:https www.example.org
+mocknet: connect to tcp:127.0.0.1:443
+connection: wrote "GET / HTTP/1.1\r\n"
+                  "host: www.example.org\r\n"
+                  "user-agent: ocaml-cohttp/\r\n"
+                  "\r\n"
+connection: closed
Fatal error: exception Failure("connection closed by peer")

Probably Cohttp_eio.Client.make should let you pass in a (Uri.t -> Eio.Net.stream_socket_ty r) function.

[mseri]

I was coming to leave a similar comment, after somebody pointed at #951 in discord (https://discord.com/channels/436568060288172042/439062744105484288/1152047945731018782).

We should also update the client example to show how to make calls with TLS.

[talex5]

I'll have a go at TLS support.

[talex5]

I've pushed a branch with TLS support here: https://github.com/mefyl/ocaml-cohttp/compare/eio...talex5:ocaml-cohttp:eio-tls?expand=1

[mefyl]

WDYT about making the https argument of Client.make optional, to avoid all the ~https:None ?

[mseri]

I agree with you, I am also not convinced by the https name of the parameter. Perhaps https_authenticator, authenticator, tls_authenticator, something pointing that this is what will check the certificates? I think we should also be more explicit on what https does in the docstring and that the default is not checking anything.

Can we also have an example showing how to create a meaningful authenticator?

[talex5]

WDYT about making the https argument of Client.make optional, to avoid all the ~https:None ?

I don't have a strong opinion, but I thought it was better to remind users that they need to decide whether they want HTTPS support or not, and where to put it if so.

I agree with you, I am also not convinced by the https name of the parameter. Perhaps https_authenticator, authenticator, tls_authenticator, something pointing that this is what will check the certificates?

It's not that though, it's the thing that wraps a connection for https. Authentication is only part of what it does. We can't just take an authenticator here and do the TLS bit in the library because then cohttp-eio would have to depend on a TLS library.

I expect that eventually there will be a separate library to combine cohttp-eio with tls-eio (cohttp-eio-tls?). In the Lwt world, conduit handles this.

[mseri]

I don’t have too strong of an opinion, I think the optional is cleaner but perhaps https is so pervasive nowadays that it is good to keep it explicit instead.

Thanks for the context, then I am ok in keeping the name provided that we explain a bit more its role.

We can't just take an authenticator here and do the TLS bit in the library because then cohttp-eio would have to depend on a TLS library.

I expect that eventually there will be a separate library to combine cohttp-eio with tls-eio (cohttp-eio-tls?). In the Lwt world, conduit handles this.

I understand that and I think it is a good place here to experiment with explicit tls management, it is something that we discussed to do in cohttp as well recently and I think we are moving in the same direction there as well. It will also solve the issue of hidden failures when 5e right ssl/tls libraries are not present.

[mefyl]

Shall we make the parameter optional and merge this as-is then ? Or if we don't want to commit to an interface yet, we could merge it as-is and open a separate PR to add TLS support.

[mseri]

Yes, let's go on with 2 PRs. Can you fix the conflict? I am going to merge this and then we can discuss TLS and the API separately

[mseri]

I aim at merging this and mefyl/ocaml-cohttp@eio...talex5:ocaml-cohttp:eio-tls as they are now though (perhaps with some more explanations on how to do tls properly), release a new beta release and see if there are comments

[talex5]

Note: tls-eio with Eio 0.12 support has now been released, so I uncommented the https example in my branch.

[mseri]

Almost there, some test dependencies are missing from the opam file:

 File "cohttp-eio/tests/dune", line 3, characters 12-20:
3 |  (libraries alcotest cohttp-eio eio eio.mock eio_main logs.fmt)
                ^^^^^^^^
Error: Library "alcotest" not found.
-> required by _build/default/cohttp-eio/tests/test.exe
-> required by alias cohttp-eio/tests/all
-> required by alias cohttp-eio/default
File "cohttp-eio/tests/dune", line 6, characters 7-15:
6 |   (pps ppx_here)))
           ^^^^^^^^
Error: Library "ppx_here" not found.
-> required by _build/default/cohttp-eio/tests/test.pp.ml
-> required by alias cohttp-eio/tests/all
-> required by alias cohttp-eio/default
Error: Process completed with exit code 1.

[mseri]

Sorry for the delay. This looks good to me. Thanks @mefyl @talex5 and @adatario.
I think we can merge this and iterate over any necessary change from that