dream
dream copied to clipboard
[RFC] Initial port to Eio
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, though I did update the first couple to give an idea: https://github.com/talex5/dream/tree/eio/example/1-hello
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 withLwt.async
beforeDream.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 theLwt_main.run
by default. -
Dream.run
now takes anenv
argument (fromEio_main.run
), granting it access to the environment. At present, it uses this just to startLwt_eio
, but once fully converted it should also use it to listen on the network and read certificates, etc.
The first two commits just delete stuff that already didn't work on multicore OCaml. The changes for review/comments are in the 3rd commit.
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.
example/k-websocket
works, but logs Async exception: (Failure "cannot write to closed writer")
. It does that on master
with Lwt too.
/cc @patricoferris
Note: example/5-promise
might need renaming, as it no longer uses any promises. However, it did get a lot simpler!
https://github.com/aantron/dream/pull/194/files#diff-650816042e8a374f7a76bf017cdf8a7ddac340e96b8c66f0e383005b390eaf48
Looks pretty good!
I am slightly concerned by capabilities. They do seem to make all the examples more verbose. In particular, the ones with sleep have extra machinery for extracting and passing clock
. I'm concerned this kind of passing capabilities around will balloon with more complex applications than the examples. It is vaguely against the streamlining tendency of the rest of Dream's API (which is otherwise furthered by this diff by getting rid of most promises).
I am slightly concerned by capabilities. They do seem to make all the examples more verbose. In particular, the ones with sleep have extra machinery for extracting and passing clock. I'm concerned this kind of passing capabilities around will balloon with more complex applications than the examples.
Yes, I'd expect it to make things a bit more verbose (though hopefully also clearer in some cases). It shouldn't balloon too much though, because if you need lots of things you can always give up and take the env
parameter itself instead of individual items. Dream.run
does this, for example, so when I move networking to Eio (and Dream.run
therefore needs a network capability), nothing will change in the examples.
Looking at the tutorials, I'd expect to need these changes if fully switched over to use capabilities:
-
static
should take the directory to serve (e.g.Dream.static env#cwd
instead ofDream.serve "."
). -
sql_pool
would need to takeenv
, because it interprets a string from the user. It might need to access the network, to use a remote database, or it might need a directory, for sqlite.
I think the rest would be unaffected. Producing logging, tracing, metrics, etc, doesn't require a capability.
I've pushed a commit to have Eio run the accept loop too (and then convert the resulting flow to Lwt_unix for httpaf-lwt). Note that this depends on https://github.com/ocaml-multicore/eio/pull/155.
Not having to pass clock around is much nicer.
Is there any interest in merging this PR if it were brought up-to-date? I would like to use Eio with Dream and therefore might work on updating the PR in the near future.
We definitely want to make Dream more compatible wth Eio, but I have to take a fresh look at this PR. This PR, as I recall, has Dream still mostly using the Lwt API. I think in the future we'd want to switch to Eio more completely, but I have to look through the impact that would have on Dream.
This PR may be a good intermediate step, however. I cannot absolutely promise a merge. At the very least, though, an up-to-date version of this PR would greatly help me with reviewing it and exploring what to do with Dream and Eio, and I would be thankful.
Sounds good! I already took a look and it seems like hiding Lwt completely in the API also requires changing large parts to Eio so maybe going all the way for Lwt isn't that hard in the end.
I'm going to close this in favor of #254, where work on the Eio is proceeding. I have the branch locally, so I can refer to authorship information, which I understand is a little bit off in #254 (but which will be squashed or otherwise have its history rewritten anyway, with due credit).