From dab56b51647852fb171e21966558166e0bced4f8 Mon Sep 17 00:00:00 2001 From: Aaron Seigo Date: Mon, 15 Dec 2025 10:48:07 +0100 Subject: [PATCH 1/2] Give the status field a default value of unknown This avoids a foot-gun where the app dev does not know which status to use and only sets the message (and/or potentially the details). Without a default value, the status field becomes `nil` which blows up when serializing which is not an easy bug to trace back to the right place in the application. --- grpc_core/lib/grpc/rpc_error.ex | 5 +++-- grpc_server/test/grpc/stream_test.exs | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/grpc_core/lib/grpc/rpc_error.ex b/grpc_core/lib/grpc/rpc_error.ex index f0766e7ab..f0fa8c658 100644 --- a/grpc_core/lib/grpc/rpc_error.ex +++ b/grpc_core/lib/grpc/rpc_error.ex @@ -51,7 +51,8 @@ defmodule GRPC.RPCError do See `GRPC.Status` for more details on possible statuses. """ - defexception [:status, :message, :details] + @enforce_keys [:status] + defexception [:message, :details, status: :unknown] defguard is_rpc_error(e, status) when is_struct(e, __MODULE__) and e.status == status @@ -70,7 +71,7 @@ defmodule GRPC.RPCError do @spec exception(args :: list()) :: t() def exception(args) when is_list(args) do - error = parse_args(args, %__MODULE__{}) + error = parse_args(args, %__MODULE__{status: :unknown}) %{ error diff --git a/grpc_server/test/grpc/stream_test.exs b/grpc_server/test/grpc/stream_test.exs index a4ebaf47e..20a8225e5 100644 --- a/grpc_server/test/grpc/stream_test.exs +++ b/grpc_server/test/grpc/stream_test.exs @@ -260,7 +260,7 @@ defmodule GRPC.StreamTest do __exception__: true, details: nil, message: ":mapped_error", - status: nil + status: :unknown }} ] end @@ -287,7 +287,7 @@ defmodule GRPC.StreamTest do 1, {:error, %GRPC.RPCError{ - status: nil, + status: :unknown, message: "{:exception, %RuntimeError{message: \"boom\"}}", details: nil }} From ef64dcc5d7b6347f1449aa682e7ce7917f2e9839 Mon Sep 17 00:00:00 2001 From: Aaron Seigo Date: Mon, 15 Dec 2025 10:58:31 +0100 Subject: [PATCH 2/2] If an error is returned as the response, send it in the trailers. This exposes a `send_error/2` function in the adapter behaviour. --- .../test/grpc/integration/server_test.exs | 6 ++--- grpc_server/lib/grpc/server/adapter.ex | 2 ++ .../lib/grpc/server/adapters/cowboy.ex | 5 ++++ .../grpc/server/adapters/cowboy/handler.ex | 24 ++++++++++++++++--- grpc_server/lib/grpc/server/stream.ex | 5 ++++ grpc_server/test/support/test_adapter.exs | 4 ++++ 6 files changed, 40 insertions(+), 6 deletions(-) diff --git a/grpc_client/test/grpc/integration/server_test.exs b/grpc_client/test/grpc/integration/server_test.exs index 0e0684bd9..a186e482f 100644 --- a/grpc_client/test/grpc/integration/server_test.exs +++ b/grpc_client/test/grpc/integration/server_test.exs @@ -268,7 +268,7 @@ defmodule GRPC.Integration.ServerTest do end) end) - assert logs =~ "Exception raised while handling /helloworld.Greeter/SayHello" + assert logs =~ "(GRPC.RPCError) Please authenticate" end test "return errors for unknown errors" do @@ -284,7 +284,7 @@ defmodule GRPC.Integration.ServerTest do end) end) - assert logs =~ "Exception raised while handling /helloworld.Greeter/SayHello" + assert logs =~ "unknown error(This is a test, please ignore it)" end test "logs error if exception_log_filter returns true" do @@ -329,7 +329,6 @@ defmodule GRPC.Integration.ServerTest do {pid, ref} = :erlang.binary_to_term(data) send(pid, {:exception_log_filter, ref, exception}) - true end end @@ -396,6 +395,7 @@ defmodule GRPC.Integration.ServerTest do {:ok, channel} = GRPC.Stub.connect("localhost:#{port}") rect = %Routeguide.Rectangle{} error = %GRPC.RPCError{message: "Please authenticate", status: 16} + assert {:error, ^error} = channel |> Routeguide.RouteGuide.Stub.list_features(rect) end) end diff --git a/grpc_server/lib/grpc/server/adapter.ex b/grpc_server/lib/grpc/server/adapter.ex index 9884ac9ff..e229a12ba 100644 --- a/grpc_server/lib/grpc/server/adapter.ex +++ b/grpc_server/lib/grpc/server/adapter.ex @@ -24,4 +24,6 @@ defmodule GRPC.Server.Adapter do @callback send_reply(state, content :: binary(), opts :: keyword()) :: any() @callback send_headers(state, headers :: map()) :: any() + + @callback send_error(state, headers :: map()) :: any() end diff --git a/grpc_server/lib/grpc/server/adapters/cowboy.ex b/grpc_server/lib/grpc/server/adapters/cowboy.ex index 95ee0a2f3..bdbc6caf0 100644 --- a/grpc_server/lib/grpc/server/adapters/cowboy.ex +++ b/grpc_server/lib/grpc/server/adapters/cowboy.ex @@ -168,6 +168,11 @@ defmodule GRPC.Server.Adapters.Cowboy do Handler.set_resp_trailers(pid, trailers) end + @impl true + def send_error(%{pid: pid}, error) do + Handler.send_error(pid, error) + end + def send_trailers(%{pid: pid}, trailers) do Handler.stream_trailers(pid, trailers) end diff --git a/grpc_server/lib/grpc/server/adapters/cowboy/handler.ex b/grpc_server/lib/grpc/server/adapters/cowboy/handler.ex index bc596772d..130b25842 100644 --- a/grpc_server/lib/grpc/server/adapters/cowboy/handler.ex +++ b/grpc_server/lib/grpc/server/adapters/cowboy/handler.ex @@ -221,6 +221,13 @@ defmodule GRPC.Server.Adapters.Cowboy.Handler do sync_call(pid, :read_body) end + @doc """ + Asynchronously send an error to the client. + """ + def send_error(pid, error) do + send(pid, {:send_error, error}) + end + @doc """ Asynchronously send back to client a chunk of `data`, when `http_transcode?` is true, the data is sent back as it's, with no transformation of protobuf binaries to http2 data frames. @@ -485,6 +492,16 @@ defmodule GRPC.Server.Adapters.Cowboy.Handler do {:ok, req, state} end + def info({:send_error, error}, req, state) do + req = send_error(req, error, state, :rpc_error) + + [req: req] + |> ReportException.new(error) + |> maybe_log_error(state.exception_log_filter) + + {:stop, req, state} + end + def info({:handling_timeout, _}, req, state) do error = %RPCError{status: GRPC.Status.deadline_exceeded(), message: "Deadline expired"} req = send_error(req, error, state, :timeout) @@ -525,13 +542,13 @@ defmodule GRPC.Server.Adapters.Cowboy.Handler do # unknown error raised from rpc def info({:EXIT, pid, {:handle_error, error}}, req, state = %{pid: pid}) do - %{kind: kind, reason: reason, stack: stack} = error + %{kind: kind, reason: reason, stack: stacktrace} = error rpc_error = %RPCError{status: GRPC.Status.unknown(), message: "Internal Server Error"} req = send_error(req, rpc_error, state, :error) [req: req] - |> ReportException.new(reason, stack, kind) - |> maybe_log_error(state.exception_log_filter, stack) + |> ReportException.new(reason, stacktrace, kind) + |> maybe_log_error(state.exception_log_filter, stacktrace) {:stop, req, state} end @@ -631,6 +648,7 @@ defmodule GRPC.Server.Adapters.Cowboy.Handler do defp send_error_trailers(%{has_sent_resp: _} = req, _, trailers) do :cowboy_req.stream_trailers(trailers, req) + req end defp send_error_trailers(req, status, trailers) do diff --git a/grpc_server/lib/grpc/server/stream.ex b/grpc_server/lib/grpc/server/stream.ex index fa0a7c195..bae69e28a 100644 --- a/grpc_server/lib/grpc/server/stream.ex +++ b/grpc_server/lib/grpc/server/stream.ex @@ -70,6 +70,11 @@ defmodule GRPC.Server.Stream do do_send_reply(stream, [], opts) end + def send_reply(%{adapter: adapter} = stream, {:error, %GRPC.RPCError{} = error}, _opts) do + adapter.send_error(stream.payload, error) + stream + end + def send_reply( %{grpc_type: :server_stream, codec: codec, access_mode: :http_transcoding, rpc: rpc} = stream, diff --git a/grpc_server/test/support/test_adapter.exs b/grpc_server/test/support/test_adapter.exs index f1eb6ae7d..ea19f8840 100644 --- a/grpc_server/test/support/test_adapter.exs +++ b/grpc_server/test/support/test_adapter.exs @@ -17,6 +17,10 @@ defmodule GRPC.Test.ServerAdapter do {stream, data} end + def send_error(stream, _data) do + stream + end + def send_headers(stream, _headers) do stream end