Renovate Server/Client API

[bikallem]

This PR renovates cohttp-eio Client and Server API.

Summary of changes:

1. Header - adds pretty printer using easy-format to make tests a bit more readable. http doesn't allow easy-format dependency.

2. Buf_read/Buf_write
   Split readers/writer from Rwer module into Buf_read and Buf_write respectively. The Rwer module is now removed.

3. Body/Chunked_body
   HTTP chunked encoding in now split into a separate module Chunked_body. Body defines two new types - Body.reader/writer. reader is used to read client response and server request bodies. Conversely, writer is used to write client request and server response bodies. Body also defines a special writer none - a no-op - that is used to denote a non-existing http response/request body. This is used in GET/HEAD method requests to denote these request types don't have a body.

   The body also provides a couple of frequently used reader and writers,

   • Body.content_reader/content_writer - This is analogous to earlier Fixed body adapted to the new reader/writer semantics.
   • Body.form_values_reader/form_values_ writer This allows reading/writing form values in request/response.

   Chunked_body - This module encapsulates HTTP chunked encoding functionality in a reader/writer semantics similar to content/form-values.

   The reader/writer is designed to be extendable such as that specialized reader/writer outside of the cohttp-eio package can be used with the package modules. Future PRs should add json and xml reader/writers.

4. Method
   The new Method module is added to make Client.call/get/post/push more type-safe (less stringly types) and semantics (HTTP) safe. Previously, Client.call still allowed client requests to have body even if request methods semantics didn't allow it, for eg. GET, HEAD, DELETE, CONNECT methods. This is no longer allowed by the type checker since choosing either of those methods results in Body.none client request body.

5. Request
   Request module defines two request types - client and server. The two types encapsulates the differences between the two use cases:
   client request types require host/port to be present while server doesn't. client request body needs to be written while the server request type needs to be read and so on.

6. Response
   Similar to Request module, Response module defines and encapsulates the difference between the two use-cases: client response types are readers and can be closed while server response types are writers and there is no corresponding close functionality.

7. Client
   Client module is renovated to address a few user feedback/issues (Cohttp-eio: "handle used after calling close" while reading body / Closing connection too soon? #965) and introduce some new functionality. Client.call/get/head/post are now more convenient to use and removes the - redundant - need to specify request host information more than once. This new functionality is enabled by Method/Request/Response module renovations themselves.

   Also, note that Client module allows additional modes of operation where a connection to a host/port is cached/reused. Future PRs will further this functionality to introduce idle connection management and the like. This change also lays the ground work for further work on the Client module, namely redirection handling and cookie jar functionality.

   The former functionality of user being able to provide connection is also preserved albeit without the connection management functionality of the other mode.

8. Server
   Server is renovated to use the new Request/Response/Body modules. The renovation also addresses the feedback from users (cohttp-eio: Document Server.run #961). Future PRs should add server shutdown functionality.

9. Testable
   The renovation also makes modules/types in cohttp-eio more testable and less reliant of OS/environment details such as network/clocks etc. All modules/functions now have corresponding unit tests. The tests are fully mdx so they additionally serve as example API usage.

NOTE to reviewers
The commits follow the natural progression of the PR. They can be reviewed commit by commit and each commit always pass the tests.

/cc @talex5 @mseri @avsm

[patricoferris]

Took a look over this @bikallem, whilst I appreciate the effort into keeping commits reviewable and separate, I'm not sure it's going to be the most productive way to get reviews given the Github UI and I think we'll end up with lots of comments and reviews and it'll get a little unmanageable and confusing (I do this all the time on Eio, then usually @talex5 is the voice of reason and I break up the PR and things go much smoother!). For example, you mentioned that this issue is taken into account and I'm not even sure where to begin checking presumably 2db1420 but it's kinda mixed with all of the other changes too. I think a separate PR would make it easier to review and quicker to merge. The same goes for this bug fix #965

That being said, I think it is super useful to have this as a bit of an end-to-end test of what a new API would look like. Accompanied with a more real-world example would help convince people of the API design too :)) What do you think ?

[bikallem]

Took a look over this @bikallem, whilst I appreciate the effort into keeping commits reviewable and separate, I'm not sure it's going to be the most productive way to get reviews given the Github UI and I think we'll end up with lots of comments and reviews and it'll get a little unmanageable and confusing (I do this all the time on Eio, then usually @talex5 is the voice of reason and I break up the PR and things go much smoother!). For example, you mentioned that #961 is taken into account and I'm not even sure where to begin checking presumably 2db1420 but it's kinda mixed with all of the other changes too. I think a separate PR would make it easier to review and quicker to merge. The same goes for this bug fix #965

@patricoferris Thanks. Yes, I can certainly break the PR into smaller PRs. I was a bit hesitant in doing so since that could make the review process miss "the forest for the trees". But you are right, I think we need both - this PR to give some overall view/sense of where all the PRs are heading to and other smaller PRs for easy review.

That being said, I think it is super useful to have this as a bit of an end-to-end test of what a new API would look like. Accompanied with a more real-world example would help convince people of the API design too :)) What do you think ?

That is certainly the plan, i.e. have a sample web app at the end. However, I would say this PR is a start.

[talex5]

I've been thinking about this, and particularly @bikallem's comment on Slack that

I think the advantage with cohttp-eio is that it is not encumbered by backwards-compatibility requirements/concerns or legacy code. Therefore it can be a lot more adventurous with the API than the other packages.

It seems to me that there's a lot of tension between a) trying to provide a stable cohttp-style API, and b) experimenting with better designs.

So I suggest that:

1. We move cohttp-eio out of this repository and rebrand it. It already has very little connection with cohttp. Then we can use that to iterate quickly on new APIs, without a lengthy review process.
2. cohttp 6.0 can be released without Eio support if desired.
3. At some point, we take the existing Lwt code, do a line-by-line translation to Eio, and call that cohttp-eio. Its goal would be to remain close to the Lwt API, with the minimal changes needed to work with Eio.

At the moment, anyone with cohttp-lwt code who wants to use effects must both switch to Eio and switch to a very different API at the same time. It would be easier if they could first switch to an Eio version of cohttp, and then switch to an alternative system in a second step.

This would also allow the new project to add extra features, such as support for web middleware, which had to be dropped from the current cohttp-eio.

What do people think about that?

[bikallem]

So I suggest that:

We move cohttp-eio out of this repository and rebrand it. It already has very little connection with cohttp. Then we can use that to iterate quickly on new APIs, without a lengthy review process.
cohttp 6.0 can be released without Eio support if desired.
At some point, we take the existing Lwt code, do a line-by-line translation to Eio, and call that cohttp-eio. Its goal would be to remain close to the Lwt API, with the minimal changes needed to work with Eio.

@talex5 Agree.

[mseri]

It makes perfect sense, although it would be nice to keep them somehow coordinated: during the current development of cohttp-eio we got lot of improvements in http and cohttp as a byproduct

[avsm]

I'm all in favour of moving cohttp-eio out of this repository and having some more breathing room to iterate on interfaces. In general, in fact, I'm in favour of a monorepo for porting protocol libraries to Eio so that a more joined-up approach can be taken. My own experiments (for a research project, so not for general consumption) are in https://github.com/avsm/eeww, and I'm finding it a nice way to make more cross-cutting changes with little package management hassle.

When things stabilise, it'll be pretty straightforward to extract upstreamable PRs from the resulting diffs.

[bikallem]

Update:
Just to give a heads up. Here is the next "cohttp-eio" that I am working towards. https://github.com/bikallem/spring, it is not yet a 0.1 complete but most of the functionality in cohttp-eio are already implemented. I will add a few more functionalities+examples+documentation on top of it before it being released as 0.1. Feedback welcome.

[dangdennis]

Update:
Just to give a heads up. Here is the next "cohttp-eio" that I am working towards. https://github.com/bikallem/spring, it is not yet a 0.1 complete but most of the functionality in cohttp-eio are already implemented. I will add a few more functionalities+examples+documentation on top of it before it being released as 0.1. Feedback welcome.

Do you plan to rename and/or move spring into some eio monorepo as an effort to stabilize http and other protocols around eio?

I’m updating disml, a discord lib, to eio. Aside from swapping out Async, I require vbmithr/ocaml-websocket#130. I can use this aforementioned PR, update it to spring, then be able to use websocket in disml. I’d like to minimize the amount of patching and vendoring possible, but understand the nest of dependencies present.

I can move this discussion into spring itself if needs be.

edit: I’ll also vendor eio to get websocket-eio to work for me to avoid having to update @patricoferris’s PR

[bikallem]

Do you plan to rename and/or move spring into some eio monorepo as an effort to stabilize http and other protocols around eio?

Most likely it will not be renamed/moved to another repo.

[bikallem]

Closing this.