-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 flaky test QuicHttpIntegrationTest::DeferredLoggingWithRetransmission #27850
Conversation
Signed-off-by: Paul Sohn <[email protected]>
Signed-off-by: Paul Sohn <[email protected]>
Signed-off-by: Paul Sohn <[email protected]>
// Allow the response to be sent downstream again. | ||
socket_swap.write_matcher_->setWriteOverride(nullptr); | ||
{ | ||
SocketInterfaceSwap socket_swap(downstreamProtocol() == Http::CodecType::HTTP3 |
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.
This s/upstream/downstream
doesn't functionally change anything, but is semantically more correct.
/assign @alyssawilk |
socket_swap.write_matcher_->setDestinationPort(lookupPort("http")); | ||
socket_swap.write_matcher_->setWriteOverride(ebadf); | ||
upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); | ||
absl::SleepFor(absl::Seconds(TSAN_TIMEOUT_FACTOR)); |
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.
I'm sorry I overlooked this in the prior version. Why are we doing sleepfor? Is there a variable we can wait on instead?
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.
The sleep is to wait for a retransmission to occur (such that QuicStatsGatherer::OnPacketRetransmitted is executed). I couldn't think of a good integration-test-level event to wait on. Is there a good way to either 1) detect a retransmitted packet from the server to downstream, or 2) create a custom event to wait on from the QuicStatsGatherer?
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.
can we use timeSystem().advanceTimeWait ? I don't remember if this test uses simtime. If so let's switch over. if not, maybe we can try to dig up some stats
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, timeSystem().advanceTimeWait seems to work.
Signed-off-by: Paul Sohn <[email protected]>
…sion (envoyproxy#27850) Fixes envoyproxy#27841: Increase the sleep from 500->1000 ms. Properly clean up the SocketInterfaceSwap. Commit Message: Fix flaky test DeferredLoggingWithRetransmission Risk Level: N/A, just testing Signed-off-by: Paul Sohn <[email protected]> Signed-off-by: asheryer <[email protected]>
…sion (envoyproxy#27850) Fixes envoyproxy#27841: Increase the sleep from 500->1000 ms. Properly clean up the SocketInterfaceSwap. Commit Message: Fix flaky test DeferredLoggingWithRetransmission Risk Level: N/A, just testing Signed-off-by: Paul Sohn <[email protected]> Signed-off-by: Ryan Eskin <[email protected]>
Fixes #27841:
Commit Message: Fix flaky test DeferredLoggingWithRetransmission
Risk Level: N/A, just testing