-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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)".
There was a problem hiding this comment.
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) | ||
}() |
There was a problem hiding this comment.
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.?)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fixes #3686
RELEASE NOTES: