cohttp-eio Client module

[bikallem]

This the client implementation of cohttp eio backend. Roughly, the PR aims to achieve the following (some are optional and/or outside the remit of this PR):

• [x] cohttp-eio client module (Cohttp_eio.Client)
• [x] add client tests, (add/update server tests)
• [x] ~domain name resolving using ocaml-dns (https://github.com/mirage/ocaml-dns/pull/312)~
• [x] ~TLS support so that client can connect to server which mandate https only connection (optional)~

A couple of tasks above are optional since the PRs/dependencies are in other repositories. I will update the task with PRs and status as they are merged/accepted.

EDIT: ocaml-tls and ocaml-dns for cohttp-eio client tls and domain name resolution functionality will be resolved in future PRs when respective PRs to support eio backend are merged.

[mseri]

Please rebase it on master. There seem to be spurious changes from an old eio-3 branch (like the shell-eio..nix file

[bikallem]

Please rebase it on master. There seem to be spurious changes from an old eio-3 branch (like the shell-eio..nix file

Do you have a suggestion on how to go about the rebase? I tried git rebase master and there were lots of conflicts which I couldn't get my head around. So I just merged master on the branch.

[mseri]

I can probably do it in a couple of days if that is ok.

I don’t know if it is the best way, but I usually do it manually by running interactive rebase. For each commit I reset, stage the relevant changes (for partial chunks, should not be the case here, there is gjt add -p). Then I drop everything else with git restore . and continue the rebase

[bikallem]

I managed to rebase on master and remove all the unnecessary commits. git rebase -i master seems to be the trick in this case.

[bikallem]

@mseri The branch is now rebased on master and all todo items are addressed. It is ready for review.

[bikallem]

@mseri The MacOS CI failing seems not related to this PR.

[bikallem]

@talex5 Eio.Client makes do with Eio.Flow.two_way now.

[mseri]

There is some race in the tests:

+++ b/_build/.sandbox/fcaf2ed2da7147d68504b5c372cfd680/default/cohttp-eio/tests/test_client.t.corrected
@@ -1,6 +1,7 @@
 Test Client.get
 
   $ test-server &
+  Fatal error: exception Eio_luv.Luv_error(EADDRINUSE) (* address already in use *)
   $ running_pid=$!
   $ test-client get
   meth: GET
@@ -9,10 +10,13 @@ Test Client.get
   headers: Header { Accept = "application/json"; Host = "localhost:8080" }
 
   $ kill ${running_pid}
+  kill: (44075) - No such process
+  [1]
 
 Test Client.post
 
   $ test-server &
+  Fatal error: exception Eio_luv.Luv_error(EADDRINUSE) (* address already in use *)
   $ running_pid=$!
   $ test-client post
   meth: POST
@@ -25,6 +29,8 @@ Test Client.post
   hello world!
 
   $ kill ${running_pid}
+  kill: (44098) - No such process
+  [1]
 
 Test Client.invalid_uri

[bikallem]

@talex5 'a conn is now ('a, 'b') conn. Since https://github.com/ocaml-multicore/eio/pull/278 is merged, this should allow us to provide a default connection mechanism which takes uri as an argument, i.e. val conn_of_uri: (Uri.t, 'b) conn

[bikallem]

@talex5 @mseri I believe I have addressed all outstanding issues. Could we please merge this if there aren't any further issues; It would help the PR to not bitrot😺.

[mseri]

Can you update the changelog? @talex5 do you have further comments?

[bikallem]

@mseri Added the changelog.

[bikallem]

@talex5

Regarding timeouts, Eio.Time.with_timeout should do it. However, an example might be nice, indeed.

I can't seem to be able to reply inline so creating a comment instead. This is now done in af9e2b5680040bed64c487f228e6ecd8487c1fcb

[bikallem]

@talex5 I think I have addressed all of the latest review comments. Could you please mark them as resolved if so.

[bikallem]

@talex5 It seems I can't reply inline to some comments still.

Yes. I don't see why the comment you linked is a problem for that:

"read_chunked returns an updated set of headers" : just return the unmodified headers in the fixed case "Each chunk may have chunk extensions" : just return an empty set of extension for fixed bodies

I did experiment with it a bit and came up with a possible read function as below:

val read : 
    ?chunk_handler:(Body.chunk -> unit) ->
    Http.Request.t ->
    Eio.Buf_read.t ->
    [`Header Http.Header.t option | `Content string]

Some considerations on the above API:

1. For most uses cases of the library you are likely to use read_fixed, therefore it makes little sense to make the user of the library handle the 'chunked' case in every instance. Likely a user would do the following,

match Cohttp_eio.Client.read req buf_read with
| `Content c -> ... (* do something with [s] *)
| `Header _ -> assert false

This makes the lib tedious and feels not optimized for the usage of the most often use case.

2. In future PRs, I want to add more such specialized reads, such as read_multipart, read_urlencoded to name a few and possibly read_json, read_xml if there is appetite for it. This means the all knowing such read function is going to be less usable and more awkward to use.

The current read API design scales to these requirements and as such it would be preferable to maintain such design property.

[mseri]

I am still not fully convinced about the read, but I think we can merge and keep iterating on the interface

[mseri]

I would like to release an alpha version of the new cohttp in few weeks, this hopefully will bring more feedback from possible users on the table and help us further shaoing the api and the functionalities

[bikallem]

Thanks.