Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

transport: add a draining state check before creating streams #6142

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Mar 20, 2023

Fixes #3686

RELEASE NOTES:

  • client: fix race between stream creation and GOAWAY receipt, which could lead to spurious UNAVAILABLE stream errors.

@dfawley dfawley added this to the 1.55 Release milestone Mar 20, 2023
@dfawley dfawley requested a review from zasweq March 20, 2023 23:53
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@@ -844,17 +844,11 @@ func (s) TestGracefulClose(t *testing.T) {
wg.Add(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can you switch the docstring for this code block from "Expect the failure for all the follow-up streams because ct has been closed gracefully." to something like "expect errors trying to create new streams after the client transport has been Gracefully Closed (and is in draining)".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}
t.Errorf("_.NewStream(_, _) = _, %v, want _, %v", err, ErrConnClosing)
}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can you add a top level docstring to this test explaining what this is actually testing (I know it wasn't there previously? I see ct.GracefulClose(), new streams not being able to be created, and a write and a read (expecting io.EOF) from the already created stream. I'm assuming move "// The stream which was created before graceful close can still proceed." from before the wg.Wait (which is waiting on the failed streams for loop) to before the ct.Write. Please add top level docstring that explains this test is gracefully closing a client transport and expecting these two downstream effects (new stream creation fails, and also previously created streams still continue to be able to be operated on and are still functional (all the time or in certain conditions/timebound etc.?)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@zasweq zasweq assigned dfawley and unassigned zasweq Mar 21, 2023
@dfawley dfawley assigned zasweq and unassigned dfawley Mar 21, 2023
@dfawley dfawley closed this Mar 21, 2023
@dfawley dfawley reopened this Mar 21, 2023
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@zasweq zasweq assigned dfawley and unassigned zasweq Mar 21, 2023
@dfawley dfawley merged commit 7651e62 into grpc:master Mar 21, 2023
1 check passed
@dfawley dfawley deleted the statechk branch March 21, 2023 20:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: Test/GracefulClientOnGoAway
2 participants