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

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

Open
polvalente opened this issue Jun 14, 2023 · 2 comments
Open

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

polvalente opened this issue Jun 14, 2023 · 2 comments
Milestone

Comments

@polvalente
Copy link
Contributor

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.

@polvalente polvalente added this to the v0.7 milestone Jun 14, 2023
@avillen
Copy link
Contributor

avillen commented Jun 14, 2023

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants