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

[RFC] Initial port to Eio #194

Closed
wants to merge 8 commits into from
Closed

[RFC] Initial port to Eio #194

wants to merge 8 commits into from

Conversation

talex5
Copy link

@talex5 talex5 commented Jan 20, 2022

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

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.
@talex5
Copy link
Author

talex5 commented Jan 20, 2022

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

@aantron
Copy link
Owner

aantron commented Jan 21, 2022

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

@talex5
Copy link
Author

talex5 commented Jan 21, 2022

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.

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).
@talex5
Copy link
Author

talex5 commented Jan 21, 2022

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.

src/dream.mli Show resolved Hide resolved
Just fixes some deprecation warnings.
Anton says he prefers not passing the clock as an argument.
@dangdennis
Copy link
Contributor

Not having to pass clock around is much nicer.

@Willenbrink
Copy link

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.

@aantron
Copy link
Owner

aantron commented Mar 29, 2023

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.

@Willenbrink
Copy link

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.

@aantron
Copy link
Owner

aantron commented Apr 21, 2023

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 aantron closed this Apr 21, 2023
@aantron aantron mentioned this pull request Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants