it should be documented that calling `requestObserver.onError(...)` on the client side cancels the call instead of transferring error status

[morgwai]

What version of gRPC-Java are you using?

1.40.1

What is your environment?

openJdk-11 on ubuntu

What did you expect to see?

in case of bi-di methods (and probably also streaming-client-unary-server), when a client calls for example requestObserver.onError(Status.INTERNAL.withDescription("desc").asException()); the server should receive all messages scheduled for transfer by the client (with requestObserver.onNext(...)) prior to the call to requestObserver.onError(...). After that the server should receive a call to onError(...) with a Status(Runtime)Exception with identical status code (INTERNAL) and description ("desc") .

What did you see instead?

results of clients calling requestObserver.onError(...) are the same as if the call was cancelled: most recent request messages don't get transferred to the server and whatever status and description the client sets, the server always receives Status.CANCELLED and "client cancelled" description.

NOTE: It is possible, that this behavior is intended, but if so, it should be documented in onError(...)'s javadoc.

Steps to reproduce the bug

start a bi-di call on the client side, send some requests, call requestObserver.onError(Status.INTERNAL.withDescription("desc").asException());

[ejona86]

Yes, the client calling onError == cancellation. The client never sends trailers.

The StreamObserver API honestly makes some concepts more confusing as it adapts gRPC semantics to "generic stream" semantics. If you aren't familiar with the async StreamObserver style then it is harder for you to tease apart what is the abstraction and what is gRPC. The ClientCall and ServerCall APIs are much more precise.

[ejona86]

This behavior was recently documented in #8449:

gRPC's implementation of {@code onError()} on client-side causes the RPC to be cancelled and discards all messages, so completes quickly.

Although I agree it is talking about the async behavior and isn't in a section specifically mentioning the semantics.

I will note that the client will see an RPC failure with a CANCELLED status and the description "Cancelled by client with StreamObserver.onError()", which should make the behavior apparent after-the-fact.

NOTE: It is possible, that this behavior is intended, but if so, it should be documented in onError(...)'s javadoc.

The trouble we have is that the behavior you describe is an implementation's behavior, but StreamObserver is an interface. The StreamObserver interface does not say, "sends the error to the remote." Instead it is just "hey, this thing happened" and it is pretty silent on what an implementation would actually do with it. And so you assumed a behavior (not faulting you; there was no behavior specified), and that happened to be wrong. After all, the application gets called with onError() it clearly shouldn't send that throwable to the remote.

This is a duplicate of #7558, although that issue had a laundry list of things it brought up so is not obviously a duplicate. However, in #7558 (comment) I do mention that we wanted to add documentation describing the behavior, but it wouldn't be in StreamObserver.

[morgwai]

This behavior was recently documented in #8449:

ah, sorry: I overlooked/forgot that part... I should be more careful: sorry for bothering!