-
Notifications
You must be signed in to change notification settings - Fork 20
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
Initial Eio port #256
base: eio
Are you sure you want to change the base?
Initial Eio port #256
Conversation
9c74a31
to
6a53d2d
Compare
Update OCaml and Cap'n Proto versions in GHA
Update for opam 2.1+ installations
Update tests to Cap'n Proto 0.10.3
update to mirage-crypto-rng 0.11 and dns 7.0.0 API changes
Co-authored-by: Thomas Leonard <[email protected]>
Add changes for v1.2.3 release
Fix Windows CI
Otherwise `Stdint.Uint32.to_int (Stdint.Uint32.max_int) = -1`!
Add some missing lower-bounds
gha: pin compiler and caml-config.2 on Windows
opam's macos sandbox prevents tests from running. Suggested by Kate <[email protected]>.
Gha updates
Update links to ocaml-ci and capnp-ocaml
Can we keep the LWT version available for OCaml 4.14? We will need to move services to OCaml 5 and EIO over a longer period of time, both for platform support reasons and just because it will take some time to port things over. |
The existing release will continue to work there. capnp-rpc is pretty stable, so I'm not expecting any major new features that would need back-porting. |
We'll take a release of this when it's ready. clarke and solver-service could use it. |
Update tests to Cap'n Proto 1.0.1
unix/network.ml
Outdated
Eio_unix.Resource.fd_opt flow | ||
|> Option.iter (fun fd -> | ||
Eio_unix.Fd.use_exn "TCP_NODELAY" fd @@ fun fd -> | ||
Unix.setsockopt fd Unix.TCP_NODELAY true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to wrap this to catch
Unix.Unix_error(Unix.EOPNOTSUPP, "setsockopt", "")
Stumbled across this new change when using this branch on macOS with a unix domain socket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Not sure whether to do it here, or get ocaml-multicore/eio#575 finished. We could then do something like:
Eio.Net.connect ~sw net eio_addr ~options:[Try (Delay false)]
capnp-rpc-net/endpoint.ml
Outdated
let pp_error f = function | ||
| `Closed -> Fmt.string f "Connection closed" | ||
| `Msg m -> Fmt.string f m | ||
Eio.Flow.shutdown t.flow `All |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapping this in a try stops us crashing when a flow has already been closed. I.e.
try
Eio.Flow.shutdown t.flow `All
with | Eio.Io (Eio.Net.E Connection_reset _, _) as ex ->
Log.info (fun f -> f "%a" Eio.Exn.pp ex);
Update for breaking changes in tls, mirage-crypto and mirage-flow. capnp-rpc-mirage is gone for now as there were further changes needed there and once the conversion to Eio is done we won't need a separate API for unikernels anyway.
- In `sturdy-refs-3`, `for_alice` wasn't explicitly released. If it gets GC'd while the connection is still up, it logs a warning. This was causing CI to fail in some cases. - The restorer table takes ownership of the reference (back-port from Eio branch). Ensure vats are shutdown cleanly to make errors easier to find (Eio does this automatically).
Update dependencies and remove capnp-rpc-mirage
707eee6
to
20a18fb
Compare
Test with OCaml 5. Updated setup-ocaml to v3. Had to add `SHELLOPTS: igncr`, to fix: line 1: $'\r': command not found See ocaml/setup-ocaml#815 (comment) Removed opam-repository-mingw.git#sunset. It doesn't seem to be needed now, and was causing ocamlfind to fail to install.
Update GH actions to OCaml 5
This switches capnp-rpc from Lwt to Eio. One particularly nice side effect of this is that `Service.return_lwt` has gone, as there is no distinction now between concurrent and non-concurrent service methods. Note: - In this commit, everything is still using the "lwt" names to make the diff useful. In a future commit, this should be renamed. Also, some of the "unix" functions can be moved into the core library with Eio. This would likely be a good time to rename `capnp_rpc` to `capnp_rpc_protocol` or something, leaving the short name free for the main library.
Instead of spawning a new flush fiber if one isn't running, just keep one around at all times. This is simpler, makes the traces cleaner, and will probably help with buffering later.
Should probably buffer up messages instead, but this solves the immediate problem.
Sending each message in its own TCP packet isn't very efficient. Note: The Prometheus metrics `messages_outbound_sent_total` and `messages_outbound_dropped_total` have gone. They weren't very useful and we no longer know the number of messages by the time the connection is dropped (could report dropped bytes if needed though). $ dune exec -- ./test-bin/echo/echo_bench.exe echo_bench.exe: [INFO] rate = 18466.209374 # Before echo_bench.exe: [INFO] rate = 59655.912455 # After
Also, fix ref-counting race in test.
This switches capnp-rpc from Lwt to Eio. One particularly nice side effect of this is that
Service.return_lwt
has gone, as there is no distinction now between concurrent and non-concurrent service methods.Notes:
capnp_rpc
tocapnp_rpc_protocol
or something, leaving the short name free for the main library.This commit aims to keep the diff (relatively) small. Several more things are needed after this is merged. In particular:
Exit
to finish.