Avoid throwing InvalidOpEx in server stream

[erikmav]

Return false instead of throwing InvalidOperationException on read of a gRPC server stream after the connection was closed.

In porting a large application from the deprecated Google gRPC server to ASP.NET, this exception path required adding an additional catch where none had been needed before. Related to #1219 but not a complete fix for that issue.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Erik Mavrinac (0c5f7d5)

[JamesNK]

Hmmm, I disagree with what Grpc.Core is doing. Returning false instead of an error seems like an easy way for people to unknowingly ignore errors.

Perhaps there should be a setting to opt-in to this behavior. People who are porting from Grpc.Core can enable it once on the server.

@jtattermusch

[erikmav]

The case here in the existing client-server implementation is that a Task is kept awaiting the server stream while the client disconnects, as the server must continue to accept any late-arriving client messages. On the Google server this simply returns false once the client closes the connection.

[jtattermusch]

Hmmm, I disagree with what Grpc.Core is doing. Returning false instead of an error seems like an easy way for people to unknowingly ignore errors.

Perhaps there should be a setting to opt-in to this behavior. People who are porting from Grpc.Core can enable it once on the server.

@jtattermusch

The question is whether receiving cancellation from the client on the server side is an "error" that needs to be handled/reported. For some RPCs, the client cancelling the RPC is a "normal" mode of operation in the sense that the client announces that given streaming RPC is no longer needed (since e.g. a bidi call be be used to represent a sequence of "commands" sent by the client. Or if the RPC is used to send updates from the server, client cancelling the RPC is just a signal to stop sending updates).
The natural consequence of client cancelling seems to be that the request stream suddenly ends (since we know that client is done sending). If the client cancels, the server won't be able to send any more responses to the client, so arguably there's not much point in throwing in the server-side handler (sending responses back from the server will fail anyway). The cancellation flag can still be detected with serverCallContext.CancellationToken (if important for the server).

Throwing in requestStream.ReadNext() just make the code more complicated in case where cancelling the RPC is part of a normal workflow (and I agree that it makes migrating code from Grpc.Core more complicated, and we don't want that).

I'd be in favor of changing the grpc-dotnet behavior or at least adding a knob to configure the behavior.

[halter73]

Return false instead of throwing InvalidOperationException on read of a gRPC server stream after the connection was closed.

Wouldn't BodyReader.ReadStreamMessageAsync throw anyway if the underlying HTTP/2 connection closed before the HTTP/2 stream completed? Or if the underlying HTTP/2 stream was reset? I think this is probably desirable so you can distinguish between the client closing the stream gracefully vs. ungracefully.

Hmmm, I disagree with what Grpc.Core is doing. Returning false instead of an error seems like an easy way for people to unknowingly ignore errors.

This makes sense to me too. However, why was this ever an InvalidOperationException instead of an OperationCanceledException?