test: Create SimulateTimeSystem for running tests #4340

jmarantz · 2018-09-05T02:15:05Z

Description: Add an alternate TimeSystem implementation which keeps manually-adjusted time and timer-callbacks making sense together. This is intended for use in most if not all unit tests, and many integration tests. This is another step to resolve #4160.

Risk Level: low -- this is a new test-only class that's not used anywhere yet.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

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

…nd monotonic-time. Remove a NUL char. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…s(n) Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

…calling Dispatcher::run() should be calling the timer callbacks. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

zuercher

Some nits on comments, but otherwise looks great.

zuercher · 2018-09-10T17:51:43Z

test/test_common/simulated_time_system.cc

+
+// Our simulated alarm inherits from TimerImpl so that the same dispatching
+// mechanism used in RealTimeSystem timers is employed for simulated alarms.
+// using libevent's event_active(). Note that libevent is placed into


nit: I think you can just delete the "using libevent's event_active()" bit.

zuercher · 2018-09-10T17:51:55Z

test/test_common/simulated_time_system.cc

+
+  /**
+   * Activates the timer so it will be run the next time the libevent loop is run,
+   * typiically via Dispatcher::run().


nit: typically

zuercher · 2018-09-10T17:52:27Z

test/test_common/simulated_time_system.cc

+// Compare two alarms, based on wakeup time and insertion order. Returns true if
+// a comes before b.
+
+// like strcmp (<0 for a < b, >0 for a > b), based on wakeup time and index.


nit: rm strcmp reference and blank line

zuercher · 2018-09-10T17:53:24Z

test/test_common/simulated_time_system.cc

+  return false;
+};
+
+// Each scheduler maintains its own timer


nit: time system instead of timer?

The TimeSystem is common across all the schedulers. At one point I thought each Scheduler would maintain its own set of timers, but it turns out to be easier to have the TimeSystem own the timers for every scheduler. The scheduler just holds onto the libevent_ base ptr, to allow activation when time moves forward. I'll add more commenting.

Reworded as:

// Each timer is maintained and ordered by a common TimeSystem, but is // associated with a scheduler. The scheduler creates the timers with a libevent // context, so that the timer callbacks can be executed via Dispatcher::run() in // the expected thread.

zuercher · 2018-09-10T17:53:56Z

test/test_common/simulated_time_system.h

+// Represents a simulated time system, where time is advanced by calling
+// sleep(), setSystemTime(), or setMonotonicTime(). systemTime() and
+// monotonicTime() are maintained in the class, and alarms are fired in response
+// to adjsutments in time.


nit: adjustments

zuercher · 2018-09-10T17:56:22Z

test/test_common/simulated_time_system.h

+   * along the way that have been scheduled to fire.
+   *
+   * @param duration The amount of time to sleep, expressed in any type that
+   * can be added to duration-cast to MonotonicTime::duration.


nit: I had a hard time parsing this, maybe switch the hypen to "when"?

reworded; to:

* @param duration The amount of time to sleep, expressed in any type that * can be duration_casted to MonotonicTime::duration.

zuercher · 2018-09-10T17:56:35Z

test/test_common/simulated_time_system.h

+  }
+
+  /**
+   * Sets the time forward monotonically. if the supplied argument moves


nit: capitalize if

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

zuercher

Thanks!

zuercher · 2018-09-10T22:36:53Z

@junr03 can you have a look at this one just for a second set of eyes?

junr03

a couple comments

junr03 · 2018-09-10T23:47:45Z

test/test_common/simulated_time_system.h

+
+private:
+  class SimulatedScheduler;
+  friend class SimulatedScheduler;


both here and below the friend class declaration is not needed. I just learned that since C++11 inner classes are automatically friends of the outer class.

nice...that is useful. done.

junr03 · 2018-09-10T23:49:30Z

test/test_common/simulated_time_system.h

+private:
+  class SimulatedScheduler;
+  friend class SimulatedScheduler;
+  class Alarm;


just wondering about the choice of forward declaring these two classes and then defining them in the cc file rather than the header file. I am not sure which is canonical c++.

Not sure about whether it is idiomatic...I have a practice of trying to put impl details into the cc where possible and not impactful to performance.

IMO that helps hide unimportant details from the user of the simulated time system, keep the impl details together, and minimize recompiles when changing impl.

But i can put them here if it is a strong envoy convention.

That seems to me like a fair assessment. Good to know the intention, and maybe something I can adopt in the future.

I mentioned it because in general what I have seen in the envoy codebase is the other way around, where, if small enough, implementation is brought into the header file. But I don't think there is a hard and fast rule.

@alyssawilk or @htuch just wondering if you lean in one way or another in this topic?

btw based on past experience, I think Envoy coding style of inling virtual dtors may contribute to slow link-time for test binaries, which we have a lot of.

In Chrome coding style this inlining of all ctors/dtors is discouraged, though IMO the virtual dtors are more of an issue.

The other consideration is rebuild time; if you have to make a change to a .h that is imported in 100 places you have 100 rebuilds, vs. the single .cc. I agree with @junr03 that we have a habit of putting small implementations in .h. I think this is because (i) it's less typing and (ii) more readable. I think link and rebuild time is a good argument for preferring .cc, but no strong preference.

Do we really need to do as much forward declaration here? Is this to break a cycle?

No, it's to declare friend classes that only need to be ref'd internal to the impl.

junr03 · 2018-09-11T00:04:23Z

test/test_common/simulated_time_system.cc

+  // That can only happen here in alarm->activate(), which is run with the mutex
+  // released.
+  if (monotonic_time >= monotonic_time_) {
+    while (!alarms_.empty()) {


nit: not sure if it is worth it or not. But I think this might be confusing to a new pair of eyes. It involves understanding that sets iterate in sorted order (and thanks to your overloaded operator this works), that we are modifying the set as we go, and that we are also moving time. I wonder if a comment spelling this out a bit would be helpful for others.

Added comment

// Alarms is a std::set ordered by wakeup time, so pulling off begin() each // iteration gives you wakeup order. Also note that alarms may be added // or removed during the call to activate() so it would not be correct to // range-iterate over the set.

clear, thanks!

junr03 · 2018-09-11T00:05:39Z

test/test_common/simulated_time_system.h

+  };
+  typedef std::set<Alarm*, CompareAlarms> AlarmSet;
+
+  void setMonotonicTimeAndUnlock(const MonotonicTime& monotonic_time) UNLOCK_FUNCTION(mutex_);


This function also changes system time, I wonder if we should rename.

Do you have a suggestion? Above I have the same issue 2x...maybe just a comment suffices?

Yeah, it is kind of funny because the type of time in the function name is not what you are setting, but what you are setting both times with... I guess in my mind setX is a function that sets X not a function that uses X to set other things.

Sorry, don't want to split too many hairs, so perhaps just a comment like the above suffices as you suggest.

This makes it possible to incorporate simulated time (#4340) into integration tests, driving toward resolution of #4160 . Risk Level: low Testing: //test/... Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

zuercher · 2018-09-14T22:41:20Z

.circleci/config.yml

@@ -10,7 +10,6 @@ jobs:
     resource_class: xlarge
     working_directory: /source
     environment:
-       BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/


I don't think this was related to the failures you saw. Let's make sure we revert this before merge.

agreed. but it won't pass CI without it, afaict

Looks like CI is starting to work again without removing the remote cache.

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

jmarantz added 3 commits September 4, 2018 20:20

add simulated-time class.

0c60fcd

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

Cleanup and add more tests for system time.

bcd14c7

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

Improve comments and clean up class visibility and hierarchy.a

10ab07b

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

jmarantz mentioned this pull request Sep 5, 2018

time: Plumb in TimeSystem into the integration test framework #4341

Merged

jmarantz added 10 commits September 5, 2018 07:32

Remove 'auto delta' temp as it broke compile on mac.

a7d40e1

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

Clarify some comments.

b0f9e08

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

Use duration_cast properly when applying deltas between system-time a…

a8d82f5

…nd monotonic-time. Remove a NUL char. Signed-off-by: Joshua Marantz <jmarantz@google.com>

format; and rename misleading sleep(n) test helper function to sleepM…

e1bb397

…s(n) Signed-off-by: Joshua Marantz <jmarantz@google.com>

Allow deleting armed timers -- they should disarm on delete. Add test…

c8cc5f1

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

Use the appropriate duration_cast in the sleep implementation as well.

32c6af3

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

Merge branch 'master' into sim-time

4ccaa4d

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

Don't run timer callbacks from sleep(), the existing production code …

d55dec4

…calling Dispatcher::run() should be calling the timer callbacks. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into sim-time

34e08b6

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

Merge branch 'master' into sim-time

68364e4

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

zuercher previously approved these changes Sep 10, 2018

View reviewed changes

Fix typos & reword some confusing or inaccurate comments.

c6dda49

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

jmarantz dismissed zuercher’s stale review via c6dda49 September 10, 2018 18:48

re-fix wording, which was still messy.

c55d43b

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

zuercher previously approved these changes Sep 10, 2018

View reviewed changes

zuercher assigned zuercher and junr03 Sep 10, 2018

junr03 reviewed Sep 11, 2018

View reviewed changes

jmarantz added 2 commits September 13, 2018 20:09

Merge branch 'master' into sim-time

944c9ea

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

Add comments and remove superfluous friend decls.

950cc7a

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

jmarantz dismissed zuercher’s stale review via 950cc7a September 14, 2018 17:17

jmarantz added 2 commits September 14, 2018 13:18

Kick CI

4ac133f

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

removing cache for now to see if CI works.

39921d1

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

zuercher reviewed Sep 14, 2018

View reviewed changes

jmarantz added 2 commits September 15, 2018 19:57

replace cache line to see if CI works now.

a6696f7

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

Merge branch 'master' into sim-time

bdd4c81

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

zuercher approved these changes Sep 17, 2018

View reviewed changes

junr03 approved these changes Sep 17, 2018

View reviewed changes

htuch merged commit 03e714e into envoyproxy:master Sep 17, 2018

jmarantz deleted the sim-time branch September 17, 2018 18:16

test: Create SimulateTimeSystem for running tests #4340

test: Create SimulateTimeSystem for running tests #4340

Uh oh!

Conversation

jmarantz commented Sep 5, 2018

Uh oh!

zuercher 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!

zuercher Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Sep 10, 2018

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

zuercher Sep 10, 2018 •

edited

Loading

jmarantz Sep 10, 2018 •

edited

Loading

junr03 Sep 10, 2018 •

edited

Loading