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

Fix goaway #3835

Merged
merged 5 commits into from
Nov 14, 2024
Merged

Fix goaway #3835

merged 5 commits into from
Nov 14, 2024

Conversation

mcollina
Copy link
Member

This PR changes how GOAWAY frames and user aborts are handled in HTTP/2.

In essence, we had a flaky test for a while that showed up locally on my machine after 2-3 hours of continuous runs. It's a bad race condition, which has been hard to find and a bad overlap between goaway, aborts, and manipulation of the Client state from the outside.

Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
@mcollina
Copy link
Member Author

I've opened this PR as draft with a lot of debug output, I will clean it up if this actually passes.

Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
@mcollina mcollina marked this pull request as ready for review November 14, 2024 10:16
Comment on lines 490 to 492
// The request was aborted
if (!request.aborted) {
request.onComplete([])
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just a little bit confused by the comment. Do you mean, The request was not aborted?

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

I dont see any obvious issue
LGTM

Signed-off-by: Matteo Collina <[email protected]>
@mcollina
Copy link
Member Author

@Uzlopak PTAL

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM :)

@mcollina mcollina merged commit 5ac4fb8 into main Nov 14, 2024
42 checks passed
@Uzlopak Uzlopak deleted the fix-goaway branch November 14, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants