Almost complete port to Eio #254
Conversation
It segfaults under multicore: see savonet/ocaml-ssl#76
This is a proof-of-concept port of Dream to Eio. Most of the public API in dream.mli has been changed to no longer use promises and the main tutorial examples (`[1-9a-l]-*`) have been updated and are working. The documentation mostly hasn't been updated. Internally, it's still using Lwt in many places, using Lwt_eio to convert between them. The main changes are: - User code doesn't need to use lwt (or lwt_ppx) now for Dream stuff. However, the SQL example still uses lwt for the Caqti callback. - Dream servers must be wrapped in an `Eio_main.run`. Unlike Lwt, where you can somewhat get away with running other services with `Lwt.async` before `Dream.run` and relying on the mainloop picking them up later, everything in Eio must be started from inside the loop. Personally, I think this is clearer and less magical, making it obvious that Dream can run alongside other Eio code, but obviously Dream had previously made the choice to hide the `Lwt_main.run` by default. - `Dream.run` now takes an `env` argument (from `Eio_main.run`), granting it access to the environment. At present, it uses this just to start `Lwt_eio`, but once fully converted it should also use it to listen on the network and read certificates, etc. Error handling isn't quite right yet. Ideally, we'd create a new Eio switch for each new connection, and that would get the errors. However, connection creation is currently handled by Lwt. Also, it still tries to attach the request ID to the Lwt thread for logging, which likely won't work. I should provide a way to add log tags to fibres in Eio. Note: `example/k-websocket` logs `Async exception: (Failure "cannot write to closed writer")`. It does that on `master` with Lwt too.
After accepting a connection we convert it to a Lwt_unix.file_descr and continue as before. The `stop` argument has gone, as you can now just cancel the Eio fibre instead. Note that this will cancel all running requests too (unlike the previous behaviour, where it only stopped accepting new connections).
Segfaults due to savonet/ocaml-ssl#76
Just fixes some deprecation warnings.
Anton says he prefers not passing the clock as an argument.
upload.ml uses Lwt_streams, we convert them directly with lwt_eio
This reverts commit 6f608d0.
Using capabilities shouldn't be a big problem? Eio.Path.with_open_dir seems to work quite nicely if needed
Random.initialize isn't called any more. I think its unnecessary as any crypto calls will fail explicitly (due to performing an effect without a handler). I did not remove it as I'm not that familiar with mirage and didn't want to silently break crypto
|
Reviewing the changes in API due to direct style in more detail, a note with a slight objection, as a data point for designing APIs:
|
It was picking up Mirage dependencies. The build target has been updated in master so as to be useful in a Dune workspace and in this PR, without picking up extra dependencies.
|
@Willenbrink, since you opened this PR, I've cleaned up the Some irrelevant bugfixes from |
|
@Willenbrink, the build in this PR causes several warnings, which are errors in a development build. I currently work around that by adding Even |
|
@Willenbrink, I tried example This seems to hang indefinitely. |
Yes, there are still quite a few things left to fix, I didn't have time yet to look into SSL and the examples. Fixing those two should resolve most of the issues with unused variables. I completely forgot about this causing errors as I have simply disabled unused variables causing an error in
I might have missed that but I know for a fact that
I have certainly been a bit too aggressive in making functions blocking. In general, I am under the impression that Eio prefers blocking functions but calling them in their own thread, e.g. by |
|
I am wondering, I just saw that some of the commits no longer have talex5 as the original author. I am fairly certain that I simply rebased the branch and addressed the conflicts. Is this a common issue with rebasing? Is there a way to avoid this besides switching to a merge-based workflow? I expected that git would keep the original commit author (just like the message). |
I noticed this too, but that's a good thing because I am pedantic about authorship and I will add the proper credit to whatever final history we end up with. I've never seen this happen before, but I suggest not to worry about it -- I will make sure @talex5 gets all the proper credit for his commits and contributions and recognition for review as well :) I think we can be a bit sloppy in this PR before the final history rewrite, as it's a big PR and a lot will have to move around before merge. |
|
I just would like to note that, in anyway, |
|
@dinosaure We will definitely consider that in however we merge this. |
| @@ -696,7 +695,7 @@ val all_cookies : request -> (string * string) list | |||
|
|
|||
| (** {1 Bodies} *) | |||
|
|
|||
| val body : 'a message -> string promise | |||
| val body : 'a message -> string Eio.Promise.or_exn | |||
There was a problem hiding this comment.
I don't think we should expose a promise to the API user. We just need a promise at a high level internally inside this function so that concurrent calls to it resolve to the same body without causing concurrent reading of the same ephemeral stream.
There was a problem hiding this comment.
It's fine, I think, for concurrent calls to body to eventually return the same string in direct style, as long as internally it's implemented without racing on the internal streams.
There was a problem hiding this comment.
In other words, before this PR, the body promise was associated with the message, and exposed to the user. With Eio, we can still associate the promise with the message, but hide it from the user using an await. This PR originally disassociated the promise from the message, and created multiple promises, one for each concurrent call to body, on the call stack. This caused racing on the underlying body stream. I'm suggesting to again store a promise inside the message, as before this PR, and await on that promise at the top level inside body.
There was a problem hiding this comment.
s/on the call stack/on the call stackS
There was a problem hiding this comment.
So e.g. let body message = Eio.Promise.await_exn (Message.body message)
@dinosaure I think there is a misunderstanding here. Eio has separate If you have any concerns about using Eio with Mirage, please file an issue on the Eio issue tracker: https://github.com/ocaml-multicore/eio/issues |
|
@Willenbrink, if you don't object, I'm going to "take over" this PR -- that is, fix any more bugs we find in it, etc., and keep it up to date with |
|
Yes, feel free to go ahead. Unfortunately, I'm currently preparing for exams so I don't have as much time as I would like (and will likely not have so for some time) |
|
Hi, I didn't quite get this working, but I reproduced and got around the current CI test failures. I put my changes on top @Willenbrink 's branch here: https://github.com/aostiles/dream/tree/eio. Here is the output I see on my linux box: I manually ran some of the websocket examples and they hung indefinitely. For that and the 993/994 job output (also hangs), I classify my branch as not yet working. |
| @@ -870,7 +869,7 @@ val abort_stream : stream -> exn -> unit | |||
|
|
|||
| (**/**) | |||
| val write_buffer : | |||
| ?offset:int -> ?length:int -> response -> buffer -> unit promise | |||
| ?offset:int -> ?length:int -> response -> buffer -> unit | |||
There was a problem hiding this comment.
Wouldn't it be better to remove deprecated items? Since the Eio change would be a fully breaking change in the API anyway?
| ?tls:bool -> | ||
| ?certificate_file:string -> | ||
| ?key_file:string -> | ||
| ?builtins:bool -> | ||
| ?greeting:bool -> | ||
| ?adjust_terminal:bool -> | ||
| < clock:Eio.Time.clock; net:#Eio.Net.t; secure_random:Eio.Flow.source; ..> -> |
There was a problem hiding this comment.
This new parameter needs to be documented in the doc comment.
| @@ -2214,10 +2213,6 @@ val run : | |||
| - [~interface] is the network interface to listen on. Defaults to | |||
| ["localhost"]. Use ["0.0.0.0"] to listen on all interfaces. | |||
| - [~port] is the port to listen on. Defaults to [8080]. | |||
| - [~stop] is a promise that causes the server to stop accepting new | |||
There was a problem hiding this comment.
The ~stop parameter was not removed, so its comment should be updated, not removed.
| ?loader:(string -> string -> handler) -> | ||
| string -> handler | ||
| ?loader:('a Eio.Path.t -> string -> handler) -> | ||
| 'a Eio.Path.t -> handler |
There was a problem hiding this comment.
Maybe to fit more with Dream style, we could alias it as one of Dream's 'core type aliases', e.g. type 'a path = 'a Eio.Path.t. We are getting rid of 'a promise in this PR so this seems like a reasonable exchange?
| @@ -696,7 +695,7 @@ val all_cookies : request -> (string * string) list | |||
|
|
|||
| (** {1 Bodies} *) | |||
|
|
|||
| val body : 'a message -> string promise | |||
| val body : 'a message -> string Eio.Promise.or_exn | |||
There was a problem hiding this comment.
So e.g. let body message = Eio.Promise.await_exn (Message.body message)
| user's_dream_handler = | ||
| ignore certificate_file; |
There was a problem hiding this comment.
Instead of ignore we can alias the parameter names to drop them directly: ?key_file:_ etc.
| ~error_handler | ||
| ~certificate_file | ||
| ~key_file | ||
| ~builtins | ||
| user's_dream_handler | ||
|
|
||
| | `Memory (certificate_string, key_string, verbose_or_silent) -> | ||
| Lwt_eio.Promise.await_lwt @@ | ||
| Lwt_io.with_temp_file begin fun (certificate_file, certificate_stream) -> |
There was a problem hiding this comment.
We can remove the Lwt use here once Eio allows creating temp files e.g. ocaml-multicore/eio#510 (comment)
EDIT: we can create temp files ourselves now of course, if we have access to Eio.Stdenv.fs i.e. 'the entire filesystem'. E.g. let fs = Eio.Stdenv.fs env in Eio.Path.(fs / Filename.get_temp_dir_name ())
| @@ -728,7 +727,7 @@ module Make | |||
|
|
|||
| (** {1 Bodies} *) | |||
|
|
|||
| val body : 'a message -> string promise | |||
| val body : 'a message -> string Eio.Promise.or_exn | |||
There was a problem hiding this comment.
Presumably same comment here as for Dream.body.
| (fun () -> Lwt.wakeup_later resolver ()); | ||
| promise | ||
| ~close:(fun _code -> Eio.Promise.resolve_error resolver End_of_file) | ||
| ~exn:(fun exn -> Eio.Promise.resolve_error resolver exn) |
There was a problem hiding this comment.
Can eta-reduce: ~exn:(Eio.Promise.resolve_error resolver)
| promise | ||
| ~close:(fun _code -> Eio.Promise.resolve_error resolver End_of_file) | ||
| ~exn:(fun exn -> Eio.Promise.resolve_error resolver exn) | ||
| (fun () -> Eio.Promise.resolve_ok resolver ()); |
This is a continuation of #194 and ports not only the interface but most of the implementation to Eio.
The commits in this PR can largely be reviewed independently although not each commit is buildable.
There is also some renaming necessary in the vendor directory, not included in this PR as they are individual submodules. The changes boil down to renamed modules in the Eio variants (e.g.
module Gluten = Dream_gluten.Gluteningluten/eio/gluten_eio.ml).Parts that are not yet ported to Eio are:
Furthermore, ssl is disabled right now.
There is also one bug I've been unable to locate. A test showing this failure is in
test/mock/g-upload. I've tried for some time but unfortunately do not have enough knowledge of httpaf, gluten, multipart-forms etc. to locate the error. I will likely try fixing it myself over the next week or so.