transport: fix racey send to writes channel in WriteStatus

[gyuho]

Concurrent 'SendMsg' calls to stream lead to
multiple 'WriteStatus' calls, while closing
'writes' channel is not synchronized.

This patch marks 'streamDone' first before 'ht.do',
so that following 'WriteStatus' does not trigger panic
on 'writes' channel.

Address #1111.

c.f. etcd-io/etcd#8627.

[gyuho]

Reproducible case:

func TestHandlerTransport_HandleStreams_MultiWriteStatus(t *testing.T) {
	st := newHandleStreamTest(t)
	handleStream := func(s *Stream) {
		if want := "/service/foo.bar"; s.method != want {
			t.Errorf("stream method = %q; want %q", s.method, want)
		}
		st.bodyw.Close() // no body

		var wg sync.WaitGroup
		wg.Add(5)
		for i := 0; i < 5; i++ {
			go func() {
				defer wg.Done()
				st.ht.WriteStatus(s, status.New(codes.OK, ""))
			}()
		}
		wg.Wait()
	}
	st.ht.HandleStreams(
		func(s *Stream) { go handleStream(s) },
		func(ctx context.Context, method string) context.Context { return ctx },
	)
}

We are using grpc-go v1.6.0.

Thanks!

[gyuho]

Could anybody review this, please? /cc @menghanl @MakMukhi @dfawley

We are seeing the panics on this code path pretty often when closing down grpc-server with inflight streams.

type ServerTransport interface {
	...

	// WriteStatus sends the status of a stream to the client.  WriteStatus is
	// the final call made on a stream and always occurs.
	WriteStatus(s *Stream, st *status.Status) error

	...

I believe serverHandlerTransport.streamDone check is there to prevent redundant error writes from SendMsg. But it is possible that following SendMsg calls on the same stream error and WriteStatus before serverHandlerTransport.do completes, thus panic-ing on closing of closed ht.writes channel.

This patch just marks the stream as done before calling serverHandlerTransport.do, so subsequent error-ing SendMsg calls do not trigger panic on WriteStatus.

[menghanl]

The change LGTM. Thanks for the fix.
Can you also copy your test to handler_server_test.go?

[gyuho]

@menghanl PTAL.

And could we have patch release v1.6.1 with this?

Thanks a lot!

[menghanl]

Thanks for the quick fix.
The release date for 1.7.0 is next Wednesday (Oct 11).
Is it OK for you to wait for 1.7.0?

[gyuho]

@menghanl Sounds good. Thanks!

[menghanl]

Can please also you do a rebase?
The test failed because go get staticcheck failed (disabled in #1561). Thanks!

[gyuho]

@menghanl Just rebased with current master. Thanks!

[menghanl]

Thanks!
LGTM.