Skip to content

RFC : Port cohttp-lwt-unix to eio using lwt_eio #854

Closed
bikallem wants to merge 4 commits intomirage:masterfrom
bikallem:lwt-eio
Closed

RFC : Port cohttp-lwt-unix to eio using lwt_eio #854
bikallem wants to merge 4 commits intomirage:masterfrom
bikallem:lwt-eio

Conversation

@bikallem
Copy link
Copy Markdown
Contributor

This is a WIP port of cohttp-lwt-unix to eio using the lwt_eio (https://github.com/talex5/lwt_eio/) library. lwt_eio is an eio backend for lwt and allows existing lwt libraries to used together with eio.

Asides:

  1. Mirage and jsoo packages are disabled for now since both mirage and jsoo don't yet have eio backend(AFAICT).
  2. This resulting API lives in cohttp-lwt-unix and it is not backwards-compatible with existing codebases. Should this live off new package altogether?
  3. I haven't migrated tests just yet.

The gist of the PR is the new Cohttp_lwt_unix.Client and Cohttp_lwt_unix.Server API which doesn't expose any 'a Lwt.t in the module interface signature. This allows a direct style of coding where the lwt monad and its artefacts(>>=, >|=, bind, map etc) are less pronounced, visible or necessary.

The purpose of this Draft PR is to get a feeler for what the maintainers think of this PR? Is there an appetite for this work?

Bikal Lem added 4 commits January 24, 2022 15:44
(cherry picked from commit b0d6607)
(cherry picked from commit 65f2dd9)
Convert Cohttp_lwt.Client and Cohttp_lwt.Net modules to direct
style API.
@mseri
Copy link
Copy Markdown
Collaborator

mseri commented Jan 28, 2022

Wow, thanks a lot. I will need a few day to look at it and play with it. From my point of view is great to have this, but will require some extra thinking. For example, from the top of my head, this would make it only available on ocaml 5.0+, right?

@bikallem
Copy link
Copy Markdown
Contributor Author

bikallem commented Jan 28, 2022

this would make it only available on ocaml 5.0+, right?

Correct. Also mirage and jsoo packages will be unavailable since jsoo and mirage don't yet support ocaml 5.0 effects.

@avsm
Copy link
Copy Markdown
Member

avsm commented Jan 28, 2022

Thanks for this @bikallem! As a counterpoint, do you also have the WIP port of cohttp directly to eio? (without Lwt_eio). That would perhaps give us a feel for what a totally separate implementation of cohttp without Lwt looks like.

@mseri
Copy link
Copy Markdown
Collaborator

mseri commented Jan 28, 2022

If needed, I have a fork of cohttp and conduit lying around using only unix (mostly worked but was single threaded still, I have never found the time to fix it)

@bikallem
Copy link
Copy Markdown
Contributor Author

Thanks for this @bikallem! As a counterpoint, do you also have the WIP port of cohttp directly to eio? (without Lwt_eio). That would perhaps give us a feel for what a totally separate implementation of cohttp without Lwt looks like.

Sure, I will create another PR with pure eio cohttp as a counter point.

@smorimoto
Copy link
Copy Markdown
Contributor

Am I the only one who is curious about how eio performs under cohttp as well?

@mseri
Copy link
Copy Markdown
Collaborator

mseri commented Jan 30, 2022

No. I can’t wait to test it!

@bikallem
Copy link
Copy Markdown
Contributor Author

bikallem commented Feb 1, 2022

I have opened a draft PR for eio-only cohttp - #857

@rgrinberg
Copy link
Copy Markdown
Member

I have opened a draft PR for eio-only cohttp

Thanks. To proceed with this PR, I think we need a separate cohttp-lwt-eio package. It's not possible to break code for everyone who is using the current packages.

@avsm
Copy link
Copy Markdown
Member

avsm commented Feb 2, 2022

Let's focus on getting #857 in first. This adapter will be easier to figure out once we have the pure eio version in

@bikallem
Copy link
Copy Markdown
Contributor Author

closing this since #857 adds cohttp-eio

@bikallem bikallem closed this Jun 15, 2022
@bikallem bikallem deleted the lwt-eio branch November 9, 2022 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants