test: track simulated timers per dispatcher to simplify thread interactions and locking. by antoniovicente · Pull Request #12609 · envoyproxy/envoy

antoniovicente · 2020-08-12T14:04:47Z

Commit Message: test: track simulated timers per dispatcher to simplify thread interactions and locking.
Additional Description:
Risk Level: low, test-only
Testing: unit
Docs Changes:
Release Notes:
Issue #12585 #12638

… cross thread interactions. Signed-off-by: Antonio Vicente <avd@google.com>

…tcher thread to avoid data race found by tsan. Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-08-12T14:05:11Z

cc @mattklein123 @jmarantz

jmarantz

flushing comments as I'll be in meetings for quite a bit starting now.

This looks great so far.

jmarantz · 2020-08-12T14:10:27Z

test/test_common/simulated_time_system.cc

+
+  bool isEnabled(Alarm& alarm);
+  void enableAlarm(Alarm& alarm, const std::chrono::microseconds& duration);
+  void disableAlarm(Alarm& alarm) {


nit: ABSL_LOCKS_EXCLUDED(mutex_) here and similar APIs on this object.

jmarantz · 2020-08-12T14:11:20Z

test/test_common/simulated_time_system.cc

+
+  class AlarmSet {
+  public:
+    bool empty() { return alarms_.empty(); }


jmarantz · 2020-08-12T14:11:43Z

test/test_common/simulated_time_system.cc

+  public:
+    bool empty() { return alarms_.empty(); }
+
+    const AlarmRegistration& next() { return *alarms_.begin(); }


nit: ASSERT(!empty()) ?

jmarantz · 2020-08-12T14:15:06Z

test/test_common/simulated_time_system.cc

+    }
+
+  private:
+    std::set<AlarmRegistration> alarms_;


nit (this was pre-existing): call this ordered_alarms_, just for clarity at use?

changed to sorted_alarms_

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz

flushing more comments.

jmarantz · 2020-08-12T15:14:24Z

test/test_common/simulated_time_system.cc

+  MonotonicTime current_monotonic_time_ ABSL_GUARDED_BY(mutex_);
+  SystemTime current_system_time_ ABSL_GUARDED_BY(mutex_);
+
+  MonotonicTime next_monotonic_time_ ABSL_GUARDED_BY(mutex_);


what this means is the timestamp of the next scheduled event on this scheduler? Add comment?

I found that the two fields were not needed after all. Changed to just monotonic_time_ and system_time_

test/test_common/simulated_time_system.cc

jmarantz · 2020-08-12T15:21:43Z

test/test_common/simulated_time_system.cc

-  if (armed_) {
-    disableTimer();
+  if (!registered_alarms_.empty()) {
+    return registered_alarms_.next().time_;


is it possible that a registered but not-yet-triggered alarm will be earlier than a triggered alarm?

Fairly sure the answer was no, but I'm removing this method since it is no longer needed after Matt's awesome WaitFor refactor.

jmarantz · 2020-08-12T15:22:42Z

test/test_common/simulated_time_system.cc

-    time_system_.decPendingLockHeld();
+
+  if (duration.count() == 0) {
+    if (Runtime::runtimeFeatureEnabled(


should we cache this bool in the helper class? Or do we expect it to change during the course of a test method?

I can see someone setting runtime features after creating the SimulatedScheduler but before creating the first alarm. Does it hurt to check the runtime features this often?

mattklein123 · 2020-08-12T17:47:55Z

Given @antoniovicente is OOO for the rest of the week, let's continue in #12614.

…r_dispatcher Signed-off-by: Antonio Vicente <avd@google.com>

…ving draft status from the PR. Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz · 2020-08-23T20:04:54Z

please fix format so CI runs.

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz · 2020-08-23T20:09:42Z

test/test_common/simulated_time_system.cc

+  SchedulableCallbackPtr run_alarms_cb_;
+
+  absl::Mutex mutex_;
+  bool not_running_cbs_ ABSL_GUARDED_BY(mutex_) = true;


comment that this bool has negative sense to make it easier to use with absl::Condition.

Changed implementation to avoid the double negatives.

jmarantz · 2020-08-23T20:14:30Z

test/test_common/simulated_time_system.cc

+    }
+
+  private:
+    std::set<AlarmRegistration> sorted_alarms_;


should we have thread annotations on this member var and the methods above? or if it is to be left unguarded due to a threading discipline can you call that out in comments?

All synchronization for this object is external. See:
AlarmSet registered_alarms_ ABSL_GUARDED_BY(mutex_);
AlarmSet triggered_alarms_ ABSL_GUARDED_BY(mutex_);

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-08-25T00:11:27Z

test/server/BUILD


 envoy_cc_test(
    name = "guarddog_impl_test",
+    size = "small",


Explanation of this change:
Before other changes in this PR this test occasionally times out due to flaky alarm behavior as seen in #12638

This test usually runs in about 5s to 10s in standard confirgurations, and 20 seconds under clang-tsan. It may make sense to reduce the configured timeout for this test to small which has an associated timeout of 60 secs in order to get faster feedback when this test fails due to timeout.

antoniovicente · 2020-08-25T00:56:30Z

test/test_common/simulated_time_system.cc

+    }
+
+  private:
+    std::set<AlarmRegistration> sorted_alarms_;


All synchronization for this object is external. See:
AlarmSet registered_alarms_ ABSL_GUARDED_BY(mutex_);
AlarmSet triggered_alarms_ ABSL_GUARDED_BY(mutex_);

antoniovicente · 2020-08-25T02:34:05Z

test/test_common/simulated_time_system.cc

+  SchedulableCallbackPtr run_alarms_cb_;
+
+  absl::Mutex mutex_;
+  bool not_running_cbs_ ABSL_GUARDED_BY(mutex_) = true;


Changed implementation to avoid the double negatives.

mattklein123

Awesome, this makes sense at a high level. I didn't analyze all of the locking, etc. deeply. One API thought for consideration. Thank you for working on this!

/wait

mattklein123 · 2020-08-25T17:36:24Z

test/extensions/filters/http/adaptive_concurrency/controller/gradient_controller_test.cc

  }

+  template <typename DurationType> void advanceTimeAndLoop(DurationType duration) {
+    time_system_.advanceTimeAsync(duration);


WDYT about having advanceTimeAsync take a dispatcher and whether to run it blocking or non-blocking? It seems like it would be almost always broken to not do this pattern when using this API? If there are some cases I guess the dispatcher could be optional in the API but it would at least make test writers think about it?

@jmarantz @antoniovicente ping on this one. I see a new commit but I'm unsure of the plan here. If this is not needed we can merge but wanted to check.

/wait-any

Sorry, was doing some research in order to provide a complete reply to your comment.

As you noticed I rolled back this file since the changes to it are no longer required once I moved to updating monotonic time directly instead of updating next_monotonic_time and propagating it to event loops next time they run.

The idea of taking a dispatcher on advanceTimeAsync is an interesting suggestion. I found that most uses of this function happen in cases where there is a single active Dispatcher, but there's 1 exception to this in ServerStatsTest.FlushStats. Still it makes sense to have a SimulatedTimeSystem function that moves time forward and then runs the dispatcher event loop. Preferences for doing that change in this PR or a followup cleanup?

Preferences for doing that change in this PR or a followup cleanup?

Up to you, I'm fine either way. Thank you!

I'll do the cleanup as a followup.

Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123

Nice!

antoniovicente added 4 commits August 12, 2020 09:58

Track simulated timers by dispatcher in order to simplify locking and…

ff46275

… cross thread interactions. Signed-off-by: Antonio Vicente <avd@google.com>

Wait until idle when deleting simulated timers from outside the dispa…

9062235

…tcher thread to avoid data race found by tsan. Signed-off-by: Antonio Vicente <avd@google.com>

comment

810b4ae

Signed-off-by: Antonio Vicente <avd@google.com>

cleanup

75972c2

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz self-assigned this Aug 12, 2020

jmarantz reviewed Aug 12, 2020

View reviewed changes

jmarantz mentioned this pull request Aug 12, 2020

simtime alarm race condition #12585

Closed

jmarantz added a commit to jmarantz/envoy that referenced this pull request Aug 12, 2020

direct patch of envoyproxy#12609

b9b9773

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Aug 12, 2020

test: track simulated timers per dispatcher to simplify thread interactions and locking #12614

Closed

jmarantz reviewed Aug 12, 2020

View reviewed changes

mattklein123 closed this Aug 12, 2020

Merge remote-tracking branch 'upstream/master' into simulated_time_pe…

2dd0e2e

…r_dispatcher Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz reopened this Aug 20, 2020

antoniovicente added 2 commits August 21, 2020 12:40

fix test failures and address review comments in preparation for remo…

8aadbef

…ving draft status from the PR. Signed-off-by: Antonio Vicente <avd@google.com>

format

5e2ded6

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente marked this pull request as ready for review August 21, 2020 16:55

antoniovicente requested review from alyssawilk and mattklein123 as code owners August 21, 2020 16:55

add comment

16a7656

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente added 2 commits August 24, 2020 14:23

fix typo

8d9bd42

Signed-off-by: Antonio Vicente <avd@google.com>

update spelling dictionary

4a2c7b8

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz reviewed Aug 25, 2020

View reviewed changes

antoniovicente mentioned this pull request Aug 25, 2020

GuardDogActionsTest flake (ARM optimized) #12638

Closed

adjust implementation to avoid double negative

9b4c771

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente commented Aug 25, 2020

View reviewed changes

jmarantz previously approved these changes Aug 25, 2020

View reviewed changes

mattklein123 self-assigned this Aug 25, 2020

mattklein123 requested changes Aug 25, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 25, 2020

revert unnecessary changes to gradient_controller_test

30b8695

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed jmarantz’s stale review via 30b8695 August 25, 2020 21:37

repokitteh-read-only bot removed the waiting label Aug 25, 2020

jmarantz approved these changes Aug 26, 2020

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Aug 26, 2020

mattklein123 approved these changes Aug 26, 2020

View reviewed changes

mattklein123 merged commit d125b37 into envoyproxy:master Aug 26, 2020

antoniovicente mentioned this pull request Sep 15, 2020

test: replace advanceTimeAsync with advanceTimeAndRun #13122

Merged

Conversation

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

Uh oh!

Uh oh!

antoniovicente commented Aug 12, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Aug 12, 2020

Uh oh!

jmarantz commented Aug 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

antoniovicente commented Aug 12, 2020 •

edited

Loading