aantron
Almost complete port to Eio
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.
Amazing, thank you! I will review this gradually and I expect that I will learn a lot from it!
@Willenbrink, I'm getting ready to build this PR locally. I've found your multipart_form-eio package/branch from the PR in the multipart_form repo. I'm assuming you've done a renaming in Gluten_eio, etc., to refer to Dream_gluten.Gluten, etc. locally to get this to build. Do you have these branches uploaded somewhere? I suppose if it's too annoying, I can do the renaming myself, but I'd like to ping you first, in case you already have them or can make them available without too much of a pain.
Sorry, have been busy the past few days. I did not published the changes but I guess that's resolved now anyway.
I'm disappointed that the examples have become unnecessarily awkward. I need to do some experiments to see what actual user code will look like when it needs to have capabilities in scope or as extra arguments to be partially applied before passing functions to Dream. That's ultimately completely independent of Dream and an artifact of Eio, however, so porting Dream to Eio can probably proceed regardless.
I guess you're talking about Dream.static etc.? It would certainly be possible to handle this within Dream to hide the complexity in the interface. That would force threading the capabilities through parts of the library so I guess that's not really that desirable. Although adding a env/fs argument to Dream.get etc. would still be relatively minimal.
In general, I agree that the capability based approach does not mesh that well with Dream in its current form. There might be a more natural way to expose the same functionality, both from the perspective of the user and Eio. I personally find it a bit confusing how Dream.get and Dream.static combine as in e.g. Dream.get "/static/**" (Dream.static "."). I guess that's something for the future.
The good thing is that even if Eio drops capabilities, or an Eio-compatible "easier" library becomes available, since only Dream's "entry points" have been affected, none of the rest of Dream's API will have to be changed to adapt to that style of programming. It will only affect the "entry points" and user code. So I think porting Dream is worthwhile even to capability-Eio, despite my reservations about capability-Eio.
Glad to hear that! Creating a no-capability-Eio library should be quite simple as one could simply wrap the Eio_main.run to install a Get_Env effect handler, allowing all the functions requiring capabilities to get env on demand. Personally, I prefer capabilities but that might be an alternative for those that desire it.
@Willenbrink, I'm going to occasionally push minor tweaks into this PR, or merge master into it. Hope you don't mind :) Please don't force push into it (and thus, please don't rebase!). In the end, when we merge this, it'll probably go into a branch or a separate OCaml module at first, and I'll divide up the net diff into a bunch of commits (or maybe squash it) and give @talex5 and you the appropriate credit for all parts with git commit --author and/or Co-Authored-By:.
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:https://github.com/aantron/dream/blob/edd0cd916719fc7b5291e99f581c5e69b0ac0005/src/dream.mli#L458-L462
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 -> responsefunctions. -
Functions like
Dream.body:https://github.com/aantron/dream/blob/edd0cd916719fc7b5291e99f581c5e69b0ac0005/src/dream.mli#L699
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, likerequest -> 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, 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.
@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.
@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.
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.
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 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, 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).
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
@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.
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:
[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.
