xdsclient: fix LRS stream leaks when errors are encountered

[dfawley]

RELEASE NOTES:

• xdsclient: fix goroutine leaks when load reporting is enabled

[easwars]

s/getStreamError/getStreamRecvError/?

[easwars]

What is trying to achieve?

[dfawley]

The error returned from Send is only ever io.EOF in the event of the server terminating the stream or a connection loss. This function receives the actual error from the stream.

grpc-go/stream.go

Lines 108 to 110 in 6417495

	// the stream. If the error was generated by the client, the status is
	// returned directly; otherwise, io.EOF is returned and the status of
	// the stream may be discovered using RecvMsg.

[easwars]

Is Recv always supposed to contain a more informational error on the client side, even if the error was encountered on the client? I'm thinking about the case where the error for Send happens on the client, but we still end up using the error we get from Recv. Is it possible that the former contained more contextual information?

[easwars]

My question is whether we should do this only when Send returns an error which is no io.EOF.

[dfawley]

Ah I see. Yes, that's a good idea: done.

[easwars]

Combine these two lines.

[dfawley]

Done.

[easwars]

Should there be a way for this for loop to terminate other than when Recv() returns non-nil error?

[dfawley]

stream.Recv will terminate with an error if the context for the RPC is canceled, which is also how sendLoads exits (in transport.go). So I believe this is fine.

[easwars]

RPC's monitoring goroutine? Which goroutine are you taking about here?

[dfawley]

This one:

grpc-go/stream.go

Lines 354 to 361 in 6417495

	go func() {
	select {
	case <-cc.ctx.Done():
	cs.finish(ErrClientConnClosing)
	case <-ctx.Done():
	cs.finish(toRPCErr(ctx.Err()))
	}
	}()

[easwars]

Should we do a defer cancel() and put everything that is currently inside of the for loop in an anonymous function?

[dfawley]

I have mixed feelings about this sort of thing. Done; let me know what you think.