xdsclient: fix LRS stream leaks when errors are encountered #5505

dfawley · 2022-07-13T22:42:23Z

RELEASE NOTES:

xdsclient: fix goroutine leaks when load reporting is enabled

easwars · 2022-07-14T23:36:04Z

xds/internal/xdsclient/controller/version/v2/loadreport.go

@@ -149,5 +152,17 @@ func (v2c *client) SendLoadStatsRequest(s grpc.ClientStream, loads []*load.Data)

 	req := &lrspb.LoadStatsRequest{ClusterStats: clusterStats}
 	v2c.logger.Infof("lrs: sending LRS loads: %+v", pretty.ToJSON(req))
-	return stream.Send(req)
+	if err := stream.Send(req); err != nil {
+		return getStreamError(stream)


s/getStreamError/getStreamRecvError/?

What is trying to achieve?

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.

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?

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

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

easwars · 2022-07-14T23:36:17Z

xds/internal/xdsclient/controller/version/v2/loadreport.go

+		_, err := stream.Recv()
+		if err != nil {


Combine these two lines.

easwars · 2022-07-14T23:38:21Z

xds/internal/xdsclient/controller/version/v2/loadreport.go

+}
+
+func getStreamError(stream lrsStream) error {
+	for {


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

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 · 2022-07-14T23:39:28Z

xds/internal/xdsclient/controller/transport.go

-		stream, err := t.vClient.NewLoadStatsStream(ctx, cc)
+		// streamCtx is created and canceled in case we terminate the stream
+		// early for any reason, to avoid gRPC-Go leaking the RPC's monitoring
+		// goroutine.


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

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 · 2022-07-14T23:40:16Z

xds/internal/xdsclient/controller/transport.go

 		if err != nil {
 			t.logger.Warningf("lrs: failed to create stream: %v", err)
+			cancel()


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

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

xdsclient: fix LRS stream leaks when errors are encountered

58bbdbf

dfawley added Type: Bug Cherry Pick labels Jul 13, 2022

dfawley added this to the 1.49 Release milestone Jul 13, 2022

dfawley requested a review from easwars July 14, 2022 14:56

dfawley assigned easwars Jul 14, 2022

easwars reviewed Jul 14, 2022

View reviewed changes

easwars assigned dfawley and unassigned easwars Jul 14, 2022

dfawley added 2 commits July 14, 2022 23:56

review comments

18ac24d

stray cancel

bb503c3

easwars assigned easwars and unassigned dfawley Jul 15, 2022

return non-io.EOF errors

0533333

easwars approved these changes Jul 15, 2022

View reviewed changes

easwars assigned dfawley and unassigned easwars Jul 15, 2022

dfawley merged commit 3a77d29 into grpc:master Jul 15, 2022

dfawley deleted the lrs_fix branch July 15, 2022 18:58

moul-bot mentioned this pull request Oct 7, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.0 moul/depviz#581

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2023

dfawley removed the Cherry Pick label Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdsclient: fix LRS stream leaks when errors are encountered #5505

xdsclient: fix LRS stream leaks when errors are encountered #5505

dfawley commented Jul 13, 2022

easwars Jul 14, 2022

easwars Jul 14, 2022

dfawley Jul 14, 2022

easwars Jul 15, 2022

easwars Jul 15, 2022

dfawley Jul 15, 2022

easwars Jul 14, 2022

dfawley Jul 14, 2022

easwars Jul 14, 2022

dfawley Jul 14, 2022

easwars Jul 14, 2022

dfawley Jul 14, 2022

easwars Jul 14, 2022

dfawley Jul 14, 2022

	// 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.

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

xdsclient: fix LRS stream leaks when errors are encountered #5505

xdsclient: fix LRS stream leaks when errors are encountered #5505

Conversation

dfawley commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment