[RFC] Initial port to Eio #194
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.
|
Note: |
|
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 |
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 Looking at the tutorials, I'd expect to need these changes if fully switched over to use capabilities:
I think the rest would be unaffected. Producing logging, tracing, metrics, etc, doesn't require a capability. |
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).
|
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 ocaml-multicore/eio#155. |
Segfaults due to savonet/ocaml-ssl#76
Just fixes some deprecation warnings.
Anton says he prefers not passing the clock as an argument.
|
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. |
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-helloInternally, 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.asyncbeforeDream.runand 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.runby default.Dream.runnow takes anenvargument (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-websocketworks, but logsAsync exception: (Failure "cannot write to closed writer"). It does that onmasterwith Lwt too./cc @patricoferris