time: Event::TimeSystem abstraction to make it feasible to inject tim…

…e with simulated timers (#4257)

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 <jmarantz@google.com>

include/envoy/api/api.h

@@ -5,6 +5,7 @@
 
 #include "envoy/common/time.h"
 #include "envoy/event/dispatcher.h"
+#include "envoy/event/timer.h"
 #include "envoy/filesystem/filesystem.h"
 #include "envoy/stats/store.h"
 #include "envoy/thread/thread.h"
@@ -24,7 +25,7 @@ class Api {
    * @param time_source the time source.
    * @return Event::DispatcherPtr which is owned by the caller.
    */
-  virtual Event::DispatcherPtr allocateDispatcher(TimeSource& time_source) PURE;
+  virtual Event::DispatcherPtr allocateDispatcher(Event::TimeSystem& time_system) PURE;
 
   /**
    * Create/open a local file that supports async appending.

include/envoy/common/time.h

@@ -5,6 +5,7 @@
 #include "envoy/common/pure.h"
 
 namespace Envoy {
+
 /**
  * Less typing for common system time and steady time type.
  *
@@ -14,76 +15,22 @@ namespace Envoy {
 typedef std::chrono::time_point<std::chrono::system_clock> SystemTime;
 typedef std::chrono::time_point<std::chrono::steady_clock> MonotonicTime;
 
-/**
- * Abstraction for getting the current system time. Useful for testing.
- *
- * TODO(#4160): eliminate this class and pass TimeSource everywhere.
- */
-class SystemTimeSource {
-public:
-  virtual ~SystemTimeSource() {}
-
-  /**
-   * @return the current system time.
-   */
-  virtual SystemTime currentTime() PURE;
-};
-
-/**
- * Abstraction for getting the current monotonically increasing time. Useful for
- * testing.
- *
- * TODO(#4160): eliminate this class and pass TimeSource everywhere.
- */
-class MonotonicTimeSource {
-public:
-  virtual ~MonotonicTimeSource() {}
-
-  /**
-   * @return the current monotonic time.
-   */
-  virtual MonotonicTime currentTime() PURE;
-};
-
 /**
  * Captures a system-time source, capable of computing both monotonically increasing
  * and real time.
- *
- * TODO(#4160): currently this is just a container for SystemTimeSource and
- * MonotonicTimeSource but we should clean that up and just have this as the
- * base class. Once that's done, TimeSource will be a pure interface.
  */
 class TimeSource {
 public:
-  TimeSource(SystemTimeSource& system, MonotonicTimeSource& monotonic)
-      : system_(system), monotonic_(monotonic) {}
+  virtual ~TimeSource() {}
 
   /**
    * @return the current system time; not guaranteed to be monotonically increasing.
    */
-  SystemTime systemTime() { return system_.currentTime(); }
-
+  virtual SystemTime systemTime() PURE;
   /**
    * @return the current monotonic time.
    */
-  MonotonicTime monotonicTime() { return monotonic_.currentTime(); }
-
-  /**
-   * Compares two time-sources for equality; this is needed for mocks.
-   */
-  bool operator==(const TimeSource& ts) const {
-    return &system_ == &ts.system_ && &monotonic_ == &ts.monotonic_;
-  }
-
-  // TODO(jmarantz): Eliminate these methods and the SystemTimeSource and MonotonicTimeSource
-  // classes, and change method calls to work directly off of TimeSource.
-  SystemTimeSource& system() { return system_; }
-  MonotonicTimeSource& monotonic() { return monotonic_; }
-
-private:
-  // These are pointers rather than references in order to support assignment.
-  SystemTimeSource& system_;
-  MonotonicTimeSource& monotonic_;
+  virtual MonotonicTime monotonicTime() PURE;
 };
 
 } // namespace Envoy

include/envoy/event/BUILD

@@ -20,7 +20,8 @@ envoy_cc_library(
         ":deferred_deletable",
         ":file_event_interface",
         ":signal_interface",
-        ":timer_interface",
+        "//include/envoy/common:time_interface",
+        "//include/envoy/event:timer_interface",
         "//include/envoy/filesystem:filesystem_interface",
         "//include/envoy/network:connection_handler_interface",
         "//include/envoy/network:connection_interface",
@@ -44,4 +45,7 @@ envoy_cc_library(
 envoy_cc_library(
     name = "timer_interface",
     hdrs = ["timer.h"],
+    deps = [
+        "//source/common/event:libevent_lib",
+    ],
 )

include/envoy/event/dispatcher.h

@@ -6,6 +6,7 @@
 #include <string>
 #include <vector>
 
+#include "envoy/common/time.h"
 #include "envoy/event/file_event.h"
 #include "envoy/event/signal.h"
 #include "envoy/event/timer.h"
@@ -35,12 +36,9 @@ class Dispatcher {
   /**
    * Returns a time-source to use with this dispatcher.
    *
-   * TODO(#4160) the implementations currently manage timer events that
-   * ignore the time-source, and thus can't be mocked or faked. So it's
-   * difficult to mock time in an integration test without mocking out
-   * the dispatcher.
+   * TODO(#4160) Rename to timeSystem().
    */
-  virtual TimeSource& timeSource() PURE;
+  virtual TimeSystem& timeSource() PURE;
 
   /**
    * Clear any items in the deferred deletion queue.
@@ -114,10 +112,10 @@ class Dispatcher {
                                               bool hand_off_restored_destination_connections) PURE;
 
   /**
-   * Allocate a timer. @see Event::Timer for docs on how to use the timer.
+   * Allocate a timer. @see Timer for docs on how to use the timer.
    * @param cb supplies the callback to invoke when the timer fires.
    */
-  virtual TimerPtr createTimer(TimerCb cb) PURE;
+  virtual Event::TimerPtr createTimer(TimerCb cb) PURE;
 
   /**
    * Submit an item for deferred delete. @see DeferredDeletable.

include/envoy/event/timer.h

@@ -5,10 +5,15 @@
 #include <memory>
 
 #include "envoy/common/pure.h"
+#include "envoy/common/time.h"
+
+#include "common/event/libevent.h"
 
 namespace Envoy {
 namespace Event {
 
+class TimerCB;
+
 /**
  * Callback invoked when a timer event fires.
  */
@@ -34,5 +39,32 @@ class Timer {
 
 typedef std::unique_ptr<Timer> TimerPtr;
 
+class Scheduler {
+public:
+  virtual ~Scheduler() {}
+
+  /**
+   * Creates a timer.
+   */
+  virtual TimerPtr createTimer(const TimerCb& cb) PURE;
+};
+
+typedef std::unique_ptr<Scheduler> SchedulerPtr;
+
+/**
+ * Interface providing a mechanism to measure time and set timers that run callbacks
+ * when the timer fires.
+ */
+class TimeSystem : public TimeSource {
+public:
+  virtual ~TimeSystem() {}
+
+  /**
+   * Creates a timer factory. This indirection enables thread-local timer-queue management,
+   * so servers can have a separate timer-factory in each thread.
+   */
+  virtual SchedulerPtr createScheduler(Libevent::BasePtr&) PURE;
+};
+
 } // namespace Event
 } // namespace Envoy

include/envoy/server/BUILD

@@ -93,6 +93,7 @@ envoy_cc_library(
         ":options_interface",
         "//include/envoy/access_log:access_log_interface",
         "//include/envoy/api:api_interface",
+        "//include/envoy/event:timer_interface",
         "//include/envoy/http:query_params_interface",
         "//include/envoy/init:init_interface",
         "//include/envoy/local_info:local_info_interface",

include/envoy/server/filter_config.h

@@ -130,13 +130,6 @@ class FactoryContext {
    */
   virtual const envoy::api::v2::core::Metadata& listenerMetadata() const PURE;
 
-  /**
-   * @return SystemTimeSource& a reference to the system time source.
-   * TODO(#4160): This method should be eliminated, and call-sites and mocks should
-   * be converted to work with timeSource() below.
-   */
-  virtual SystemTimeSource& systemTimeSource() PURE;
-
   /**
    * @return TimeSource& a reference to the time source.
    */

include/envoy/server/instance.h

@@ -6,6 +6,7 @@
 
 #include "envoy/access_log/access_log.h"
 #include "envoy/api/api.h"
+#include "envoy/event/timer.h"
 #include "envoy/init/init.h"
 #include "envoy/local_info/local_info.h"
 #include "envoy/network/listen_socket.h"
@@ -194,8 +195,9 @@ class Instance {
 
   /**
    * @return the time source used for the server.
+   * TODO(#4160): rename this to timeSystem().
    */
-  virtual TimeSource& timeSource() PURE;
+  virtual Event::TimeSystem& timeSource() PURE;
 
   /**
    * @return the flush interval of stats sinks.

source/common/api/api_impl.cc

@@ -9,8 +9,8 @@
 namespace Envoy {
 namespace Api {
 
-Event::DispatcherPtr Impl::allocateDispatcher(TimeSource& time_source) {
-  return Event::DispatcherPtr{new Event::DispatcherImpl(time_source)};
+Event::DispatcherPtr Impl::allocateDispatcher(Event::TimeSystem& time_system) {
+  return Event::DispatcherPtr{new Event::DispatcherImpl(time_system)};
 }
 
 Impl::Impl(std::chrono::milliseconds file_flush_interval_msec)

source/common/api/api_impl.h

@@ -4,6 +4,7 @@
 #include <string>
 
 #include "envoy/api/api.h"
+#include "envoy/event/timer.h"
 #include "envoy/filesystem/filesystem.h"
 
 namespace Envoy {
@@ -17,7 +18,7 @@ class Impl : public Api::Api {
   Impl(std::chrono::milliseconds file_flush_interval_msec);
 
   // Api::Api
-  Event::DispatcherPtr allocateDispatcher(TimeSource& time_source) override;
+  Event::DispatcherPtr allocateDispatcher(Event::TimeSystem& time_system) override;
   Filesystem::FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher,
                                        Thread::BasicLockable& lock,
                                        Stats::Store& stats_store) override;

source/common/common/perf_annotation.h

@@ -87,7 +87,7 @@ class PerfAnnotationContext {
               absl::string_view description);
 
   /** @return MonotonicTime the current time */
-  MonotonicTime currentTime() { return time_source_.currentTime(); }
+  MonotonicTime currentTime() { return time_source_.monotonicTime(); }
 
   /**
    * Renders the aggregated statistics as a string.
@@ -141,7 +141,7 @@ class PerfAnnotationContext {
 #else
   DurationStatsMap duration_stats_map_;
 #endif
-  ProdMonotonicTimeSource time_source_;
+  RealTimeSource time_source_;
 };
 
 /**

source/common/common/token_bucket_impl.cc

@@ -4,15 +4,13 @@
 
 namespace Envoy {
 
-TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, MonotonicTimeSource& monotonic_time_source,
-                                 double fill_rate)
+TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, TimeSource& time_source, double fill_rate)
     : max_tokens_(max_tokens), fill_rate_(std::abs(fill_rate)), tokens_(max_tokens),
-      last_fill_(monotonic_time_source.currentTime()),
-      monotonic_time_source_(monotonic_time_source) {}
+      last_fill_(time_source.monotonicTime()), time_source_(time_source) {}
 
 bool TokenBucketImpl::consume(uint64_t tokens) {
   if (tokens_ < max_tokens_) {
-    const auto time_now = monotonic_time_source_.currentTime();
+    const auto time_now = time_source_.monotonicTime();
     tokens_ = std::min((std::chrono::duration<double>(time_now - last_fill_).count() * fill_rate_) +
                            tokens_,
                        max_tokens_);

source/common/common/token_bucket_impl.h

@@ -18,8 +18,7 @@ class TokenBucketImpl : public TokenBucket {
    * @param fill_rate supplies the number of tokens that will return to the bucket on each second.
    * The default is 1.
    */
-  explicit TokenBucketImpl(uint64_t max_tokens, MonotonicTimeSource& time_source,
-                           double fill_rate = 1);
+  explicit TokenBucketImpl(uint64_t max_tokens, TimeSource& time_source, double fill_rate = 1);
 
   bool consume(uint64_t tokens = 1) override;
 
@@ -28,7 +27,7 @@ class TokenBucketImpl : public TokenBucket {
   const double fill_rate_;
   double tokens_;
   MonotonicTime last_fill_;
-  MonotonicTimeSource& monotonic_time_source_;
+  TimeSource& time_source_;
 };
 
 } // namespace Envoy

source/common/common/utility.h

@@ -96,21 +96,13 @@ class AccessLogDateTimeFormatter {
 };
 
 /**
- * Production implementation of SystemTimeSource that returns the current time.
+ * Real-world time implementation of TimeSource.
  */
-class ProdSystemTimeSource : public SystemTimeSource {
+class RealTimeSource : public TimeSource {
 public:
-  // SystemTimeSource
-  SystemTime currentTime() override { return std::chrono::system_clock::now(); }
-};
-
-/**
- * Production implementation of MonotonicTimeSource that returns the current time.
- */
-class ProdMonotonicTimeSource : public MonotonicTimeSource {
-public:
-  // MonotonicTimeSource
-  MonotonicTime currentTime() override { return std::chrono::steady_clock::now(); }
+  // TimeSource
+  SystemTime systemTime() override { return std::chrono::system_clock::now(); }
+  MonotonicTime monotonicTime() override { return std::chrono::steady_clock::now(); }
 };
 
 /**

source/common/config/grpc_mux_impl.cc

@@ -111,11 +111,9 @@ GrpcMuxWatchPtr GrpcMuxImpl::subscribe(const std::string& type_url,
   // TODO(gsagula): move TokenBucketImpl params to a config.
   if (!api_state_[type_url].subscribed_) {
     // Bucket contains 100 tokens maximum and refills at 5 tokens/sec.
-    api_state_[type_url].limit_request_ =
-        std::make_unique<TokenBucketImpl>(100, time_source_.monotonic(), 5);
+    api_state_[type_url].limit_request_ = std::make_unique<TokenBucketImpl>(100, time_source_, 5);
     // Bucket contains 1 token maximum and refills 1 token on every ~5 seconds.
-    api_state_[type_url].limit_log_ =
-        std::make_unique<TokenBucketImpl>(1, time_source_.monotonic(), 0.2);
+    api_state_[type_url].limit_log_ = std::make_unique<TokenBucketImpl>(1, time_source_, 0.2);
     api_state_[type_url].request_.set_type_url(type_url);
     api_state_[type_url].request_.mutable_node()->MergeFrom(local_info_.node());
     api_state_[type_url].subscribed_ = true;

source/common/config/grpc_mux_impl.h

@@ -103,7 +103,7 @@ class GrpcMuxImpl : public GrpcMux,
   std::list<std::string> subscriptions_;
   Event::TimerPtr retry_timer_;
   Runtime::RandomGenerator& random_;
-  TimeSource time_source_;
+  TimeSource& time_source_;
   BackOffStrategyPtr backoff_strategy_;
 };