Fix race between activeStreams and bdp window size #5494

zasweq · 2022-07-11T17:27:05Z

The test case I added based off the issue test would successfully fail unexpected.EOF. This was caused by a race between when a stream was created and when updateFlowControl() was called. This fix makes sure any created streams has the correct window size on creation.

RELEASE NOTES:

Fixed dynamic BDP estimation causing unexpected EOF error by fixing race between activeStreams and bdp window size

dfawley · 2022-07-11T20:46:10Z

internal/transport/http2_client.go

+			if !checkForHeaderListSize(it) {
 				return false
 			}
-			if !checkForHeaderListSize(it) {
+			if !checkForStreamQuota(it) {
 				return false
 			}
 			return true


This can all be simply return checkForHeaderListSize(it) && checkForStreamQuota(it)

dfawley · 2022-07-11T20:49:54Z

test/end2end_test.go

+			if e, ok := status.FromError(err); ok {
+				if e.Code() != codes.ResourceExhausted {


This is simpler:

if code := status.Code(err); code != codes.ResourceExhausted { ... }

Note also that your test fails to detect if the returned error from UnaryCall is a non-status error (this isn't ever expected, of course, but is technically possible).

Switched and wrote a comment about your note.

wrote a comment about your note.

I don't understand. If you switched to my method instead of yours, my note doesn't apply anymore.

Oh duh true lol.

easwars

We also probably need a better release note. Else, we will get back to this when we make the release.

easwars · 2022-07-12T17:12:38Z

test/end2end_test.go

+		_, err := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{ResponseSize: 4194304})
+		if err != nil {


Nit: combine the assignment and the conditional statements.

easwars · 2022-07-12T17:12:50Z

test/end2end_test.go

+		_, err = ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{ResponseSize: 275075})
+		if err != nil {


Nit: combine the assignment and the conditional statements.

easwars · 2022-07-12T17:13:30Z

test/end2end_test.go

+			// this is testing for is an unexpected EOF with a status code
+			// INTERNAL so this is fine.
+			if code := status.Code(err); code != codes.ResourceExhausted {
+				t.Fatalf("unexpected err in UnaryCall: %v", err)


Could you mention the received and expected codes in the error string.

got, want := status.Code(err), codes.ResourceExhausted; got != want { t.Fatalf("UnaryCall RPC returned status code: %v, want %v", got, want) }

Printing the whole error is better than only printing the code, as there might be status message info that's useful:

t.Fatalf("UnaryCall RPC returned error: %v, want status code %v", err, want)

Switched to this:
if code := status.Code(err); code != codes.ResourceExhausted { t.Fatalf("UnaryCall RPC returned error: %v, want status code %v", err, codes.ResourceExhausted) }

easwars · 2022-07-12T17:16:35Z

test/end2end_test.go

+		// should work normally.
+		_, err = ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{ResponseSize: 275075})
+		if err != nil {
+			t.Fatalf("unexpected err in UnaryCall: %v", err)


Prefer concise error messages:

t.Fatalf("UnaryCall RPC failed: %v", err)

dfawley · 2022-07-12T17:23:24Z

internal/transport/http2_client.go

@@ -719,6 +718,11 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (*Stream,
 		t.nextID += 2
 		s.id = h.streamID
 		s.fc = &inFlow{limit: uint32(t.initialWindowSize)}
+		t.mu.Lock()
+		if t.activeStreams != nil { // Can be niled from Close()


if t.activeStreams == nil { return false } t.activeStreams[s.id] = s

zasweq

Thanks for the comments :D! I'm sure some tests will fail from this stream change though give it a second to look at again.

zasweq · 2022-07-12T21:00:00Z

test/end2end_test.go

+			// this is testing for is an unexpected EOF with a status code
+			// INTERNAL so this is fine.
+			if code := status.Code(err); code != codes.ResourceExhausted {
+				t.Fatalf("unexpected err in UnaryCall: %v", err)


Switched to this:
if code := status.Code(err); code != codes.ResourceExhausted { t.Fatalf("UnaryCall RPC returned error: %v, want status code %v", err, codes.ResourceExhausted) }

zasweq · 2022-07-12T21:06:41Z

internal/transport/http2_client.go

@@ -719,6 +718,11 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (*Stream,
 		t.nextID += 2
 		s.id = h.streamID
 		s.fc = &inFlow{limit: uint32(t.initialWindowSize)}
+		t.mu.Lock()
+		if t.activeStreams != nil { // Can be niled from Close()


dfawley · 2022-07-12T21:42:04Z

internal/transport/http2_client.go

@@ -1215,7 +1217,7 @@ func (t *http2Client) handleGoAway(f *http2.GoAwayFrame) {
 	default:
 		t.setGoAwayReason(f)
 		close(t.goAway)
-		t.controlBuf.put(&incomingGoAway{})
+		defer t.controlBuf.put(&incomingGoAway{})


Please add a comment explaining that we defer because t.mu is held

Added "Defer as t.mu is currently held".

dfawley · 2022-07-12T21:42:59Z

test/end2end_test.go

+			// This check doesn't fail on a non status error. However, the main
+			// this is testing for is an unexpected EOF with a status code
+			// INTERNAL so this is fine.


Delete now. It does fail on a non-status error since code would be Unknown in that case.

Whoops forgot deleted.

dfawley · 2022-07-12T21:45:05Z

test/end2end_test.go

+	for i := 0; i < 10; i++ {
+		// exceeds grpc.DefaultMaxRecvMessageSize, this should error with
+		// RESOURCE_EXHAUSTED error.
+		if _, err := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{ResponseSize: 4194304}); err != nil {


_, err := ss.Client.UnaryCall() if got := status.Code(err); got != codes.ResourceExhausted { t.Fatal }

This way you catch the case where the call doesn't fail (code will be OK), unexpectedly.

Ah good point switched.

zasweq

Thanks for the comments :D!

zasweq · 2022-07-12T22:07:21Z

test/end2end_test.go

+			// This check doesn't fail on a non status error. However, the main
+			// this is testing for is an unexpected EOF with a status code
+			// INTERNAL so this is fine.


Whoops forgot deleted.

zasweq · 2022-07-12T22:11:12Z

test/end2end_test.go

+	for i := 0; i < 10; i++ {
+		// exceeds grpc.DefaultMaxRecvMessageSize, this should error with
+		// RESOURCE_EXHAUSTED error.
+		if _, err := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{ResponseSize: 4194304}); err != nil {


Ah good point switched.

zasweq · 2022-07-12T22:12:12Z

internal/transport/http2_client.go

@@ -1215,7 +1217,7 @@ func (t *http2Client) handleGoAway(f *http2.GoAwayFrame) {
 	default:
 		t.setGoAwayReason(f)
 		close(t.goAway)
-		t.controlBuf.put(&incomingGoAway{})
+		defer t.controlBuf.put(&incomingGoAway{})


Added "Defer as t.mu is currently held".

Fix race between activeStreams and bdp window size

4459a50

zasweq requested a review from dfawley July 11, 2022 17:27

zasweq assigned dfawley Jul 11, 2022

zasweq added the Type: Bug label Jul 11, 2022

zasweq added this to the 1.49 Release milestone Jul 11, 2022

zasweq force-pushed the bug-fix branch from 8d93ecf to 6980fa7 Compare July 11, 2022 18:20

Fixed stream.fc being nil

f7515ef

zasweq force-pushed the bug-fix branch from 6980fa7 to f7515ef Compare July 11, 2022 18:45

dfawley reviewed Jul 11, 2022

View reviewed changes

zasweq added 3 commits July 11, 2022 19:09

drop lock before put handleGoAway

ad70290

Responded to Doug's comments

2465c21

Switched mutex unlock lock to defer

ff5bade

easwars reviewed Jul 12, 2022

View reviewed changes

easwars assigned zasweq and unassigned dfawley Jul 12, 2022

dfawley reviewed Jul 12, 2022

View reviewed changes

zasweq added 2 commits July 12, 2022 16:12

QAZSE discussion changes

30cc71c

Responded to Easwar's and Doug's comments

06f7a92

zasweq commented Jul 12, 2022

View reviewed changes

Unlocked mutex

067003e

zasweq assigned easwars and dfawley and unassigned zasweq Jul 12, 2022

dfawley reviewed Jul 12, 2022

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jul 12, 2022

Responded to Doug's comments

9af3575

zasweq commented Jul 12, 2022

View reviewed changes

zasweq assigned dfawley and unassigned zasweq Jul 12, 2022

dfawley approved these changes Jul 12, 2022

View reviewed changes

dfawley removed their assignment Jul 12, 2022

zasweq merged commit a094a10 into grpc:master Jul 13, 2022

zasweq added the Cherry Pick label Jul 18, 2022

brianneville mentioned this pull request Aug 2, 2022

gRPC connection closing with unexpected EOF on high-latency connections #5358

Closed

wimdec mentioned this pull request Sep 7, 2022

Deadlock on grpc transport #5644

Closed

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 Jan 15, 2023

dfawley removed the Cherry Pick label Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race between activeStreams and bdp window size #5494

Fix race between activeStreams and bdp window size #5494

zasweq commented Jul 11, 2022 •

edited

dfawley Jul 11, 2022

zasweq Jul 11, 2022

dfawley Jul 11, 2022

zasweq Jul 11, 2022

dfawley Jul 12, 2022

zasweq Jul 12, 2022

easwars left a comment

easwars Jul 12, 2022

zasweq Jul 12, 2022

easwars Jul 12, 2022

zasweq Jul 12, 2022

easwars Jul 12, 2022

dfawley Jul 12, 2022

zasweq Jul 12, 2022

easwars Jul 12, 2022

zasweq Jul 12, 2022

dfawley Jul 12, 2022

zasweq Jul 12, 2022

zasweq left a comment

zasweq Jul 12, 2022

zasweq Jul 12, 2022

dfawley Jul 12, 2022

zasweq Jul 12, 2022

dfawley Jul 12, 2022

zasweq Jul 12, 2022

dfawley Jul 12, 2022

zasweq Jul 12, 2022

zasweq left a comment

zasweq Jul 12, 2022

zasweq Jul 12, 2022

zasweq Jul 12, 2022

		if e, ok := status.FromError(err); ok {
		if e.Code() != codes.ResourceExhausted {

		_, err := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{ResponseSize: 4194304})
		if err != nil {

		_, err = ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{ResponseSize: 275075})
		if err != nil {

Fix race between activeStreams and bdp window size #5494

Fix race between activeStreams and bdp window size #5494

Conversation

zasweq commented Jul 11, 2022 • edited

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

easwars 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

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

zasweq commented Jul 11, 2022 •

edited