improve error returned by Send on server side after stream is reset

[euroelessar]

trafficstars

Please see the FAQ in our main README.md, then answer the questions below before submitting your issue.

What version of gRPC are you using?

1.29.0

What did you do?

Use SendAndClose or Send on server side for streaming requests. Client stream was reset by grpc stack right before that (e.g. by sending reset or by violating http/2 spec in some way).

What did you expect to see?

SendAndClose returns io.EOF or some error with non-Internal code to indicate about the error.

What did you see instead?

rpc error: code = Internal desc = transport: transport: the stream is done
or WriteHeader was already called

gRPC returns an error with Internal code, indicating API misuse, while application has no way to perform correctly in this case. Any checks (including ctx.Done()) would have been racy as stream is reset by grpc-go from another goroutine.

[menghanl]

There's no SendClose. If you meant CloseSend, it's client side only, how do you use it on server side?

And can you provide more information of what client/server do in the failure? Like what methods are called, and in what order.

[euroelessar]

Sorry, I've meant SendAndClose at server side.

Few examples:

1. Stream-unary RPC at server side 1.1. Server calls stream.Recv() until receiving io.EOF 1.2. Client resets the stream by dying 1.3. Server calls SendAndClose to send response back to client. 1.4. SendAndClose returns the error in the issue
2. Stream-stream RPC at server side 2.1. Server sleeps some time after receiving the request 2.2. Client resets the stream by dying 2.3. Server calls Send to send first payload to client 2.4. Send returns the error in the issue

[menghanl]

It sounds right to me for send to return error when the underlying stream is broken. We can argue about the error message.

And what do you do about a sending error on the server side? The service handler should return something, but the error will never be sent to the client because the stream is already broken.

[euroelessar]

It sounds right to me for send to return error when the underlying stream is broken. We can argue about the error message.

I agree that it's mainly about a content of the error (code/message).

Internal should be reserved for an actual invariant violations/API misuse. For example, it's perfectly fine to return Internal error on SendHeader if SendHeader/Send/etc were already invoked by caller as it is a protocol violation and is likely an indicator of a broken code.

Returning an error with the same Internal code on expected failure modes is not helpful as the only action item for an user it to just ignore this errors and never look at their status codes/content.

And what do you do about a sending error on the server side? The service handler should return something, but the error will never be sent to the client because the stream is already broken.

We usually check its code and aggregate all suspicious errors in a central storage. That's how this particular error did come to our attention.

[dfawley]

Maybe something like grpc.ErrConnectionBroken should be used for this kind of case. Regardless, the server side should not see RPC status errors at all, so Internal or any other code would be invalid here. RPC statuses should only be returned to clients for RPC operations.

[zouyee]

/assign /remove help

[dfawley]

@zouyee thanks for the offer. What changes do you propose to make to fix this?