search

dialohq / ocaml-grpc

gRPC library for OCaml
https://dialohq.github.io/ocaml-grpc
BSD 3-Clause "New" or "Revised" License
58 stars 9 forks source link

eio streaming example only writes one record #31

Closed joprice closed 11 months ago

joprice commented 11 months ago

I was trying out the eio routeguide example, and noticed that the server streaming was only every writing a single record to the response. I ran the lwt example and it gave the expected results of a longer stream of location records. I found that adding a yield in routeguide/src/server.ml causes the stream to be flushed. 

  ...
  encode feature |> Writer.contents |> f;
  Eio.Fiber.yield ())
  ...
quernd commented 11 months ago

Thanks for reporting this issue!

I have to dig into the specifics of H2 to fully understand this behavior, because it seems that even though H2.Body.Writer.write_string is called for every record before H2.Body.Writer.close is called, the records are indeed not written. Eio.Fiber.yield () right after write_string also works, but H2.Body.Writer.flush doesn't have any effect.

quernd commented 11 months ago

On further inspection with Wireshark, H2 does indeed write all the records (mea culpa), the batching behavior is just different with Eio.Fiber.yield (). With the unmodified example, all records are batched into one frame.

So very likely a bug on our part related to buffer handling / message extraction. I'm on it. Thanks again for reporting.

quernd commented 11 months ago

Indeed it's a matter of calling Grpc.Message.extract in a loop to account for these batched messages. So the bug is actually in the client code (in Grpc_eio.Connection.grpc_recv_streaming)! I will prepare a PR and check the other implementations for this as well.

quernd commented 11 months ago

Thanks again for reporting this issue @joprice. This turned out to be an old bug in all three implementations, and I suspect that we have hit this bug in our codebase as well with both Lwt and Eio (we just never connected the dots).

joprice commented 11 months ago

No problem! Happy to help. Glad it wasn't too bad to find a fix. I'll try out the commit when I get a chance.