dream icon indicating copy to clipboard operation
dream copied to clipboard

[RFC] Initial port to Eio

Open talex5 opened this issue 2 years ago • 5 comments

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

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

talex5 avatar Jan 20 '22 16:01 talex5

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

talex5 avatar Jan 20 '22 17:01 talex5

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

aantron avatar Jan 21 '22 12:01 aantron

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 of Dream.serve ".").
  • sql_pool would need to take env, 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.

talex5 avatar Jan 21 '22 15:01 talex5

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.

talex5 avatar Jan 21 '22 17:01 talex5

Not having to pass clock around is much nicer.

dangdennis avatar Apr 01 '22 04:04 dangdennis

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.

Willenbrink avatar Mar 28 '23 17:03 Willenbrink

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.

aantron avatar Mar 29 '23 07:03 aantron

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.

Willenbrink avatar Mar 30 '23 20:03 Willenbrink

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

aantron avatar Apr 21 '23 06:04 aantron