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

Is the response message of a response-unary status-not-OK RPC ever legitimate? #12824

Closed
nathanielmanistaatgoogle opened this Issue Oct 4, 2017 · 9 comments

Comments

@nathanielmanistaatgoogle
Member

nathanielmanistaatgoogle commented Oct 4, 2017

It's currently possible (in at least Python, and I would assume in many other languages too) for an application servicing a response-unary RPC method to both emit a response message ("emit a response message" in C++ means "mutate the 'response' out-parameter", right?) and set the RPC's status to not-OK. gRPC Core, given both a send_message operation and a status-not-OK status operation in the same batch, transmits both the message and the status (right?).

Is this a valid use case? (Not just in terms of what's implemented today, but in terms of all implementations possible in the future as well.)

  • Do any clients currently make available to invocation-side application the response message associated with a status-not-OK RPC?
    • Is it supported that such a client could be written in the future?
  • If the service-side gRPC Python runtime were altered to discard response messages in status-not-OK circumstances, would this alteration be considered incorrect?
@dfawley

This comment has been minimized.

Show comment
Hide comment
@dfawley

dfawley Oct 4, 2017

Contributor

grpc-go's client-side API currently accepts a pointer for the response message, and that is populated when the response is received from the server. If an error occurs in the middle of deserialization, it's possible the message is only partially populated, but if it happens afterwards (e.g. due to a non-OK status), then it will be fully populated. Unfortunately, with our current API, it's not easy or error-proof to determine whether the error returned back to the user came from the grpc client library or the remote server, or whether it happened before, during, or after the response message was deserialized. Also, the stubs we generate for the proto API return a nil response if any error/status is received from the grpc-go call. We have a user that is requesting this feature, though, so we have started looking into solutions.

It's my understanding that, if a user were to attempt a streaming RPC from the client to a server that implements a unary handler for that method, they would be able to see the response message and then receive the status separately afterwards. On the other side, a server could implement a streaming handler for a unary RPC and return a message and a non-OK status. So it seems to me this is more a question about what gRPC libraries SHOULD/MUST provide and it's not a wire protocol limitation.

Contributor

dfawley commented Oct 4, 2017

grpc-go's client-side API currently accepts a pointer for the response message, and that is populated when the response is received from the server. If an error occurs in the middle of deserialization, it's possible the message is only partially populated, but if it happens afterwards (e.g. due to a non-OK status), then it will be fully populated. Unfortunately, with our current API, it's not easy or error-proof to determine whether the error returned back to the user came from the grpc client library or the remote server, or whether it happened before, during, or after the response message was deserialized. Also, the stubs we generate for the proto API return a nil response if any error/status is received from the grpc-go call. We have a user that is requesting this feature, though, so we have started looking into solutions.

It's my understanding that, if a user were to attempt a streaming RPC from the client to a server that implements a unary handler for that method, they would be able to see the response message and then receive the status separately afterwards. On the other side, a server could implement a streaming handler for a unary RPC and return a message and a non-OK status. So it seems to me this is more a question about what gRPC libraries SHOULD/MUST provide and it's not a wire protocol limitation.

@vjpai

This comment has been minimized.

Show comment
Hide comment
@vjpai

vjpai Oct 5, 2017

Contributor

To followup on @dfawley's point: we have explicit support in the C++ API and code generator for the case of client-side unary RPC with a server-side streaming handler; the customer motivation in our case was for the server to actually get initial metadata without getting the message, but the purpose that you described would also be a possible use.

Contributor

vjpai commented Oct 5, 2017

To followup on @dfawley's point: we have explicit support in the C++ API and code generator for the case of client-side unary RPC with a server-side streaming handler; the customer motivation in our case was for the server to actually get initial metadata without getting the message, but the purpose that you described would also be a possible use.

@mehrdada

This comment has been minimized.

Show comment
Hide comment
@mehrdada

mehrdada Oct 5, 2017

Member

To add an additional perspective, at the transport layer, it is a common use case in REST to communicate additional data about an erroneous state in the body of a response with a non-OK status code, so even though our codegen may or may not want to support this, it would sensible for the protocol to support that use-case under the hood so that custom codegens that want to emulate such functionality are not left cold-handed.

Member

mehrdada commented Oct 5, 2017

To add an additional perspective, at the transport layer, it is a common use case in REST to communicate additional data about an erroneous state in the body of a response with a non-OK status code, so even though our codegen may or may not want to support this, it would sensible for the protocol to support that use-case under the hood so that custom codegens that want to emulate such functionality are not left cold-handed.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Oct 5, 2017

Member

Since I posed the question at the start of the issue in tone-neutral way (at least I hope I did) I'll add my personal sentiments here: gRPC is better if the answer to this question is "yes". If the answer to this question is "yes", the library has a cleaner interface, fewer special cases, simpler, more square edges, and is more developer-friendly (even if just a tiny bit) than otherwise.

Member

nathanielmanistaatgoogle commented Oct 5, 2017

Since I posed the question at the start of the issue in tone-neutral way (at least I hope I did) I'll add my personal sentiments here: gRPC is better if the answer to this question is "yes". If the answer to this question is "yes", the library has a cleaner interface, fewer special cases, simpler, more square edges, and is more developer-friendly (even if just a tiny bit) than otherwise.

@amitsaha

This comment has been minimized.

Show comment
Hide comment
@amitsaha

amitsaha Oct 10, 2017

A common situation I think where we may want to have this ability is to return validation errors to the client. Validation beyond the data types such as numeric range, for example.

amitsaha commented Oct 10, 2017

A common situation I think where we may want to have this ability is to return validation errors to the client. Validation beyond the data types such as numeric range, for example.

@a11r

This comment has been minimized.

Show comment
Hide comment
@a11r

a11r Oct 10, 2017

Contributor

It is a convention at the protobuf-based type-safe API layer to disallow responses to Unary RPCs that have a non-OK status code.
Server-implementations of gRPC MUST drop the Message if it is returned with a non-OK response.
Client side SHOULD drop any response messages received with a non-OK response.

For generaic APIs, and non-proto-based APIs, the gRPC wire protocol allows any arbitrary combination.

For streaming RPCs, zero or more messages are allowed in either direction. There is a grey area for Client Streaming RPCs (so, Unary response). The streaming convention pulls one way and the unary convention pulls the other way. I think we need to go with the Unary convention because that leads to API consistency and services that really want the streaming convention can choose to use Bidi Streaming to get it.

Note that I have used MUST on the Server-side but SHOULD on the client side. This is to avoid API changes for client implementations that hand up a received message.

Contributor

a11r commented Oct 10, 2017

It is a convention at the protobuf-based type-safe API layer to disallow responses to Unary RPCs that have a non-OK status code.
Server-implementations of gRPC MUST drop the Message if it is returned with a non-OK response.
Client side SHOULD drop any response messages received with a non-OK response.

For generaic APIs, and non-proto-based APIs, the gRPC wire protocol allows any arbitrary combination.

For streaming RPCs, zero or more messages are allowed in either direction. There is a grey area for Client Streaming RPCs (so, Unary response). The streaming convention pulls one way and the unary convention pulls the other way. I think we need to go with the Unary convention because that leads to API consistency and services that really want the streaming convention can choose to use Bidi Streaming to get it.

Note that I have used MUST on the Server-side but SHOULD on the client side. This is to avoid API changes for client implementations that hand up a received message.

@amitsaha

This comment has been minimized.

Show comment
Hide comment
@amitsaha

amitsaha Oct 10, 2017

Server-implementations of gRPC MUST drop the Message if it is returned with a non-OK response.

Even allowing an empty message would be much better here IMHO than now allowing anything at all. One of the ideas floated on this thread was of a special grpc.EmptyResponse() or such object.

amitsaha commented Oct 10, 2017

Server-implementations of gRPC MUST drop the Message if it is returned with a non-OK response.

Even allowing an empty message would be much better here IMHO than now allowing anything at all. One of the ideas floated on this thread was of a special grpc.EmptyResponse() or such object.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Nov 1, 2017

Member

@a11r: what's the basis of the convention? Is there any particular reasoning behind it? Any motivation for it other than history? How severe are the penalties for violating it? Have you had a chat with @dfawley about why his customer wants it, and for what they might use it?

Member

nathanielmanistaatgoogle commented Nov 1, 2017

@a11r: what's the basis of the convention? Is there any particular reasoning behind it? Any motivation for it other than history? How severe are the penalties for violating it? Have you had a chat with @dfawley about why his customer wants it, and for what they might use it?

@a11r

This comment has been minimized.

Show comment
Hide comment
@a11r

a11r May 15, 2018

Contributor

The convention is there for historical reasons.

It should be possible to send trailing metadata with OK status and use that to send extra information to the client.

Contributor

a11r commented May 15, 2018

The convention is there for historical reasons.

It should be possible to send trailing metadata with OK status and use that to send extra information to the client.

@a11r a11r closed this May 15, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Sep 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.