-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
grid: cleaning up potential zombie streams #31346
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
dabea30
to
ded03e0
Compare
1476bad
to
1f2cf44
Compare
this may still have a gcc test flake but I think it's ready for a review pass |
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.
Looks great! Thanks for doing this. A few small comments.
@@ -67,6 +67,18 @@ name: alternate_protocols_cache | |||
HttpProtocolIntegrationTest::initialize(); | |||
} | |||
|
|||
void writeFile(uint32_t index) { |
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: As long as you're here, would you mind renaming this method to something a bit more descriptive? maybe something like "writeAltSvcToFile()" or some such?
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
@@ -105,6 +120,7 @@ name: alternate_protocols_cache | |||
} | |||
throw EnvoyException("Failed to find a port after 10 tries"); | |||
} | |||
bool fill_cache_ = false; |
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: brief comment on the meaning of this. (Maybe name this something parallel with the new name of the writeFile()
method).
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
test/extensions/filters/http/alternate_protocols_cache/filter_integration_test.cc
Show resolved
Hide resolved
const std::chrono::milliseconds timeout = TestUtility::DefaultTimeout; | ||
|
||
initialize(); | ||
codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); |
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: is there an extra set of ()s around lookupPort
? Here and elsewhere.
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.
yep!
/wait on CI |
Fixing an issue where "prefetched" H3 streams were not cleaned up promptly.
Risk Level: low
Testing: new e2e tests
Docs Changes: n/a
Release Notes: inline
[Optional Runtime guard:] yes