time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers #4257

jmarantz · 2018-08-26T14:20:23Z

Description: Unifies the TimeSource so that system and monotonic sources are always handled similarly (mocks, real-time, or in the future, simulated), and derives a new interface, Event::TimeSystem, which managers timers. This is a pure refactor; none of the operation or semantics of any production or test code is changed with this PR. This is another step in resolving #4160.

Risk Level: medium, just because PR is large and merge conflicts happen, but it should be pretty safe
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <[email protected]>

…hanged mocks. Signed-off-by: Joshua Marantz <[email protected]>

…(at least one with my own new assert). Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

…rkerFactory::createWorker, Signed-off-by: Joshua Marantz <[email protected]>

…nt an assert and made it not fire. Signed-off-by: Joshua Marantz <[email protected]>

…eeds is a MonotonicTimeSource, so no need for the added test complexity. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

…ock-time. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

…(tests excluded). Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

This reverts commit 5338985. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

…to use 'SimulatedTestTime' which hasn't been built yet. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

…if they are not copyable. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

…Source as distinct class hierarchies. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

dnoe · 2018-08-29T20:25:24Z

Does removal of the WIP imply this one is ready for reviewer assignment?

jmarantz · 2018-08-29T20:26:24Z

Yes I think this is ready for review.

Signed-off-by: Joshua Marantz <[email protected]>

dnoe

I really like the better support for simulated time here, and getting all of this infrastructure in place will make it much easier to do things that have been tricky in the past.

A few nits and some feedback about using duration_cast to address.

dnoe · 2018-08-30T15:27:19Z

include/envoy/event/timer.h

+typedef std::unique_ptr<TimerFactory> TimerFactoryPtr;
+
+/**
+ * Captures a system for measuring time and setting timers with callbacks.


nit: This phrase seems weird to me, both the use of "Captures" (I think you mean that this interface includes both measuring time and setting timers) and the fact that it sounds like it allows you to set timers using a callback, but I think it actually allows you to set timers that call callbacks when the timer fires.

reworded:

/** * Interface providing a mechanism to measure time and set timers that run callbacks * when the timer fires. */

dnoe · 2018-09-02T00:56:06Z

include/envoy/server/instance.h

@@ -194,8 +195,9 @@ class Instance {

  /**
   * @return the time source used for the server.
+   * TODO(jmarantz): rename this to timeSystem().


Above you had a PR number for the timeSystem() rename TODO, might want to be consistent either way.

source/common/common/utility.h

source/common/event/dispatcher_impl.h

dnoe · 2018-09-02T01:13:08Z

source/common/event/real_time_system.cc

+    } else {
+      std::chrono::microseconds us = std::chrono::duration_cast<std::chrono::microseconds>(d);
+      timeval tv;
+      tv.tv_sec = us.count() / 1000000;


The conversion factors here could be more readable by using duration cast:

auto secs = std::chrono::duration_cast<std::chrono::seconds>(d); auto usecs = std::chrono::duration_cast<std::chrono::microseconds>(d - secs); tv.tv_secs = secs.count(); tv.tv_usecs = usecs.count();

Agreed, but I have strictly moved this code verbatim from timer_impl.h and would prefer not to rewrite it in this PR.

Actually I decided to restore timer_impl.h and timer_impl.cc which reveals the 'verbatim' note above to be a lie; there is a slight change which is more visible now (the timers are constructed with a libevent base rather than a dispatcher).

I did this in anticipation of delegating to real-time TimerImpl when building simulated-time TimerImpl in the next PR, but it also makes this diff cleaner :). In simulated time, the real-time timer delegate won't be enabled until the simulated time has advanced enough for it to fire, but I still want it to fire via the libevent dispatcher mechanism.

dnoe · 2018-09-02T01:17:23Z

source/common/event/real_time_system.h

+namespace Event {
+
+/**
+ * Real-time implementation of TimeSystem.


Same comment here - RealTimeSystem reads OK, but "Real-time" makes it sound like it's some extra high resolution thing. How about "Real clock implementation of TimeSystem"?

Changed to "Real-world time"

dnoe · 2018-09-02T01:26:02Z

test/test_common/test_time.h

@@ -13,16 +13,11 @@ class DangerousDeprecatedTestTime {
 public:
  DangerousDeprecatedTestTime();

-  TimeSource& timeSource() { return time_source_; }
+  // TODO(jmarantz) rename this method and all call-sites to timeSystem().


Use PR/issue number TODO?

dnoe · 2018-09-02T01:27:37Z

tools/check_format.py

@@ -26,7 +26,9 @@
 # Files matching these exact names can reference prod time. These include the class
 # definitions for prod time, the construction of them in main(), and perf annotation.
 # For now it includes the validation server but that really should be injected too.
-PROD_TIME_WHITELIST = ('./source/common/common/utility.h',
+REAL_TIME_WHITELIST = ('./source/common/common/utility.h',


Can you update the comment above to consistently refer to this as "real clock time" or whatever sounds good? It still calls it "prod time" in the comment.

dnoe · 2018-09-02T01:27:56Z

tools/check_format.py

@@ -77,8 +79,8 @@ def whitelistedForProtobufDeps(file_path):
 # Production time sources should not be instantiated in the source, except for a few


Real clock time sources should not be...

dnoe · 2018-09-02T01:28:47Z

tools/check_format.py

-  if not whitelistedForProdTime(file_path):
-    if 'ProdSystemTimeSource' in line or 'ProdMonotonicTimeSource' in line:
+  if not whitelistedForRealTime(file_path):
+    if 'RealTimeSource' in line or 'RealTimeSystem' in line:
      reportError("Don't reference real-time sources from production code; use injection")


I realize this isn't in your PR, but I still think we should lose the dash in real-time since it make it sound like a RTOS reference.

…w comments. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

…n SimulatedTimeSystem. Signed-off-by: Joshua Marantz <[email protected]>

Signed-off-by: Joshua Marantz <[email protected]>

… use thread-safe post() to wake up the correct threads. Signed-off-by: Joshua Marantz <[email protected]>

…asier to use thread-safe post() to wake up the correct threads." This reverts commit 3fe902f. Signed-off-by: Joshua Marantz <[email protected]>

dnoe

I'm not sure I understood your comment about not using duration_cast. I'd definitely prefer to see the more idiomatic C++ style way rather than math on struct timeval with ones-with-lots-of-zeros code, so if it won't be fixed in this PR can you create a GH issue and TODO for cleaning it up?

dnoe · 2018-09-04T13:28:40Z

source/common/common/utility.h

@@ -96,7 +96,7 @@ class AccessLogDateTimeFormatter {
 };

 /**
- * Real-time implementation of TimeSource.
+ * Real-world time implementation of TimeSource.


Perfect wording, thanks!

Signed-off-by: Joshua Marantz <[email protected]>

jmarantz · 2018-09-04T13:52:09Z

Added TODO for duration_cast hacking.

dnoe · 2018-09-04T14:03:19Z

Assigned @htuch for senior maintainer.

htuch

Looks like a nice cleanup.

…stem() as appropriate (#4343) In #4257, Event::TimeSystem was introduced to augment the TimeSource abstraction to include timers. Most places in the system that had previously held onto a TimeSource held it in a member variable time_source_ and, if appropriate, exposed it as a method timeSource(), and #4257 changed those to Event::TimeSystem without renaming them, in order to minimize diffs. This PR just renames the members and methods to time_system_ and timeSystem() as appropriate. This is part of the chain of CLs to resolve #4160. Risk Level: low -- no functional risk, but depending on timing of when this gets merged, an easily-remedied build breakage could arise. Testing: //test/... Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz <[email protected]>

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <[email protected]>

jmarantz added 30 commits August 15, 2018 23:54

Remove singletons for time-sources and inject them instead.

da7f24a

Signed-off-by: Joshua Marantz <[email protected]>

checkpoint with more stuff compiling, a few tests still fail due to c…

7d59de4

…hanged mocks. Signed-off-by: Joshua Marantz <[email protected]>

checkpoinging: all of //test/common/... compiles, 4 tests still fail …

10422d4

…(at least one with my own new assert). Signed-off-by: Joshua Marantz <[email protected]>

//test/common/... all pass.

282307f

Signed-off-by: Joshua Marantz <[email protected]>

More of //test/... builds, some of it works.

fa5c8c8

Signed-off-by: Joshua Marantz <[email protected]>

All tests compiling and working.

8d67f55

Signed-off-by: Joshua Marantz <[email protected]>

More comments and TODOs.

c9a7d62

Signed-off-by: Joshua Marantz <[email protected]>

Pass the TimeSource into the WorkerFactory's ctor rather than into Wo…

2d9f2aa

…rkerFactory::createWorker, Signed-off-by: Joshua Marantz <[email protected]>

Fix race found in coverage tests due to order of destruction. Uncomme…

1a84061

…nt an assert and made it not fire. Signed-off-by: Joshua Marantz <[email protected]>

Don't bother using a whole TimeSource for TokenBucketImpl -- all it n…

eb2347e

…eeds is a MonotonicTimeSource, so no need for the added test complexity. Signed-off-by: Joshua Marantz <[email protected]>

More API cleanups and TODOs.

21ac418

Signed-off-by: Joshua Marantz <[email protected]>

Kick CI

0a2514c

Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'master' into inject-time-source

c642dbc

Signed-off-by: Joshua Marantz <[email protected]>

formatting.

646a500

Signed-off-by: Joshua Marantz <[email protected]>

Add comment about preserving entropy in the future, when we move to m…

e65a754

…ock-time. Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'master' into inject-time-source

50dbb33

Signed-off-by: Joshua Marantz <[email protected]>

Add check_format for instantiating time-sources in a new source file …

c1f0dba

…(tests excluded). Signed-off-by: Joshua Marantz <[email protected]>

Address review comments.

5338985

Signed-off-by: Joshua Marantz <[email protected]>

Revert "Address review comments."

edba780

This reverts commit 5338985. Signed-off-by: Joshua Marantz <[email protected]>

Address review comments.

be44f2d

Signed-off-by: Joshua Marantz <[email protected]>

Kick CI

1488d58

Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'master' into inject-time-source

44d880d

Signed-off-by: Joshua Marantz <[email protected]>

Address review comments.

416917f

Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'inject-time-source' into limit-prod-timesource

c33f66c

Signed-off-by: Joshua Marantz <[email protected]>

Rename 'TestTime' to 'RealTestTime' and add TODO to change all tests …

6d9fac6

…to use 'SimulatedTestTime' which hasn't been built yet. Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'inject-time-source' into limit-prod-timesource

8855c5a

Signed-off-by: Joshua Marantz <[email protected]>

half-baked TimerFactory concept

c717a28

Signed-off-by: Joshua Marantz <[email protected]>

Don't copy TimeSource objects; it's easier to make them pure virtual …

944d75b

…if they are not copyable. Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'inject-time-source' into virtual-time-source

65d756b

Signed-off-by: Joshua Marantz <[email protected]>

Makes TimeSource virtual, removing SystemTimeSource and MonotonicTime…

970c28f

…Source as distinct class hierarchies. Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'master' into mock-time-for-gzip-test

6fcfe70

Signed-off-by: Joshua Marantz <[email protected]>

jmarantz changed the title ~~WIP: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers~~ time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers Aug 29, 2018

jmarantz added 2 commits August 29, 2018 14:50

check_format update to incorporate RealTimeSystem.

85b28bf

Signed-off-by: Joshua Marantz <[email protected]>

Merge branch 'master' into mock-time-for-gzip-test

f11342d

Signed-off-by: Joshua Marantz <[email protected]>

dnoe self-assigned this Aug 29, 2018

jmarantz added 2 commits August 30, 2018 14:40

Merge branch 'master' into mock-time-for-gzip-test

89e677a

Signed-off-by: Joshua Marantz <[email protected]>

rename TimerFactory to Scheduler, and changes for related spellings.

5ecf9a4

Signed-off-by: Joshua Marantz <[email protected]>

dnoe suggested changes Sep 2, 2018

View reviewed changes

jmarantz added 6 commits September 2, 2018 08:30

Merge branch 'master' into mock-time-for-gzip-test, and address revie…

68dfdf4

…w comments. Signed-off-by: Joshua Marantz <[email protected]>

Fix comment.

c060eb9

Signed-off-by: Joshua Marantz <[email protected]>

Restore timer_impl.h and .cc as files to enable easier re-use later i…

c5e33c2

…n SimulatedTimeSystem. Signed-off-by: Joshua Marantz <[email protected]>

Kick CI

54bbe09

Signed-off-by: Joshua Marantz <[email protected]>

add dispatcher to createScheduler interface, which makes it easier to…

3fe902f

… use thread-safe post() to wake up the correct threads. Signed-off-by: Joshua Marantz <[email protected]>

Revert "add dispatcher to createScheduler interface, which makes it e…

b5d64e4

…asier to use thread-safe post() to wake up the correct threads." This reverts commit 3fe902f. Signed-off-by: Joshua Marantz <[email protected]>

dnoe reviewed Sep 4, 2018

View reviewed changes

jmarantz mentioned this pull request Sep 4, 2018

clean up management of durations in timer_impl #4332

Closed

Add TODO to clean up use of duration_cast.

7f3c042

Signed-off-by: Joshua Marantz <[email protected]>

dnoe assigned htuch Sep 4, 2018

dnoe approved these changes Sep 4, 2018

View reviewed changes

htuch approved these changes Sep 5, 2018

View reviewed changes

htuch merged commit c6bfc7d into envoyproxy:master Sep 5, 2018

jmarantz deleted the mock-time-for-gzip-test branch September 5, 2018 00:08

htuch mentioned this pull request Sep 5, 2018

fix time_since_epoch different in different os default return precision #4288

Merged

jmarantz mentioned this pull request Sep 5, 2018

time: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate #4343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers #4257

time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers #4257

jmarantz commented Aug 26, 2018 •

edited

Loading

dnoe commented Aug 29, 2018

jmarantz commented Aug 29, 2018

dnoe left a comment

dnoe Aug 30, 2018

jmarantz Sep 2, 2018

dnoe Sep 2, 2018

jmarantz Sep 2, 2018

dnoe Sep 2, 2018

jmarantz Sep 2, 2018

jmarantz Sep 3, 2018

dnoe Sep 2, 2018

jmarantz Sep 2, 2018

dnoe Sep 2, 2018

jmarantz Sep 2, 2018

dnoe Sep 2, 2018

jmarantz Sep 2, 2018

dnoe Sep 2, 2018

jmarantz Sep 2, 2018

dnoe Sep 2, 2018

jmarantz Sep 2, 2018

dnoe left a comment

dnoe Sep 4, 2018

jmarantz commented Sep 4, 2018

dnoe commented Sep 4, 2018

htuch left a comment

		@@ -77,8 +79,8 @@ def whitelistedForProtobufDeps(file_path):
		# Production time sources should not be instantiated in the source, except for a few

time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers #4257

time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers #4257

Conversation

jmarantz commented Aug 26, 2018 • edited Loading

dnoe commented Aug 29, 2018

jmarantz commented Aug 29, 2018

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Sep 4, 2018

dnoe commented Sep 4, 2018

htuch left a comment

Choose a reason for hiding this comment

jmarantz commented Aug 26, 2018 •

edited

Loading