Add `eio` implementation #21

quernd · 2023-01-07T07:50:18Z

Both unary and streaming, client and server are implemented. The API mostly follows the existing API but we can take this opportunity to overhaul it.

wokalski · 2023-01-19T23:21:44Z

We'll iterate on it for our internal project and merge when ready.

tmcgilchrist

This changes are now required to run the greeter example code.

examples/greeter-client-eio/dune

examples/greeter-server-eio/dune

examples/greeter-client-eio/greeter_client_eio.ml

tmcgilchrist · 2023-04-28T23:42:26Z

The last two commits on https://github.com/tmcgilchrist/ocaml-grpc/tree/grpc-eio provide the fixes and ports the route guide example to EIO.

dune-project

This fixes a race condition where the trailer was ignored because the read body was not fully processed, so the status promise was not resolved by the trailer handler in time.

We don't need a separate request queue if we just fork

We might wait forever causing a deadlock. Maybe this race condition won't occur in the real world because we have already written the status promise meaning a well-behaved client should terminate the request, but we can't rely on that. Also it occurs with mocked request streams in testing, and overall I see no reason why we should exhaust the request stream here.

Co-authored-by: Tim McGilchrist <timmcgil@gmail.com>

quernd · 2023-05-24T06:56:51Z

Thanks @tmcgilchrist. I think the rebasing went well this time. CI is now all green except for the experimental lint workflows. I converted the greeter EIO examples to ocaml-protoc-plugin because the shared greeter library changed. Since you ported the routeguide tutorial to EIO, there are probably a few changes needed in the instructions. I only auto-generated the MDX code blocks. It's great that we already have a tutorial that uses EIO!

dune-project

tmcgilchrist · 2023-07-06T23:06:29Z

lib/grpc-eio/connection.ml

+  Seq.iter
+    (fun encoder ->
+      let payload = Grpc.Message.make encoder in
+      H2.Body.Writer.write_string body payload)


Should this be calling flush after each iteration?
I'm undecided about whether it should. The way it is now will batch up the writes and send them in a smaller number of network packets. Flushing after each write_string would send them immediately to the server, which might be preferable.

That's a good question. It could be preferable when aiming for low latency. I'm thinking maybe it should just be opt-in.

On this note, we ran some load tests but they were inconclusive because we stumbled upon a bug in Eio: ocaml-multicore/eio#572
It has been fixed, so I hope we'll have better numbers soon.

We're also experimenting with a different interface for the reader. Lazy sequence might be a bit too opinionated, and they introduce a bit of overhead, so the alternative interface is similar to H2 with on_msg/on_eof callbacks, with a conversion to Seq.t as a convenience helper: https://github.com/dialohq/ocaml-grpc/tree/grpc-eio-noseq/lib/grpc-eio

Interesting experiment, it will be good to see some performance numbers from it. Keeping the Seq.t for a simple interface is a good idea for introducing the library and less performance sensitive applications.

Opt-in sounds good, I'm happy to leave that till later on. There are a few other config pieces like auth and compression that will need a way to configure them.

tmcgilchrist · 2023-07-07T00:04:30Z

lib/grpc-eio/server.ml

+                    else respond_with `Not_acceptable)
+            | Some _ ->
+                (* TODO: not sure if there is a specific way to handle this in grpc *)
+                respond_with `Bad_request


This details the responses required for unsupported encodings https://github.com/grpc/grpc/blob/master/doc/compression.md#test-cases

Thanks! I'll take a look. By the way, with the Eio implementation and your Async server implementation, this code is now duplicated and could be factored out.

I'll do another pass over the tutorial code after this is merged.

dune-project

examples/routeguide-tutorial.md

examples/greeter-client-eio/greeter_client_eio.ml

examples/routeguide/src/client.ml

Lower bounds for `eio_main` and `h2-eio` Co-authored-by: Tim McGilchrist <timmcgil@gmail.com>

tmcgilchrist · 2023-07-10T23:19:31Z

examples/greeter-server-eio/greeter_server_eio.ml

+  print_endline "Try running:";
+  print_endline "";
+  print_endline
+    {| dune exec -- examples/greeter-client-lwt/greeter_client_lwt.exe <your_name> |};


Suggested change

{| dune exec -- examples/greeter-client-lwt/greeter_client_lwt.exe <your_name> |};

{| dune exec -- examples/greeter-client-eio/greeter_client_eio.exe <your_name> |};

tmcgilchrist · 2023-07-10T23:45:02Z

lib/grpc-eio/connection.ml

+  Seq.iter
+    (fun encoder ->
+      let payload = Grpc.Message.make encoder in
+      H2.Body.Writer.write_string body payload)


Interesting experiment, it will be good to see some performance numbers from it. Keeping the Seq.t for a simple interface is a good idea for introducing the library and less performance sensitive applications.

Opt-in sounds good, I'm happy to leave that till later on. There are a few other config pieces like auth and compression that will need a way to configure them.

tmcgilchrist · 2023-07-10T23:48:53Z

Merging this to unblock other PRs.

quernd force-pushed the grpc-eio branch 2 times, most recently from d4c75c6 to 38d8fd8 Compare January 7, 2023 09:25

wokalski force-pushed the grpc-eio branch from 42bbc21 to 9f70672 Compare February 3, 2023 23:35

quernd mentioned this pull request Mar 17, 2023

Getting started documentation for ocaml-grpc #25

Merged

4 tasks

tmcgilchrist reviewed Apr 28, 2023

View reviewed changes

examples/greeter-client-eio/dune Outdated Show resolved Hide resolved

examples/greeter-server-eio/dune Outdated Show resolved Hide resolved

examples/greeter-client-eio/greeter_client_eio.ml Outdated Show resolved Hide resolved

quernd force-pushed the grpc-eio branch 3 times, most recently from 702d4b5 to c56d70e Compare May 23, 2023 08:59

tmcgilchrist reviewed May 24, 2023

View reviewed changes

dune-project Outdated Show resolved Hide resolved

quernd and others added 19 commits May 24, 2023 08:05

Add eio implementation

5c685f1

Update dune-project

d579539

Exhaust reader streams

add94c9

This fixes a race condition where the trailer was ignored because the read body was not fully processed, so the status promise was not resolved by the trailer handler in time.

Refactoring

776c05b

Server only needs a push function, not a writer

9e30fc6

Client-streaming client should receive a promise

ad872f2

Add a greeter server example for eio

6983656

Simplify the greeter server

2757a36

We don't need a separate request queue if we just fork

Add a proper error handler

2f90b63

Fix dune-project and re-generate grpc-eio.opam

eaacd26

Always flush the headers immediately

043cd7e

Create readme.md

60e0dfc

Fixup eio dependencies.

d74d5b0

Add EIO version of routeguide.

4076e48

Update dune-project

d441017

Co-authored-by: Tim McGilchrist <timmcgil@gmail.com>

Update grpc-examples.opam

2e14187

Update grpc-eio.opam

eecd65e

Replace examples/routeguide-tutorial.md with EIO version

eda1ea8

quernd force-pushed the grpc-eio branch from f1f9308 to eda1ea8 Compare May 24, 2023 06:25

quernd added 2 commits May 24, 2023 08:41

Convert eio examples to ocaml-protoc-plugin

32ef04a

Formatting

96587e3

quernd force-pushed the grpc-eio branch from f088e55 to 96587e3 Compare May 24, 2023 06:50

quernd marked this pull request as ready for review May 24, 2023 06:56

quernd requested a review from tmcgilchrist May 24, 2023 06:57

tmcgilchrist approved these changes Jul 7, 2023

View reviewed changes

tmcgilchrist and others added 5 commits July 7, 2023 12:18

Update examples/greeter-client-eio/greeter_client_eio.ml

01cbecd

Update examples/routeguide/src/client.ml

5842e0c

Re-run routeguide tutorial

0c115fa

Add routeguide-lwt example.

6f192ac

Apply suggestions from code review

ab8a80b

Lower bounds for `eio_main` and `h2-eio` Co-authored-by: Tim McGilchrist <timmcgil@gmail.com>

tmcgilchrist approved these changes Jul 10, 2023

View reviewed changes

tmcgilchrist merged commit 4fa8b53 into main Jul 10, 2023

tmcgilchrist deleted the grpc-eio branch July 10, 2023 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `eio` implementation #21

Add `eio` implementation #21

quernd commented Jan 7, 2023 •

edited

Loading

wokalski commented Jan 19, 2023

tmcgilchrist left a comment

tmcgilchrist commented Apr 28, 2023

quernd commented May 24, 2023

tmcgilchrist Jul 6, 2023

quernd Jul 10, 2023

tmcgilchrist Jul 10, 2023

tmcgilchrist Jul 7, 2023

quernd Jul 10, 2023

tmcgilchrist Jul 10, 2023

tmcgilchrist Jul 10, 2023

tmcgilchrist Jul 10, 2023

tmcgilchrist commented Jul 10, 2023

	{\| dune exec -- examples/greeter-client-lwt/greeter_client_lwt.exe <your_name> \|};
	{\| dune exec -- examples/greeter-client-eio/greeter_client_eio.exe <your_name> \|};

Add eio implementation #21

Add eio implementation #21

Conversation

quernd commented Jan 7, 2023 • edited Loading

wokalski commented Jan 19, 2023

tmcgilchrist left a comment

Choose a reason for hiding this comment

tmcgilchrist commented Apr 28, 2023

quernd commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmcgilchrist commented Jul 10, 2023

Add `eio` implementation #21

Add `eio` implementation #21

quernd commented Jan 7, 2023 •

edited

Loading