client stream fails poorly when using CloseSend()

[mightyguava]

When doing client streaming, and if you close the connection with CloseSend() immediately after sending the request (fire-and-forget), the server handler may never receive the request. The request is dropped somewhere in the grpc server level.

If you use CloseAndRecv() to close the connection though, the request will be processed successfully.

This behavior is surprising because, in the case where CloseSend() is called, I expect the server to at least receive the request, and get an error only when it tries to write to the closed stream, rather than not getting the request at all.

What version of gRPC are you using?

master: dfa1834

What version of Go are you using (go version)?

go version go1.10 darwin/amd64

What operating system (Linux, Windows, …) and version?

OS X High Sierra 10.13.2

What did you do?

Repro available at https://github.com/mightyguava/grpc-streaming-bug

Run server/main.go, then run client/main.go

The client sends 30 requests in 10 batches, via client streaming. In each batch, every request opens and closes its own client connection. For each batch:

• Request 1 calls CloseAndRecv() on the stream after sending and is successful
• Request 2 calls CloseSend() on the stream after sending, and almost always fails
• Request 3 waits 1 millisecond before calling CloseSend() on the stream, and is always successful.

What did you expect to see?

All 30 requests succeed.

What did you see instead?

20 of the requests always succeed. The remaining 10 (Request 2) sometimes do succeed but usually fail.

[menghanl]

The ClientConn is closed before requests are sent on wire, so the requests are never sent out.

Sent only pushes the requests to a buffer, it doesn't make sure they are sent on wire.

• In the failed tests, the ClientConn is closed (in the defer) before the requests got sent out.
• In the case of CloseAndRecv, the client blocks until it receives something from the server. So the requests are guaranteed to be sent out.
• In the delayed case. ClientConn is closed 1 ms later, which is long enough to send the requests.

For a streaming RPC, the client is expected to call Recv to read the response and also the final status.
If you really don't care about the server's response, you need to explicitly cancel the RPC so the remaining goroutines will be cleaned up.
See this comment #1854 (comment) for more details.

[mightyguava]

Hey @menghanl, I think this issue should remain open.

Let me preface the following with: I understand this is totally user error. However, this is also unexpected behavior given the way the API contracts within the client stream code are phrased, so I believe this is a real bug. The proper fix may be to update documentation.

The documentation for SendMsg says:

// SendMsg blocks until it sends m, the stream is done or the stream
// breaks.
// On error, it aborts the stream and returns an RPC status on client
// side. On server side, it simply returns the error to the caller.
// SendMsg is called by generated code. Also Users can call SendMsg
// directly when it is really needed in their use cases.
// It's safe to have a goroutine calling SendMsg and another goroutine calling
// recvMsg on the same stream at the same time.
// But it is not safe to call SendMsg on the same stream in different goroutines.

There's no mention of buffering here. If closing the ClientConn causes messages to not be sent, SendMsg is breaking its API contract.

Similarly, the only documentation I see in code around needing to call Recv() is:

// Always call Stream.RecvMsg() to drain the stream and get the final
// status, otherwise there could be leaked resources.

These documentation in no way imply that the message will not be sent. For the repro example and my actual current use case, which is a short-lived commandline tool, it's totally fine to leak resources.

Finally, what exactly is the purpose of CloseSend(), if the proper ways to close the client connection and flush the pipes are:

• cancel the context
• call SendAndRecv()

[menghanl]

I agree that documentation needs to be updated. We can track that with another issue (#1880) if you are OK. Re-purposing this issue might be confusing for other people.

---

CloseSend is to only close the send direction, but keep receiving. The function itself is not that useful in the case of client only streaming RPC (the one you are using), because the nature of this RPC is to send a list of requests, stop sending, and receive.

It would make more sense if you think of a bidirectional streaming RPC, where the client could send a list of requests, stop sending (also tell the server it's done), and receive a list of responses.

Also, actually, CloseAndRecv is a CloseSend plus a Recv:

grpc-go/examples/route_guide/routeguide/route_guide.pb.go

Lines 317 to 326 in 583a630

	func (x *routeGuideRecordRouteClient) CloseAndRecv() (*RouteSummary, error) {
	if err := x.ClientStream.CloseSend(); err != nil {
	return nil, err
	}
	m := new(RouteSummary)
	if err := x.ClientStream.RecvMsg(m); err != nil {
	return nil, err
	}
	return m, nil
	}