-
Notifications
You must be signed in to change notification settings - Fork 237
Send GRPC errors in trailers in all cases #486
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need a new send_error callback? What does send_reply not have?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function already exists, it's just not accessible via the Adaptor behaviour. The difference to send_reply is that the error path doesn't try to GRPC-encode the message, puts the error information into the trailers, and ends the connection immediately. It would be possible to change the contract of I felt it cleaner to keep these two paths clearly separated, and as the adapter will need to track its own errors anyways and have a way to deal with those, this is isn't actually introducing new complexity. But if you'd prefer to overload
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Second paragraph sold me on the new callback!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Second paragraph sold me on the new callback! |
||
| stream | ||
| end | ||
|
|
||
| def send_reply( | ||
| %{grpc_type: :server_stream, codec: codec, access_mode: :http_transcoding, rpc: rpc} = | ||
| stream, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we need to check is if the only errors that can reach here are in the form of this struct, of if we need to add another clause
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good question.
I've oscillated between having one and not having one, as it would be "easy" enough to try and match on any error .. or even just errors with strings (an error message) and wrap those in an RPCError.
The flip side of that is should that occur, it is almost certainly a developer error. Either an RPCError , a thrown exception, or a protobuf-encodable struct should be the result. Returning Something Else(tm) is probably something like a raw Ecto query or whatever that has been returned, and simply passing that back silently to the client, even as an error, feels somewhere between dangerous and too much magic.
In those cases, I'd rather see a request crash on the server side that can be tracked and addressed.