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):

• cohttp-eio client module (Cohttp_eio.Client)
• add client tests, (add/update server tests)
• domain name resolving using ocaml-dns (Add eio backend for ocaml-dns client ocaml-dns#312)
• 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.

[patricoferris]

Thanks for this @bikallem, I'm using a slightly modified version over in https://github.com/geocaml/carbon-intensity :))

[bikallem]

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

[talex5]

At the moment, get, post, etc take a URI and an open-ended set of resources that might be needed to connect to it. This is not very flexible, since it requires cohttp to define all possible connection methods. I think we should split this into two parts:

1. A plain HTTP API where you pass in an already-connected Eio.Flow.two_way (plus the URI, for e.g. the "host" header).
2. A high-level API for resolving URIs to resources. This will know about networks, TLS, DNS, etc. It might follow redirects too.

It's not clear to me that (2) should even be in this library, as it should also handle other protocols too (file://, ftp://, gemini://, etc). In Python, the urllib.request module handles that.

Perhaps we should put a Resolver in Eio, and let cohttp populate it, e.g. something like:

let r = Eio.Resolver.make () in
Resolver.register r "http"  @@ Cohttp_eio.http_resolver env;
Resolver.register r "https" @@ Cohttp_eio.https_resolver env;
let data = Resolver.resolve r uri in

There could also be a convenience function to do this in one go, of course.

[bikallem]

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

[bikallem]

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

[talex5]

Thanks! I made a few comments on the interface inline.

[talex5]

The 'a isn't used here. But this would work (avoids the user having to cast things).

Suggested change

	type 'a body_disallowed_call =
	?version:Http.Version.t ->
	?headers:Http.Header.t ->
	Eio.Switch.t ->
	Eio.Flow.two_way ->
	resource_path ->
	response
	type 'a body_disallowed_call =
	?version:Http.Version.t ->
	?headers:Http.Header.t ->
	(#Eio.Flow.two_way as 'a) ->
	resource_path ->
	response

[bikallem]

Done

[talex5]

@talex5 do you have further comments?

I found that some of the previous comments had been marked as resolved accidentally, so I unresolved them (it seems that GitHub lets you mark other people's comments as resolved for some reason).

I think it's mostly OK, but we should separate out connections from requests. Adding an example that sends two requests over one connection would probably make things clearer.

[talex5]

Suggested change

	(** [('a, 'b conn)] is [(resource_path, host, flow)]. [flow] is the Eio flow
	(** [('a, 'b) conn] is [(resource_path, host, flow)]. [flow] is the Eio flow

However, this description does not match the type, which is still (strangely) a function.

[bikallem]

I have removed 'a conn now. The are now passed directly to call as parameters. 7599e46

[talex5]

Suggested change

	('a, 'b) conn ->
	'a ->
	'a conn ->

There is no point taking a function and its input here, as you just call it once immediately in all cases. It should just take the connection directly.

[bikallem]

Done in 09c2d01

[talex5]

Taking resource_path here means that every time you want a different resource, you also need to open a new connection. Why not separate out the connection from individual requests?

[bikallem]

Done in 09c2d01

[talex5]

Two common questions I had in the past are how to timeout requests and how to connect to unix domain sockets. Since with ocaml5+eio it is done differently, I think we should document it either in the interface, in an example to link to the README or in the README, so that we avoid answering the same questions all the time.

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

[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 af9e2b5

[bikallem]

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

[talex5]

Thanks. There are things can be still be improved here, but I don't see any problem with merging it as it is and then working on the main branch.

[talex5]

This is minor, but it might be worth putting the body argument first. Then it could be defined more simply as:

Suggested change

	type 'a body_allowed_call =
	?version:Http.Version.t ->
	?headers:Http.Header.t ->
	?body:Body.t ->
	conn:(#Eio.Flow.two_way as 'a) ->
	host ->
	resource_path ->
	response
	type 'a body_allowed_call = ?body:Body.t -> 'a body_disallowed_call

(possibly renaming the types for clarity)

[bikallem]

Yes, I did consider such a scheme but found that when reading such types it read as ... so a body_allowed_call is also a body_disallowed_call. It didn't quite roll off the tongue for me so I left it as it is even though it is a bit repetitive. Also I didn't want to introduce another level of indirection since the types are mostly for alias and clarity for the values below, i.e get, post, head, etc.

[talex5]

Do you mean convert read data into Body.chunk ? Possibly could work. Let me see.

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

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

[talex5]

Don't treat all errors (e.g. not-an-integer) as a missing content-length:

Suggested change

	let read_fixed ((response, reader) : Http.Response.t * Buf_read.t) =
	match
	Http.Header.get response.headers "Content-Length"
	|> Option.get
	|> int_of_string
	with
	| content_length -> Buf_read.take content_length reader
	| exception _ -> Buf_read.take_all reader
	let read_fixed ((response, reader) : Http.Response.t * Buf_read.t) =
	match Http.Header.get response.headers "Content-Length" with
	| Some content_length -> Buf_read.take (int_of_string content_length) reader
	| None -> Buf_read.take_all reader

[bikallem]

Shouldn't an invalid content-length value(integer) be understood as an invalid Content-Length header and be treated as such? Specifically,

5. If a valid Content-Length header field is present without
   Transfer-Encoding, its decimal value defines the expected message
   body length in octets. If the sender closes the connection or
   the recipient times out before the indicated number of octets are
   received, the recipient MUST consider the message to be
   incomplete and close the connection.

I assumed the "invalid" in a broader sense, not just missing 'Content-Length' header. So this would be invalid: Content-length: abc since abc can't be a valid integer value in OCaml.

[talex5]

That's an interesting point. https://www.rfc-editor.org/rfc/rfc7230#section-3.3.2 says:

Any Content-Length field value greater than or equal to zero is valid.

I think it's assuming that the value will always be an integer, though it's not clear. It's also unclear why negative values should be ignored - possibly some systems use that to indicate streaming?

The older https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html says:

When a Content-Length is given in a message where a message-body is allowed, its field value MUST exactly match the number of OCTETs in the message-body. HTTP/1.1 user agents MUST notify the user when an invalid length is received and detected.

[bikallem]

Thanks for the pointers. They have been very helpful. I have created issues based on our conversation so far.
#927
#926

I want to address them in PRs after this.

[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.