testing: fix multiple race conditions in simulated time tests by mattklein123 · Pull Request #12527 · envoyproxy/envoy

mattklein123 · 2020-08-07T00:22:40Z

This PR fixes multiple race conditions in tests. The summary is:

All waitFor() operations are now fully synchronized.
waitFor() no longer moves simulated time and performs real sleeps in
all time systems. This means that all network operations are now
"instantaneous" and makes all time advances for alarms explicit. This
required fixes in a few tests but should make simulated time much easier
to reason about.
All timeout durations for network operations use real time for timeouts.

Risk Level: None for prod code, high for tests
Testing: Existing and fixed tests
Docs Changes: N/A
Release Notes: N/A

mattklein123 · 2020-08-07T00:24:24Z

This is not done and I'm still working through various issues but I wanted to let you see my current progress. I think the idea here is sound, however see my comment around waitFor() being inherently racy. I think we can fix this and cleanup a lot of callers by switching this to take a Mutex and a Condition and then using Await. I will work on this more later or tomorrow.

Ref: https://github.com/envoyproxy/envoy/pull/12527/files#diff-f2c85459672519c620a47b880b9b0d20R317-R319

jmarantz

cross-referencing #12539 which tries to switch integration tests to using await() rather than condvar, for more robust operation.

mattklein123 · 2020-08-08T00:20:04Z

@jmarantz I'm about to quit for today but this passes except for 2 tests which don't compile which should not be difficult to fix. From a test perspective this is a pretty scary change, but overall I think this makes everything much simpler to reason about and cleans up a bunch of stuff. Feel free to start reviewing and helping me to fix things.

mattklein123 · 2020-08-08T00:50:22Z

Running TSAN and seeing some errors. None of them look too bad to fix so will work on that next.

mattklein123 · 2020-08-08T22:44:38Z

No good deed goes unpunished. The TSAN issues are internal to abseil. I think they were recently fixed with:

9fc78436565eb3b204d4aa425ee3773354392f45 by Derek Mauro <dmauro@google.com>:

Use auto-detected sanitizer attributes for ASAN, MSAN, and TSAN builds

But now when I pull current abseil there are TSAN errors without any other changes: see abseil/abseil-cpp#760

mattklein123 · 2020-08-09T21:30:18Z

@jmarantz this passes all tests for me locally now under fastbuild and tsan. There are some flakes that I have hit. It's unclear if they are new or if they are pre-existing and exacerbated by the alternate TSAN lock implementation we are now using. It will be better to merge the other PR with the abseil bump first and see how that goes.

Fixes #12480 Fixes #10568 Signed-off-by: Matt Klein <mklein@lyft.com>

Signed-off-by: Matt Klein <mklein@lyft.com>

test/integration/fake_upstream.cc

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2020-08-13T19:36:51Z

@jmarantz this is passing all tests on fastbuild and I think is ready for real review. I'm going to start looking for flakes.

Signed-off-by: Matt Klein <mklein@lyft.com>

jmarantz

flushing comments; mostly nits

test/common/router/header_parser_fuzz_test.cc

test/common/formatter/substitution_formatter_fuzz_test.cc

test/integration/fake_upstream.cc

jmarantz

looks great; just a few nits, mostly about clarity and comments.

test/integration/fake_upstream.h

test/test_common/simulated_time_system_test.cc

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2020-08-13T22:50:33Z

@jmarantz updated. Great suggestion about the time bounds class. Much cleaner!

mattklein123 · 2020-08-14T00:03:44Z

ARM flake is a known different issue #12638

jmarantz

Awesome, thank you for finally cleaning this up!

Up to you if you want to apply the syntactic tweaks or just leave that for next time.

jmarantz · 2020-08-14T01:03:12Z

test/test_common/simulated_time_system_test.cc

+  auto thread = Thread::threadFactoryForTest().createThread([this, &mutex, &done]() {
+    for (;;) {
+      {
+        absl::MutexLock lock(&mutex);


taste test, as this syntax works now (for you golang fans):

for (;;) { if (absl::MutexLock lock(&mutex); done) { return; } base_scheduler_.run(Dispatcher::RunType::Block); }

Looking at this code is the first time it occurred to me to use it in C++.

Oh yeah that's good. I will fix that in a follow up. I want to get this merged so we can see how we are doing with flakes.

jmarantz · 2020-08-14T01:15:23Z

test/test_common/simulated_time_system_test.cc

+  auto thread = Thread::threadFactoryForTest().createThread([this, &mutex, &done]() {
+    for (;;) {
+      {
+        absl::MutexLock lock(&mutex);


golang syntax here if you like.

jmarantz · 2020-08-14T01:17:20Z

test/test_common/test_time_system.h

+  public:
+    template <class D>
+    RealTimeBound(const D& duration)
+        : end_time_(std::chrono::steady_clock::now() + duration) // NO_CHECK_FORMAT(real_time)


i'm guessing you were swayed to use this style by the convenience of not bothering to pass timeSystem() into the ctor here.

Regardless; this turned out really well. Thanks!

* master: (67 commits) logger: support log control in admin interface and command line option for Fancy Logger (envoyproxy#12369) test: fix http_timeout_integration_test flake (envoyproxy#12654) [fuzz]added an input check in writefilter fuzzer and added test cases (envoyproxy#12628) add 'explicit' restriction. (envoyproxy#12643) scoped_rds_integration_test migrate from api v2 to api v3. (envoyproxy#12633) fuzz: added fuzz test for listener filter tls_inspector (envoyproxy#12617) testing: fix multiple race conditions in simulated time tests (envoyproxy#12527) [tls] Move handshaking behavior into SslSocketInfo. (envoyproxy#12571) header: getting rid of exception-throwing behaviors in header files [the rest] (envoyproxy#12611) router: add new ratelimited retry backoff strategy (envoyproxy#12202) [redis_proxy] added a constraint for route.prefix().size() (envoyproxy#12637) network: add tcp listener backlog config (envoyproxy#12625) runtime: debug log that condition is always true when fractionalPercent numerator > denominator (envoyproxy#12068) WatchDog Extension hook (envoyproxy#12416) router: add dynamic metadata header formatter (envoyproxy#11858) statsd: revert visibility to public (envoyproxy#12621) Fix regression of /build_* in gitignore (envoyproxy#12630) Added a missing extension point to documentation. (envoyproxy#12620) Reverts proxy protocol test on windows (envoyproxy#12619) caching: Improved the tests and coverage of the CacheFilter tree (envoyproxy#12544) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

jmarantz reviewed Aug 7, 2020

View reviewed changes

mattklein123 force-pushed the fix_sim_time branch 4 times, most recently from b8e85f7 to 3b90839 Compare August 8, 2020 00:15

mattklein123 force-pushed the fix_sim_time branch from 3b90839 to 9add2b7 Compare August 8, 2020 22:33

mattklein123 force-pushed the fix_sim_time branch from 9add2b7 to 3d947c9 Compare August 9, 2020 21:05

mattklein123 mentioned this pull request Aug 10, 2020

Potential Dangling Reference in On Demand VHDS #12235

Closed

mattklein123 force-pushed the fix_sim_time branch from 3d947c9 to 30c6ef4 Compare August 10, 2020 23:46

mattklein123 mentioned this pull request Aug 11, 2020

simtime alarm race condition #12585

Closed

testing: fix multiple race conditions in simulated time tests

af6e1fd

Fixes #12480 Fixes #10568 Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 force-pushed the fix_sim_time branch from 30c6ef4 to af6e1fd Compare August 11, 2020 18:09

wrowe mentioned this pull request Aug 11, 2020

Draft: Diagnosing Windows integration test flakes #11904

Closed

mattklein123 added 8 commits August 11, 2020 21:02

fix

afab606

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/master' into fix_sim_time

9cef710

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/master' into fix_sim_time

dc4f3ac

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/master' into fix_sim_time

7d32b17

Signed-off-by: Matt Klein <mklein@lyft.com>

update

4a952a7

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

f2f7817

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/master' into fix_sim_time

2998f36

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

ed84220

Signed-off-by: Matt Klein <mklein@lyft.com>

jmarantz reviewed Aug 13, 2020

View reviewed changes

test/integration/fake_upstream.cc Show resolved Hide resolved

mattklein123 added 3 commits August 13, 2020 17:55

Merge remote-tracking branch 'origin/master' into fix_sim_time

e021b0a

Signed-off-by: Matt Klein <mklein@lyft.com>

more

42a3303

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

5a0d47b

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 marked this pull request as ready for review August 13, 2020 19:36

mattklein123 requested review from alyssawilk, junr03 and lizan as code owners August 13, 2020 19:36

mattklein123 changed the title ~~[WIP] testing: fix multiple race conditions in simulated time tests~~ testing: fix multiple race conditions in simulated time tests Aug 13, 2020

mattklein123 assigned jmarantz and antoniovicente Aug 13, 2020

fix

d77f54b

Signed-off-by: Matt Klein <mklein@lyft.com>

jmarantz reviewed Aug 13, 2020

View reviewed changes

mattklein123 mentioned this pull request Aug 13, 2020

network: add tcp listener backlog config #12625

Merged

jmarantz previously approved these changes Aug 13, 2020

View reviewed changes

test/integration/fake_upstream.h Outdated Show resolved Hide resolved

test/integration/fake_upstream.h Outdated Show resolved Hide resolved

test/test_common/simulated_time_system_test.cc Outdated Show resolved Hide resolved

fix

f452429

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 dismissed jmarantz’s stale review via f452429 August 13, 2020 22:48

jmarantz approved these changes Aug 14, 2020

View reviewed changes

mattklein123 merged commit a42a677 into master Aug 14, 2020

mattklein123 deleted the fix_sim_time branch August 14, 2020 02:45

wrowe mentioned this pull request Aug 17, 2020

Greenlight and reclassify //test/... with the current progress for Windows #12695

Merged

Conversation

mattklein123 commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Aug 8, 2020

Uh oh!

mattklein123 commented Aug 8, 2020

Uh oh!

mattklein123 commented Aug 8, 2020

Uh oh!

mattklein123 commented Aug 9, 2020

Uh oh!

Uh oh!

mattklein123 commented Aug 13, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Aug 13, 2020

Uh oh!

mattklein123 commented Aug 14, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

jmarantz Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

jmarantz Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattklein123 commented Aug 7, 2020 •

edited

Loading

mattklein123 commented Aug 7, 2020 •

edited

Loading