search

elixir-grpc / grpc

An Elixir implementation of gRPC
https://hex.pm/packages/grpc
Apache License 2.0
1.36k stars 210 forks source link

Bubble errors up from server (and client?) pipelines #316

Open polvalente opened 1 year ago

polvalente commented 1 year ago

From PR #218, we should add a capability for both server (and client?) pipelines to bubble errors up instead of needing to raise for flow control.

avillen commented 1 year ago

Do you have something in mind about this? taking a look at the code I think that it may be "as easy" as the following on this part:

     result = server.__call_rpc__(path, stream)

     case result do
+      {:ok, _stream, error = %GRPC.RPCError{message: nil, status: status}} ->
+        {:error, %{error | message: GRPC.Status.status_message(status)}}
+
+      {:ok, _stream, error = %GRPC.RPCError{}} ->
+        {:error, error}
+
       {:ok, stream, response} ->
         stream
         |> GRPC.Server.send_reply(response)

The first match is because, if I understand correctly, right now the server creates a GRPC.RPCError exception that already generates the :message if necessary, so we need to generate it somehow.

I think that this will be retrocompatible, so both exception and "handled" errors will be allowed. WDYT? I can submit a quick PR if you agree 😄 or if you have something different in mind or I'm wrong, please, ignore me 😂

polvalente commented 1 year ago

Yeah, it's something along those lines!

I don't worry too much about retrocompatibility in the sense that this is a new code path that didn't exist before, but we should ensure the test coverage is good for the modified code path.

I'm happy to review a PR on this