Fix race between activeStreams and bdp window size by zasweq · Pull Request #5494 · grpc/grpc-go

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

+			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

+			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

+		_, 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

+		_, 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

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

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

 		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

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

 		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

 		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

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

+	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

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

+	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

 		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

		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 {

Conversation

zasweq commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zasweq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zasweq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zasweq commented Jul 11, 2022 •

edited

Loading