grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

Cardinality violations should use error code “unimplemented”

Open jhump opened this issue 1 year ago • 6 comments

The gRPC docs for error codes state that both client and server should use the unimplemented code for cardinality violations. See table at the bottom of this doc (you can search for “cardinality violation” in the doc): https://grpc.github.io/grpc/core/md_doc_statuscodes.html.

A cardinality violation is when a stream contains an incorrect number of messages. Specifically, when a response stream for a unary or client-stream RPC contains zero messages with an OK status or more than one message; or when a request stream for a unary or server-stream RPC contains zero or more than one messages.

The client and server in this Go module both return an unknown error in this situation instead of unimplemented.

jhump avatar May 30 '24 17:05 jhump

For the Java issue I mentioned this change to the spec was made without any real notification/discussion. So we may want to re-consider the spec in this case. https://github.com/grpc/grpc-java/issues/11247#issuecomment-2140803584

ejona86 avatar May 31 '24 16:05 ejona86

@jhump we have to reconsider on deciding the error code for cardinality violations. For now, as long as an error code is being returned, we won't be making any changes.

purnesh42H avatar Jun 14 '24 16:06 purnesh42H

I think we should leave this open until we decide what the behavior should be.

dfawley avatar Jun 21 '24 22:06 dfawley

Should we mark it blocked on the change to the spec, as promised in: https://github.com/grpc/grpc-java/issues/11247#issuecomment-2183531589

easwars avatar Jun 26 '24 18:06 easwars

I believe we've fully committed to making it INTERNAL. I don't think we need to wait on a doc update to implement it.

@ejona86 WDYT?

dfawley avatar Jun 26 '24 22:06 dfawley

Per @ejona86 offline, it's fine to go ahead and make this change before the gRFC happens. We've agreed on that change, just need to go through the process to make it happen, and the current behavior doesn't match the spec already.

dfawley avatar Jul 30 '24 16:07 dfawley