test: more fast-timeouts.#15959
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| ASSERT_TRUE(response->waitForEndStream()); | ||
| // The delayed close timeout should trigger since client is not closing the connection. | ||
| EXPECT_TRUE(codec_client_->waitForDisconnect(std::chrono::milliseconds(5000))); | ||
| EXPECT_TRUE(codec_client_->waitForDisconnect(std::chrono::milliseconds(10000))); |
There was a problem hiding this comment.
Why did you decide to increase the timeout here?
|
lgtm |
| } | ||
|
|
||
| void IntegrationStreamDecoder::waitForReset() { | ||
| AssertionResult IntegrationStreamDecoder::waitForReset(std::chrono::milliseconds timeout) { |
There was a problem hiding this comment.
Did you mean ABSL_MUST_USE_RESULT?
There was a problem hiding this comment.
Yep, seems like we want callers to check this?
There was a problem hiding this comment.
Ahh, maybe I can on this one - I couldn't on the other one I ported without changing signature, updating envoy filter examples, and then adding MUST_USE but I think Enovy filter examples doens't use this function
| void IntegrationStreamDecoder::waitForReset() { | ||
| AssertionResult IntegrationStreamDecoder::waitForReset(std::chrono::milliseconds timeout) { | ||
| if (!saw_reset_) { | ||
| Event::TimerPtr timer(dispatcher_.createTimer([this]() -> void { dispatcher_.exit(); })); |
There was a problem hiding this comment.
Maybe add a comment on how this works (or this is a common pattern?), it took me a minute. If it is a common pattern, can we extract this to some helper?
Also might want to note somewhere that this leaves the dispatcher in a bad state so you really want ASSERT vs EXPECT
There was a problem hiding this comment.
I don't think it leaves the dispatcher in a bad state, what do you think the problem is?
There was a problem hiding this comment.
I'm probably misunderstanding how this works, but seems to me like this stops the dispatcher after the timer expires? It would then mean that it's no longer running after this? But on the other hand we don't disable this so this will fire on its own at some point anyways, so I guess it can't be messing up the dispatcher. So what exactly does calling dispatcher_.exit() do?
There was a problem hiding this comment.
Ah, the dispatcher below is run in blocking mode, so it will loop for ever unless something tells it to exit.
Previously, it would exit if reset was received (or disconnect), now it also gives up after 5s.
|
It is unexpected that |
|
/wait |
|
Agree, Yan, it would be good to fix H2 so your waitForX didn't finish if X didn't happen, but I'm going to put off shaving that yak for another day :-) |
H2 waitForReset also picks up end stream, while H2 doesn't, so a bunch of QUIC tests were actually spinning on waitForReset until disconnect happened. Fixing it and fixing it forward by having a faster timeout which catches if we're waiting for the wrong thing. Risk Level: n/a Testing: passes locally at least =P Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
H2 waitForReset also picks up end stream, while H2 doesn't, so a bunch of QUIC tests were actually spinning on waitForReset until disconnect happened. Fixing it and fixing it forward by having a faster timeout which catches if we're waiting for the wrong thing. Risk Level: n/a Testing: passes locally at least =P Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
4 participants
H2 waitForReset also picks up end stream, while H2 doesn't, so a bunch of QUIC tests
were actually spinning on waitForReset until disconnect happened.
Fixing it and fixing it forward by having a faster timeout which catches if we're waiting for the wrong thing.
Risk Level: n/a
Testing: passes locally at least =P
Docs Changes: n/a
Release Notes: n/a