transport: Fix deadlock in transport caused by GOAWAY race with new stream creation #5652

zasweq · 2022-09-13T03:16:38Z

RELEASE NOTES:

transport: Fix deadlock in transport caused by GOAWAY race with new stream creation (add add'l comment about how it only occurs with a non-well-behaved HTTP/2 server implementation)

dfawley · 2022-09-13T15:57:14Z

internal/transport/http2_client.go

+	activeStreams := make(map[uint32]*Stream)
 	for streamID, stream := range t.activeStreams {
+		activeStreams[streamID] = stream
+	}


Use a slice; only add streams to be closed to it. Early exit if t.activeStreams is zero?

t.prevGoAwayID = id if len(t.activeStreams) == 0 { t.mu.Unlock() t.Close(...) return } streamsToClose := make([]*Stream, 0, len(t.activeStreams)) for streamID, stream := range t.activeStreams { if streamID > id && streamID <= upperLimit { streamsToClose = append(streamsToClose, stream) } } t.mu.Unlock() for _, stream := range(streamsToClose) { atomic.StoreUint32() t.closeStream() }

Why is it important to have the cap(streamsToClose) len(t.activeStreams)?

Actually, sorry, I tried this way and for some reason it still induced deadlock. I'm going to keep it as is. I couldn't figure out why your way wouldn't work.

Done. Verified on my e2e_test I checked in and also on the transport test which I didn't check in since it wrote directly to framer.

zasweq

Thanks for the comment. Sorry I couldn't get your solution working. The e2e test with the new helper class that actually induced it took everything out of me.

zasweq · 2022-09-15T20:49:31Z

internal/transport/http2_client.go

+	activeStreams := make(map[uint32]*Stream)
 	for streamID, stream := range t.activeStreams {
+		activeStreams[streamID] = stream
+	}


Why is it important to have the cap(streamsToClose) len(t.activeStreams)?

zasweq · 2022-09-15T21:02:10Z

internal/transport/http2_client.go

+	activeStreams := make(map[uint32]*Stream)
 	for streamID, stream := range t.activeStreams {
+		activeStreams[streamID] = stream
+	}


Actually, sorry, I tried this way and for some reason it still induced deadlock. I'm going to keep it as is. I couldn't figure out why your way wouldn't work.

dfawley

LGTM, thanks! Just a few really small things.

dfawley · 2022-09-21T16:21:18Z

internal/transport/http2_client.go

+		return
+	}
+
+	streamsToClose := make([]*Stream, 0)


var streamsToClose []*Stream

Or delete the ,0 but make is unnecessary for a slice either way if you aren't allocating anything.

Switched. You initially suggested make for the capacity length. Why was that?

If you specify the capacity then it won't need to do reallocations as you add to it. In this case, since we don't have any idea how many active streams are before/after the ID (and it isn't worth computing it first) then this should be fine. It will need reallocations (Go doubles the slice capacity as you add so it's O(logN)), but this is a rare case and not worth optimizing.

dfawley · 2022-09-21T16:22:13Z

internal/transport/http2_client.go

 	t.mu.Unlock()
-	if active == 0 {
-		t.Close(connectionErrorf(true, nil, "received goaway and there are no active streams"))
+	for _, stream := range streamsToClose {


Please add a comment about why this is called outside t.mu.

test/clienttester.go

dfawley · 2022-09-21T16:27:07Z

test/end2end_test.go

+			t.Errorf("error in lis.Accept(): %v", err)
+		}
+		ct := newClientTester(t, conn)
+		ct.greet()


Fold this into new?

Sure, but I'm doing this very begrudingly. ServerTester is done this way, and I feel like it' made a lot clearer to the user of the clientTester struct that it gets to the point where the client is happy with it's HTTP2 connection establisment.

dfawley · 2022-09-21T16:29:25Z

test/end2end_test.go

+	ct, ok := val.(*clientTester)
+	if !ok {
+		t.Fatalf("value received not a clientTester")
+	}


Just let it panic and keep the code simpler. This is just a test and the panic is as informative as this error, and it's a coding error if it happens. ct := val.(*clientTester)

dfawley · 2022-09-21T16:36:09Z

test/end2end_test.go

+			if i == 10 {
+				<-goAwayWritten.Done()
+			}


What does this do that is different from blocking on <-goAwayWritten.Done() immediately after someStreamsCreate.Fire() instead?

I honestly was just playing around with this until it induced deadlock, it was hard to induce on master. This allows it to induce it because the only way to induce it is to concurrently create a stream (controlBuf.mu then transport.mu), where the closeStream was (transport.mu then controlBuf.mu). If I block where you suggested, there are no concurrent streams trying to be created while the goAway is being sent from server.

zasweq

Thanks for the comments :D!

zasweq · 2022-09-21T17:10:57Z

internal/transport/http2_client.go

+		return
+	}
+
+	streamsToClose := make([]*Stream, 0)


Switched. You initially suggested make for the capacity length. Why was that?

zasweq · 2022-09-21T17:14:51Z

test/end2end_test.go

+	ct, ok := val.(*clientTester)
+	if !ok {
+		t.Fatalf("value received not a clientTester")
+	}


zasweq · 2022-09-21T17:18:59Z

test/end2end_test.go

+			if i == 10 {
+				<-goAwayWritten.Done()
+			}


I honestly was just playing around with this until it induced deadlock, it was hard to induce on master. This allows it to induce it because the only way to induce it is to concurrently create a stream (controlBuf.mu then transport.mu), where the closeStream was (transport.mu then controlBuf.mu). If I block where you suggested, there are no concurrent streams trying to be created while the goAway is being sent from server.

zasweq · 2022-09-21T17:22:17Z

test/end2end_test.go

+			t.Errorf("error in lis.Accept(): %v", err)
+		}
+		ct := newClientTester(t, conn)
+		ct.greet()


Sure, but I'm doing this very begrudingly. ServerTester is done this way, and I feel like it' made a lot clearer to the user of the clientTester struct that it gets to the point where the client is happy with it's HTTP2 connection establisment.

test/clienttester.go

zasweq · 2022-09-21T17:34:37Z

internal/transport/http2_client.go

 	t.mu.Unlock()
-	if active == 0 {
-		t.Close(connectionErrorf(true, nil, "received goaway and there are no active streams"))
+	for _, stream := range streamsToClose {


dfawley · 2022-09-21T18:13:24Z

internal/transport/http2_client.go

+		return
+	}
+
+	streamsToClose := make([]*Stream, 0)


If you specify the capacity then it won't need to do reallocations as you add to it. In this case, since we don't have any idea how many active streams are before/after the ID (and it isn't worth computing it first) then this should be fine. It will need reallocations (Go doubles the slice capacity as you add so it's O(logN)), but this is a rare case and not worth optimizing.

test/clienttester.go

zasweq · 2022-10-20T19:50:17Z

I clearly see this in the 1.50.x commit history: https://github.com/grpc/grpc-go/commits/v1.50.x. Scroll to 29 days ago.

zasweq added the Type: Bug label Sep 13, 2022

zasweq added this to the 1.50 Release milestone Sep 13, 2022

zasweq requested a review from dfawley September 13, 2022 03:16

zasweq assigned dfawley Sep 13, 2022

zasweq force-pushed the fix-transport-deadlock branch 2 times, most recently from 9bd4117 to 05537c1 Compare September 13, 2022 05:04

Fixed deadlock in transport

6004b0d

zasweq force-pushed the fix-transport-deadlock branch from 05537c1 to 6004b0d Compare September 13, 2022 05:14

dfawley reviewed Sep 13, 2022

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Sep 14, 2022

zasweq commented Sep 15, 2022

View reviewed changes

Switched transport level test to e2e test

3e5eae7

zasweq assigned dfawley and unassigned zasweq Sep 15, 2022

Responded to Doug's comment

09bdda8

dfawley reviewed Sep 21, 2022

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Sep 21, 2022

zasweq commented Sep 21, 2022

View reviewed changes

Responded to Doug's comments

f9a0d75

zasweq assigned dfawley and unassigned zasweq Sep 21, 2022

dfawley approved these changes Sep 21, 2022

View reviewed changes

dfawley changed the title ~~transport: Fix deadlock in transport~~ transport: Fix deadlock in transport caused by GOAWAY race with new stream creation Sep 21, 2022

dfawley assigned zasweq and unassigned dfawley Sep 21, 2022

zasweq merged commit b1d7f56 into grpc:master Sep 21, 2022

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

moul-bot mentioned this pull request Oct 17, 2022

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

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport: Fix deadlock in transport caused by GOAWAY race with new stream creation #5652

transport: Fix deadlock in transport caused by GOAWAY race with new stream creation #5652

zasweq commented Sep 13, 2022 •

edited by dfawley

Loading

dfawley Sep 13, 2022

zasweq Sep 15, 2022

zasweq Sep 15, 2022

zasweq Sep 19, 2022

zasweq left a comment

zasweq Sep 15, 2022

zasweq Sep 15, 2022

dfawley left a comment

dfawley Sep 21, 2022

zasweq Sep 21, 2022

dfawley Sep 21, 2022

dfawley Sep 21, 2022

zasweq Sep 21, 2022

dfawley Sep 21, 2022

zasweq Sep 21, 2022

dfawley Sep 21, 2022

zasweq Sep 21, 2022

dfawley Sep 21, 2022

zasweq Sep 21, 2022

zasweq left a comment

zasweq Sep 21, 2022

zasweq Sep 21, 2022

zasweq Sep 21, 2022

zasweq Sep 21, 2022

zasweq Sep 21, 2022

dfawley Sep 21, 2022

zasweq commented Oct 20, 2022

transport: Fix deadlock in transport caused by GOAWAY race with new stream creation #5652

transport: Fix deadlock in transport caused by GOAWAY race with new stream creation #5652

Conversation

zasweq commented Sep 13, 2022 • edited by dfawley Loading

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

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a 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

zasweq left a 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

zasweq commented Oct 20, 2022

zasweq commented Sep 13, 2022 •

edited by dfawley

Loading