test: deflaking unexpected disconnects permanently#12939
test: deflaking unexpected disconnects permanently#12939snowp merged 2 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
test/integration/fake_upstream.cc
Outdated
| dispatcher_(api_->allocateDispatcher("fake_upstream")), | ||
| handler_(new Server::ConnectionHandlerImpl(*dispatcher_)), | ||
| allow_unexpected_disconnects_(false), read_disable_on_new_connection_(true), | ||
| allow_unexpected_disconnects_(true), read_disable_on_new_connection_(true), |
There was a problem hiding this comment.
really my only question is if we should remove allow_unexpected_disconnects_ and set_allow_unexpected_disconnects() entirely, which will likely break downstream test builds, but it's only worth keeping around if someone really wants some test to allow_unexpected_disconnects_(false) explicitly. Maybe a follow-up so we can roll only that part back if someone wants that?
There was a problem hiding this comment.
I think doing it in a follow up makes sense. Do we not have any tests that want to test for unexpected disconnects right now?
There was a problem hiding this comment.
not that I know of? I mean we could arguably set it false for every test which didn't previously have it set true, but I think most tests which wanted to know they got a complete request waited for a complete requests, ditto response.
There was a problem hiding this comment.
I don't think that's necessary, probably fine to just get rid of for now unless someone complains
There was a problem hiding this comment.
I'm fine with a follow up but I would be fine just getting rid of it all now.
test/integration/fake_upstream.cc
Outdated
| dispatcher_(api_->allocateDispatcher("fake_upstream")), | ||
| handler_(new Server::ConnectionHandlerImpl(*dispatcher_)), | ||
| allow_unexpected_disconnects_(false), read_disable_on_new_connection_(true), | ||
| allow_unexpected_disconnects_(true), read_disable_on_new_connection_(true), |
There was a problem hiding this comment.
I think doing it in a follow up makes sense. Do we not have any tests that want to test for unexpected disconnects right now?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
went with Matt's request and did it in one fell swoop. LGTY? |
mattklein123
left a comment
There was a problem hiding this comment.
Awesome, just one comment for your thoughts on.
/wait-any
| if (unexpected_disconnect && !allow_disconnects) { | ||
| ENVOY_LOG_MISC(warn, "executeOnDispatcher failed due to disconnect\n"); | ||
| } |
There was a problem hiding this comment.
Is anyone ever going to see this? Delete also?
There was a problem hiding this comment.
My thought, and why I left it at warn, was if some executeOnDispatcher call asserts the call happens when in fact it's an unexpected disconnect, it'd be a pain to debug without at least some breadcrumb.
There was a problem hiding this comment.
OK SGTM. We can always iterate further later!
Backport crucial part of envoyproxy/envoy#12939 to get rid of flaky assertion failure. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Backport crucial part of envoyproxy/envoy#12939 to get rid of flaky assertion failure. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Allowing unexpected disconnects by default.
Risk Level: Low
Testing: tests pass
Docs Changes: n/a
Release Notes: n/a
Fixes #12861