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

Eio port #256

Closed
wants to merge 1 commit into from
Closed

Eio port #256

wants to merge 1 commit into from

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Jul 29, 2022

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:

  • Some of the "unix" functions can be moved into the core library with Eio.
  • Mirage support is gone. The main library should now work with both Unix and unikernels.

This commit aims to keep the diff (relatively) small. Several more things are needed after this is merged. In particular:

  • Performance improvements (such as sending multiple messages at once).
  • Handling leaked refs correctly. We need to schedule a callback on our owning domain's event loop from the finalizer. Eio doesn't currently provide a way to do this.

@tmcgilchrist
Copy link
Member

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.

@talex5
Copy link
Contributor Author

talex5 commented Jun 2, 2023

Can we keep the LWT version available for OCaml 4.14?

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.

@tmcgilchrist
Copy link
Member

We'll take a release of this when it's ready. clarke and solver-service could use it.

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

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.

Copy link
Contributor Author

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

let pp_error f = function
| `Closed -> Fmt.string f "Connection closed"
| `Msg m -> Fmt.string f m
Eio.Flow.shutdown t.flow `All
Copy link

@RyanGibb RyanGibb Jun 6, 2024

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

@talex5 talex5 mentioned this pull request Sep 30, 2024
@talex5 talex5 changed the base branch from eio to master November 8, 2024 10:54
@talex5 talex5 mentioned this pull request Nov 13, 2024
@talex5 talex5 changed the title Initial Eio port Eio port Nov 13, 2024
@talex5 talex5 marked this pull request as draft November 13, 2024 12:41
@talex5 talex5 added this to the v2.0 (Eio) milestone Nov 16, 2024
Should probably buffer up messages instead, but this solves the
immediate problem.
@talex5
Copy link
Contributor Author

talex5 commented Nov 17, 2024

Everything here got merged with #284 and #287, except for disabling Nagle's algorithm, which is probably no longer necessary. At least, it no longer seems to affect the benchmark results.

@talex5 talex5 closed this Nov 17, 2024
@talex5 talex5 deleted the eio branch November 19, 2024 12:37
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