Port to Eio and Cohttp by aantron · Pull Request #327 · camlworks/dream

aantron · 2024-07-03T21:03:30Z

This is a messy draft port of Dream to Eio using cohttp-eio. On HTTP/1, it works better than the previous port attempts (#254, #194) and should be more maintainable, including because cohttp-eio offers a direct-style interface (the advantages of which this port still needs to propagate further into the Dream code), but a lot of features will be missing even after the port is complete -- HTTP/2 support, GraphQL, and others, because the corresponding libraries are not ported to Eio or not available with Cohttp. A lot of features, like Caqti integration, that will be available when this port is done, are temporarily disabled for now, and will be added back as the port progresses. WebSockets will require additional work.

The current status of the port is that I am working through various deadlocks and corner cases of using Eio "promises" together with fibers, which are caused by the existing code of Dream assuming a sort of promise-callback soup, where any callback can be called on the one big stack under Lwt.main and make progress, while with fibers and Eio blocking the wrong fiber to wait on a promise can block a stack that would have been used to make progress to resolve that promise. The semantics are therefore substantially different. This was a major problem with the approach in #194 and #254. It is substantially negated by thoroughly using direct style wherever possible, rather than promises or callbacks.

Due to over-confident naming of top-level module Http from Cohttp.

smorimoto · 2024-11-22T07:58:40Z

6.0 just shipped! https://github.com/mirage/ocaml-cohttp/releases/tag/v6.0.0

yawaramin · 2024-07-15T05:23:54Z

example/3-router/dune

 (executable
 (name router)
- (libraries dream))
+ (libraries dream eio_main))


Won't eio_main get pulled in transitively by dream? In the same way that lwt.unix was before.

yawaramin · 2024-12-23T05:04:08Z

src/dream.mli

+    string -> response
 (** Same as {!Dream.val-response}, but the new {!type-response} is wrapped in a
    {!type-promise}. *)
+(* TODO Remove, or remove response. *)


IMHO it would be easier on users to remove response as most like respond is used a lot more.

yawaramin · 2024-12-23T05:48:37Z

src/dream.mli

  ?port:int ->
  ?socket_path:string ->
-  ?stop:unit promise ->
+  ?stop:unit -> (* TODO What should this be? And fix the docs. *)


Looks like Cohttp_eio is using 'a Eio.Promise.t and returning the 'a: https://github.com/mirage/ocaml-cohttp/blob/716d6e66be35ed11967f3984ace27c1fa327b8ed/cohttp-eio/src/server.mli#L19

yawaramin · 2024-12-24T03:37:04Z

src/http/driver.ml

+                single_read stream destination
+                |> Eio.Promise.resolve_ok resolver
+            end
+            ~flush:(fun () -> chunk_loop ())


This can be just ~flush:chunk_loop

yawaramin · 2024-12-24T03:44:43Z

src/http/driver.ml

-    Log.set_up_exception_hook ();
+  ignore user's_error_handler;
+
+  let httpaf_request_handler


Should this name be changed?

yawaramin · 2024-12-24T04:00:41Z

src/http/driver.ml

+        address
  in

+  (* TODO Value of the backlog argument? *)


Seems like backlog should be around 128: https://stackoverflow.com/a/36452474/20371

yawaramin · 2024-12-24T04:23:01Z

src/pure/message.ml

  | Response -> message.client_stream <- Stream.string body

 let set_content_length_headers message =
+  (* TODO Restore.


Seems like we can use Eio.Promise.peek to do something similar to Lwt.poll here?

yawaramin · 2024-12-24T04:25:27Z

src/pure/stream.mli


 type buffer =
  (char, Bigarray.int8_unsigned_elt, Bigarray.c_layout) Bigarray.Array1.t
-type 'a promise =


Ironically, we could probably do type 'a promise = 'a Eio.Promise.t here.

yawaramin · 2024-12-24T04:34:04Z

src/server/log.ml


 (* Lwt sequence-associated storage key used to pass request ids for use when
   ~request is not provided. *)
+(* TODO Is there an equivalent mechanism with Eio?


Looks like yes: https://ocaml-multicore.github.io/eio/eio/Eio/Fiber/index.html#fiber-local-variables

Fiber-local variables are particularly useful for attaching extra information for debugging, such as a request ID that the log system can include in all logged messages.

yawaramin · 2024-12-24T04:38:27Z

src/server/log.ml



 let set_up_exception_hook () =
+  (* TODO Is there an Eio equivalent?


I think no, because to my understanding every fiber in Eio propagates its exceptions upwards to its parent fibers and eventually up to the root fiber.

yawaramin · 2024-12-24T19:32:40Z

dream.opam

+  "mirage-crypto-rng-eio"
  "multipart_form" {>= "0.4.0"}
-  "multipart_form-lwt"
  "ocaml" {>= "4.08.0"}


I think we can just remove the OCaml version since Eio will force >5

yawaramin · 2024-12-26T04:13:49Z

Trying to see if I can get this branch to run. There's been some bitrot, especially the Mirage folks have gotten rid of cstruct and moved to string/bytes. Hacking and slashing now...

EDIT: pushing my changes to https://github.com/yawaramin/dream/tree/cohttp-eio

The examples are running now. Will play around with it a bit more.

kognise · 2025-02-20T23:05:37Z

I think this is super important, is there any way I can help speed up the work to port dream to eio?

clauverjat-h · 2025-03-04T13:45:11Z

Hello @yawaramin

I tried to play with your fork, but I got the following error

Context: _private                        
File "src/http/dune", line 11, characters 2-10:
11 |   eio_main
       ^^^^^^^^
Error: Library "eio_main" not found.
-> required by library "dream.http" in _build/default/src/http
-> required by _build/default/META.dream
-> required by _build/install/default/lib/dream/META
-> required by _build/default/dream.install
-> required by alias install

To fix the issue I needed to replace eio_main by eio in src/http/dune

Note: I edited the message, I had other issues before but they were due to some misunderstanding on my part regarding dune package management, so nothing to do with your code.

Willenbrink · 2025-05-18T14:53:49Z

Just want to put out there that I have fixed some of the issues in yawaramin's fork which can be found here: https://github.com/Willenbrink/dream/tree/cohttp-eio.

All the tests pass and the examples we looked at work too. I've also rebased the changes on top of the additional tests I've written in #396 which also pass.

yawaramin · 2025-05-18T15:17:02Z

@Willenbrink great work! Checking out your fork.

aantron added 4 commits May 30, 2024 13:13

Rename http.ml to driver.ml

bbcabba

Due to over-confident naming of top-level module Http from Cohttp.

WIP

907614a

WIP

64daabf

WIP

a532c4a

aantron added 🎓 study 💔 breaking labels Sep 4, 2024

yawaramin reviewed Dec 24, 2024

View reviewed changes



		let set_up_exception_hook () =
		(* TODO Is there an Eio equivalent?

Uh oh!

Conversation

aantron commented Jul 3, 2024

Uh oh!

smorimoto commented Nov 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yawaramin commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kognise commented Feb 20, 2025

Uh oh!

clauverjat-h commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Willenbrink commented May 18, 2025

Uh oh!

yawaramin commented May 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

yawaramin commented Dec 26, 2024 •

edited

Loading

clauverjat-h commented Mar 4, 2025 •

edited

Loading