streams: Stop cleaning up after orphaned streams #1854

dfawley · 2018-02-08T00:45:12Z

This change introduces some behavior changes that should not impact users that are following the proper stream protocol. Specifically, one of the following conditions must be satisfied:

The user calls Close on the ClientConn.
The user cancels the context provided to NewClientStream, or its deadline expires. (Note that it if the context is no longer needed before the deadline expires, it is still recommended to call cancel to prevent bloat.) It is always recommended to cancel contexts when they are no longer needed, and to never use the background context directly, so all users should always be doing this.
The user calls RecvMsg (or Recv in generated code) until a non-nil error is returned.
The user receives any error from Header or SendMsg (or Send in generated code) besides io.EOF.

If none of the above happen, this will leak a goroutine and a context, and grpc will not call the optionally-configured stats handler with a stats.End message.

Before this change, if a user created a stream and the server ended the stream, the stats handler would be invoked with a stats.End containing the final status of the stream. Subsequent calls to RecvMsg would then trigger the stats handler with InPayloads, which may be unexpected by stats handlers.

This change is

menghanl · 2018-02-08T01:44:34Z

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

clientconn.go, line 1384 at r1 (raw file):

// Deprecated: This error is never returned by grpc and should not be
// referenced by users.
var ErrClientConnTimeout = errors.New("grpc: timed out when dialing")

Why is this error moved here?

stream.go, line 131 at r1 (raw file):

	if mc.Timeout != nil && *mc.Timeout >= 0 {
		// The cancel function for this context will only be called when RecvMsg
		// returns non-nil error or another error is encountered, which means the

What does this mean?

Also move the comment to where cancel is defined? (And move the definition of cancel closer to if?)

stream.go, line 419 at r1 (raw file):

			// because the generated code requires it.
			if err == io.EOF && !cs.desc.ServerStreams {
				err = nil

return nil in the second if err == io.EOF on line 481 instead of setting it back to nil here?
So you can save this if, and finish() will always be called with io.EOF in the successful case.

stream.go, line 421 at r1 (raw file):

				err = nil
			}
			cs.finish(err)

So we will call finish() with nil in some cases. It should be OK though.

stream.go, line 491 at r1 (raw file):

	}
	cs.sentLast = true
	cs.t.Write(cs.s, nil, nil, &transport.Options{Last: true})

Add comment why we don't care the returned error.

stats/stats_test.go, line 1228 at r1 (raw file):

func TestClientStatsFullDuplexRPCNotCallingLastRecv(t *testing.T) {
	count := 1
	testClientStats(t, &testConfig{compress: "gzip"}, &rpcConfig{count: count, success: true, failfast: false, callType: fullDuplexStreamRPC, noLastRecv: true}, map[int]*checkFuncWithCount{

Also delete noLastRecv field from rpcConfig.

test/end2end_test.go, line 1167 at r1 (raw file):

		Payload:            payload,
	}
	if err := stream.Send(req); err != nil && err != io.EOF {

Should we send multiple times and expect io.EOF eventually?

test/end2end_test.go, line 3800 at r1 (raw file):

	cc := te.clientConn()
	tc := testpb.NewTestServiceClient(cc)
	ctx, cancel := context.WithCancel(context.Background())

Consider also defering this cancel? Otherwise a fatal below may cause this to be leaked.

Comments from Reviewable

dfawley · 2018-02-08T04:45:32Z

Review status: all files reviewed at latest revision, 8 unresolved discussions.

clientconn.go, line 1384 at r1 (raw file):