Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Almost complete port to Eio #254

Draft
wants to merge 44 commits into
base: master
Choose a base branch
from
Draft

Conversation

Willenbrink
Copy link

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.Gluten in gluten/eio/gluten_eio.ml).

Parts that are not yet ported to Eio are:

  • graphql
  • caqti
  • mirage

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.

talex5 and others added 30 commits April 3, 2023 00:03
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).
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
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
@aantron
Copy link
Owner

aantron commented Apr 20, 2023

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:

  • Functions like Dream.html:

    dream/src/dream.mli

    Lines 458 to 462 in edd0cd9

    val html :
    ?status:[< status ] ->
    ?code:int ->
    ?headers:(string * string) list ->
    string -> response promise

    currently return an (already-fulfilled) promise only because the surrounding context typically expects a promise. Once those functions don't require promises, these helpers will become more natural string -> response functions.

  • Functions like Dream.body:

    val body : 'a message -> string promise

    formerly were "true-ish" functions that caused "mostly only co-effects" (speaking imprecisely) and evaluated to 'a promises, the promises, not the functions, stood for the I/O procedures that they would trigger, and the type revealed that I/O would be done. Their new type signatures, like request -> string, make them look like functions and potentially getters, but they are actually procedures that will wait for I/O to complete. This is not reflected in the type signature in any way, and may have to be documented. This is moving documentation out of types and into natural language, a bit of a drawback of this approach.

Willenbrink and others added 5 commits April 20, 2023 14:59
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.
@aantron
Copy link
Owner

aantron commented Apr 21, 2023

@Willenbrink, since you opened this PR, I've cleaned up the Makefile targets in master and upgraded and renamed the vendored libraries so that they are now consistent with master, Mirage, and this PR, so that we can switch between all of these without changing build commands or having to do git submodule update. I've merged these updates into this PR as well. Please pull those in, and you will have to do git submodule update once afterwards.

Some irrelevant bugfixes from master also got in, but there was no reason to exclude them. They don't affect this PR either way. The consistent build system and submodule commits makes this much easier to review.

@aantron
Copy link
Owner

aantron commented Apr 21, 2023

@Willenbrink, the build in this PR causes several warnings, which are errors in a development build. I currently work around that by adding --profile release temporarily to my build command. Is it reasonable to fix them? I understand that some of them may be due to temporarily deleted code or other temporary things during this PR, but any of them that can be fixed early probably should be :)

File "dream/src/http/http.ml", lines 760-774, characters 4-7:
760 | ....begin
761 |       serve_with_maybe_https
762 |         "run"
763 |         ~net:env#net
764 |         ~interface
...
771 |         ?certificate_string:None ?key_string:None
772 |         ~builtins
773 |         user's_dream_handler
774 |     end.
Error (warning 21 [nonreturning-statement]): this statement never returns (or has an unsound type.)
File "dream/src/http/http.ml", line 501, characters 5-21:
501 |     ?certificate_file ?key_file
           ^^^^^^^^^^^^^^^^
Error (warning 27 [unused-var-strict]): unused variable certificate_file.
File "dream/src/http/http.ml", line 501, characters 23-31:
501 |     ?certificate_file ?key_file
                             ^^^^^^^^
Error (warning 27 [unused-var-strict]): unused variable key_file.
File "dream/src/http/http.ml", line 502, characters 5-23:
502 |     ?certificate_string ?key_string
           ^^^^^^^^^^^^^^^^^^
Error (warning 27 [unused-var-strict]): unused variable certificate_string.
File "dream/src/http/http.ml", line 502, characters 25-35:
502 |     ?certificate_string ?key_string
                               ^^^^^^^^^^
Error (warning 27 [unused-var-strict]): unused variable key_string.
File "dream/src/http/http.ml", line 656, characters 6-9:
656 |     ?(tls = false)
            ^^^
Error (warning 27 [unused-var-strict]): unused variable tls.
File "dream/src/http/http.ml", line 685, characters 5-9:
685 |     ?stop
           ^^^^
Error (warning 27 [unused-var-strict]): unused variable stop.
make: *** [Makefile:5: build] Error 1  

Even ignore could do.

@aantron
Copy link
Owner

aantron commented Apr 21, 2023

@Willenbrink, I tried example 6-echo:

$ cd example/6-echo
$ dune exec ./echo.exe --profile release
$ echo -n foo | http POST :8080/echo

This seems to hang indefinitely.

@Willenbrink
Copy link
Author

Willenbrink commented Apr 21, 2023

I understand that some of them may be due to temporarily deleted code or other temporary things during this PR, but any of them that can be fixed early probably should be :)

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 dune-workspace (not included in PR).

I tried example 6-echo:

I might have missed that but I know for a fact that g-upload hangs too (see the mock in test/). I completely missed the tracing capabilities of Eio when reading through the manual so I didn't make much progress with debugging so far. I believe that with the help of CTF it will be quite a bit easier. Especially as you also mentioned that body shouldn't be blocking, changing that might resolve the endless hanging. (EDIT: turning body into a promise does not fix this problem directly. The issue is still present.)

formerly were "true-ish" functions that caused "mostly only co-effects" (speaking imprecisely) and evaluated to 'a promises, the promises, not the functions, stood for the I/O procedures that they would trigger, and the type revealed that I/O would be done. Their new type signatures, like request -> string, make them look like functions and potentially getters, but they are actually procedures that will wait for I/O to complete. This is not reflected in the type signature in any way, and may have to be documented. This is moving documentation out of types and into natural language, a bit of a drawback of this approach.

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 Fiber.forking a fiber for each request. So I think that Eio will unfortunately not expose as much information in the types. But keep in mind that I'm not an expert of Eio, Lwt or Dream so I might be mistaken.

@Willenbrink
Copy link
Author

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).

@aantron
Copy link
Owner

aantron commented Apr 21, 2023

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.

@dinosaure
Copy link
Contributor

I just would like to note that, in anyway, dream will lost the support of MirageOS with this PR. There are currently no real solutions to keep MirageOS support (although we are making efforts to move to OCaml 5) but it is certain that choosing a scheduler like eio will make MirageOS support all the more difficult - as far as I'm concerned, affect or oslo probably have a better proposition that doesn't involve a dependency on the POSIX API. For our part, we usually prefer to remain scheduler-free in order to keep long-term support for our libraries whatever happens to the OCaml ecosystem (and we do this from experience with lwt and async).

@aantron
Copy link
Owner

aantron commented Apr 23, 2023

@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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/on the call stack/on the call stackS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So e.g. let body message = Eio.Promise.await_exn (Message.body message)

@talex5
Copy link

talex5 commented Apr 24, 2023

I just would like to note that, in anyway, dream will lost the support of MirageOS with this PR [...] probably have a better proposition that doesn't involve a dependency on the POSIX API

@dinosaure I think there is a misunderstanding here. Eio has separate Eio and Eio_unix libraries, just like Lwt (with Lwt and Lwt_unix). You can already use Lwt Mirage libraries with Eio on Unix (using the lwt_eio bridge), and we're just waiting for solo5 to support OCaml 5 to get it working with unikernels too.

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

@aantron
Copy link
Owner

aantron commented May 8, 2023

@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 master while we decide how to best merge it.

@Willenbrink
Copy link
Author

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)

@aostiles
Copy link

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:

  [OK]          request          0   with_client.
  [OK]          request          1   method_.
  [OK]          request          2   with_method_.
  [OK]          request          3   target.
  [OK]          headers          0   header.
  [OK]          headers          1   header none.
  [OK]          headers          2   headers.
  [OK]          headers          3   headers empty.
  [OK]          headers          4   has_header.
  [OK]          headers          5   has_header false.
  [OK]          headers          6   all_headers.
  [OK]          headers          7   add_header.
  [OK]          headers          8   add_header duplicate.
  [OK]          headers          9   add_header compares less.
  [OK]          headers         10   drop_header.
  [OK]          headers         11   drop_header absent.
  [OK]          headers         12   with_header.
  [OK]          headers         13   with_header present.

Full test results in `~/dream/_build/default/test/unit/_build/_tests/Dream'.
Test Successful in 0.002s. 18 tests run.
Done: 99% (993/994, 1 left) (jobs: 1)

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.

Copy link
Contributor

@yawaramin yawaramin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good effort. I notice that multiple domains are not started, the server appears to be running in a single domain only. Is this intentional, and multi-domain support is planned for later?

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; ..> ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So e.g. let body message = Eio.Promise.await_exn (Message.body message)

user's_dream_handler =
ignore certificate_file;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ->
Copy link
Contributor

@yawaramin yawaramin Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants