Port to Eio and Cohttp #327
Conversation
|
6.0 just shipped! https://github.com/mirage/ocaml-cohttp/releases/tag/v6.0.0 |
| @@ -1,3 +1,3 @@ | |||
| (executable | |||
| (name router) | |||
| (libraries dream)) | |||
| (libraries dream eio_main)) | |||
There was a problem hiding this comment.
Won't eio_main get pulled in transitively by dream? In the same way that lwt.unix was before.
| (** Same as {!Dream.val-response}, but the new {!type-response} is wrapped in a | ||
| {!type-promise}. *) | ||
| (* TODO Remove, or remove response. *) |
There was a problem hiding this comment.
IMHO it would be easier on users to remove response as most like respond is used a lot more.
| @@ -2095,14 +2084,18 @@ val run : | |||
| ?interface:string -> | |||
| ?port:int -> | |||
| ?socket_path:string -> | |||
| ?stop:unit promise -> | |||
| ?stop:unit -> (* TODO What should this be? And fix the docs. *) | |||
There was a problem hiding this comment.
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
| single_read stream destination | ||
| |> Eio.Promise.resolve_ok resolver | ||
| end | ||
| ~flush:(fun () -> chunk_loop ()) |
There was a problem hiding this comment.
This can be just ~flush:chunk_loop
| Log.set_up_exception_hook (); | ||
| ignore user's_error_handler; | ||
|
|
||
| let httpaf_request_handler |
There was a problem hiding this comment.
Should this name be changed?
| in | ||
|
|
||
| (* TODO Value of the backlog argument? *) |
There was a problem hiding this comment.
Seems like backlog should be around 128: https://stackoverflow.com/a/36452474/20371
| match message.kind with | ||
| | Request -> message.server_stream <- Stream.string body | ||
| | Response -> message.client_stream <- Stream.string body | ||
|
|
||
| let set_content_length_headers message = | ||
| (* TODO Restore. |
There was a problem hiding this comment.
Seems like we can use Eio.Promise.peek to do something similar to Lwt.poll here?
| @@ -15,8 +15,6 @@ type stream | |||
|
|
|||
| type buffer = | |||
| (char, Bigarray.int8_unsigned_elt, Bigarray.c_layout) Bigarray.Array1.t | |||
| type 'a promise = | |||
There was a problem hiding this comment.
Ironically, we could probably do type 'a promise = 'a Eio.Promise.t here.
| @@ -70,8 +70,10 @@ let logs_lib_tag : string Logs.Tag.def = | |||
|
|
|||
| (* 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? | |||
There was a problem hiding this comment.
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.
| @@ -383,6 +388,7 @@ let log = | |||
|
|
|||
|
|
|||
| let set_up_exception_hook () = | |||
| (* TODO Is there an Eio equivalent? | |||
There was a problem hiding this comment.
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.
| "multipart_form" {>= "0.4.0"} | ||
| "multipart_form-lwt" | ||
| "ocaml" {>= "4.08.0"} |
There was a problem hiding this comment.
I think we can just remove the OCaml version since Eio will force >5
|
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. |
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.mainand 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.