Remove the dependency from TimeSystem to libevent by using the Event:…

…:Scheduler abstraction as a delegate. (#6240)

Description: SimulatedTimeSystem and TimeSystem should not directly depend on libevent; they just need to be able to schedule events. This PR adds an abstraction boundary using the existing Scheduler interface.
Risk Level: low -- just a refactor
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Fixes #Issue: #5820

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

include/envoy/event/BUILD

@@ -48,6 +48,5 @@ envoy_cc_library(
     hdrs = ["timer.h"],
     deps = [
         "//include/envoy/common:time_interface",
-        "//source/common/event:libevent_lib",
     ],
 )

include/envoy/event/timer.h

@@ -7,8 +7,6 @@
 #include "envoy/common/pure.h"
 #include "envoy/common/time.h"
 
-#include "common/event/libevent.h"
-
 namespace Envoy {
 namespace Event {
 
@@ -63,7 +61,7 @@ class TimeSystem : public TimeSource {
    * 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;
+  virtual SchedulerPtr createScheduler(Scheduler& base_scheduler) PURE;
 };
 
 } // namespace Event

source/common/event/BUILD

@@ -20,6 +20,7 @@ envoy_cc_library(
     ],
     deps = [
         ":dispatcher_includes",
+        ":libevent_scheduler_lib",
         ":real_time_system_lib",
         "//include/envoy/common:time_interface",
         "//include/envoy/event:signal_interface",
@@ -35,19 +36,21 @@ envoy_cc_library(
 )
 
 envoy_cc_library(
-    name = "real_time_system_lib",
-    srcs = [
-        "event_impl_base.cc",
-        "real_time_system.cc",
-        "timer_impl.cc",
-    ],
-    hdrs = [
-        "event_impl_base.h",
-        "real_time_system.h",
-        "timer_impl.h",
+    name = "event_impl_base_lib",
+    srcs = ["event_impl_base.cc"],
+    hdrs = ["event_impl_base.h"],
+    external_deps = [
+        "event",
     ],
+)
+
+envoy_cc_library(
+    name = "real_time_system_lib",
+    srcs = ["real_time_system.cc"],
+    hdrs = ["real_time_system.h"],
     deps = [
-        ":libevent_lib",
+        ":event_impl_base_lib",
+        ":timer_lib",
         "//include/envoy/event:timer_interface",
         "//source/common/common:utility_lib",
         "//source/common/event:dispatcher_includes",
@@ -63,6 +66,7 @@ envoy_cc_library(
     ],
     deps = [
         ":libevent_lib",
+        ":libevent_scheduler_lib",
         "//include/envoy/api:api_interface",
         "//include/envoy/event:deferred_deletable",
         "//include/envoy/event:dispatcher_interface",
@@ -86,6 +90,31 @@ envoy_cc_library(
     ],
 )
 
+envoy_cc_library(
+    name = "libevent_scheduler_lib",
+    srcs = ["libevent_scheduler.cc"],
+    hdrs = ["libevent_scheduler.h"],
+    external_deps = ["event"],
+    deps = [
+        ":libevent_lib",
+        ":timer_lib",
+        "//include/envoy/event:timer_interface",
+        "//source/common/common:assert_lib",
+    ],
+)
+
+envoy_cc_library(
+    name = "timer_lib",
+    srcs = ["timer_impl.cc"],
+    hdrs = ["timer_impl.h"],
+    external_deps = ["event"],
+    deps = [
+        ":event_impl_base_lib",
+        ":libevent_lib",
+        "//include/envoy/event:timer_interface",
+    ],
+)
+
 envoy_cc_library(
     name = "dispatched_thread_lib",
     srcs = ["dispatched_thread.cc"],

source/common/event/dispatcher_impl.cc

@@ -14,7 +14,9 @@
 #include "common/common/lock_guard.h"
 #include "common/common/thread.h"
 #include "common/event/file_event_impl.h"
+#include "common/event/libevent_scheduler.h"
 #include "common/event/signal_impl.h"
+#include "common/event/timer_impl.h"
 #include "common/filesystem/watcher_impl.h"
 #include "common/network/connection_impl.h"
 #include "common/network/dns_impl.h"
@@ -27,20 +29,15 @@ namespace Envoy {
 namespace Event {
 
 DispatcherImpl::DispatcherImpl(Api::Api& api, Event::TimeSystem& time_system)
-    : DispatcherImpl(std::make_unique<Buffer::WatermarkBufferFactory>(), api, time_system) {
-  // The dispatcher won't work as expected if libevent hasn't been configured to use threads.
-  RELEASE_ASSERT(Libevent::Global::initialized(), "");
-}
+    : DispatcherImpl(std::make_unique<Buffer::WatermarkBufferFactory>(), api, time_system) {}
 
 DispatcherImpl::DispatcherImpl(Buffer::WatermarkFactoryPtr&& factory, Api::Api& api,
                                Event::TimeSystem& time_system)
-    : api_(api), buffer_factory_(std::move(factory)), base_(event_base_new()),
-      scheduler_(time_system.createScheduler(base_)),
+    : api_(api), buffer_factory_(std::move(factory)),
+      scheduler_(time_system.createScheduler(base_scheduler_)),
       deferred_delete_timer_(createTimer([this]() -> void { clearDeferredDeleteList(); })),
       post_timer_(createTimer([this]() -> void { runPostCallbacks(); })),
-      current_to_delete_(&to_delete_1_) {
-  RELEASE_ASSERT(Libevent::Global::initialized(), "");
-}
+      current_to_delete_(&to_delete_1_) {}
 
 DispatcherImpl::~DispatcherImpl() {}
 
@@ -139,7 +136,7 @@ void DispatcherImpl::deferredDelete(DeferredDeletablePtr&& to_delete) {
   }
 }
 
-void DispatcherImpl::exit() { event_base_loopexit(base_.get(), nullptr); }
+void DispatcherImpl::exit() { base_scheduler_.loopExit(); }
 
 SignalEventPtr DispatcherImpl::listenForSignal(int signal_num, SignalCb cb) {
   ASSERT(isThreadSafe());
@@ -168,7 +165,11 @@ void DispatcherImpl::run(RunType type) {
   // event_base_once() before some other event, the other event might get called first.
   runPostCallbacks();
 
-  event_base_loop(base_.get(), type == RunType::NonBlock ? EVLOOP_NONBLOCK : 0);
+  if (type == RunType::NonBlock) {
+    base_scheduler_.nonBlockingLoop();
+  } else {
+    base_scheduler_.blockingLoop();
+  }
 }
 
 void DispatcherImpl::runPostCallbacks() {

source/common/event/dispatcher_impl.h

@@ -14,6 +14,7 @@
 #include "common/common/logger.h"
 #include "common/common/thread.h"
 #include "common/event/libevent.h"
+#include "common/event/libevent_scheduler.h"
 
 namespace Envoy {
 namespace Event {
@@ -31,7 +32,7 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>, public Dispatcher {
   /**
    * @return event_base& the libevent base.
    */
-  event_base& base() { return *base_; }
+  event_base& base() { return base_scheduler_.base(); }
 
   // Event::Dispatcher
   TimeSource& timeSource() override { return api_.timeSource(); }
@@ -73,7 +74,7 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>, public Dispatcher {
   Api::Api& api_;
   Thread::ThreadIdPtr run_tid_;
   Buffer::WatermarkFactoryPtr buffer_factory_;
-  Libevent::BasePtr base_;
+  LibeventScheduler base_scheduler_;
   SchedulerPtr scheduler_;
   TimerPtr deferred_delete_timer_;
   TimerPtr post_timer_;

source/common/event/libevent_scheduler.cc

@@ -0,0 +1,25 @@
+#include "common/event/libevent_scheduler.h"
+
+#include "common/common/assert.h"
+#include "common/event/timer_impl.h"
+
+namespace Envoy {
+namespace Event {
+
+LibeventScheduler::LibeventScheduler() : libevent_(event_base_new()) {
+  // The dispatcher won't work as expected if libevent hasn't been configured to use threads.
+  RELEASE_ASSERT(Libevent::Global::initialized(), "");
+}
+
+TimerPtr LibeventScheduler::createTimer(const TimerCb& cb) {
+  return std::make_unique<TimerImpl>(libevent_, cb);
+};
+
+void LibeventScheduler::nonBlockingLoop() { event_base_loop(libevent_.get(), EVLOOP_NONBLOCK); }
+
+void LibeventScheduler::blockingLoop() { event_base_loop(libevent_.get(), 0); }
+
+void LibeventScheduler::loopExit() { event_base_loopexit(libevent_.get(), nullptr); }
+
+} // namespace Event
+} // namespace Envoy

source/common/event/libevent_scheduler.h

@@ -0,0 +1,50 @@
+#pragma once
+
+#include "envoy/event/timer.h"
+
+#include "common/event/libevent.h"
+
+#include "event2/event.h"
+
+namespace Envoy {
+namespace Event {
+
+// Implements Scheduler based on libevent.
+class LibeventScheduler : public Scheduler {
+public:
+  LibeventScheduler();
+
+  // Scheduler
+  TimerPtr createTimer(const TimerCb& cb) override;
+
+  /**
+   * Runs the libevent loop once, without blocking.
+   */
+  void nonBlockingLoop();
+
+  /**
+   * Runs the libevent loop once, with block.
+   */
+  void blockingLoop();
+
+  /**
+   * Exits the libevent loop.
+   */
+  void loopExit();
+
+  /**
+   * TODO(jmarantz): consider strengthening this abstraction and instead of
+   * exposing the libevent base pointer, provide API abstractions for the calls
+   * into it. Among other benefits this might make it more tractable to someday
+   * consider an alternative to libevent if the need arises.
+   *
+   * @return the underlying libevent structure.
+   */
+  event_base& base() { return *libevent_; }
+
+private:
+  Libevent::BasePtr libevent_;
+};
+
+} // namespace Event
+} // namespace Envoy

source/common/event/real_time_system.cc

@@ -3,30 +3,25 @@
 #include <chrono>
 
 #include "common/common/assert.h"
-#include "common/event/event_impl_base.h"
 #include "common/event/timer_impl.h"
 
-#include "event2/event.h"
-
 namespace Envoy {
 namespace Event {
 namespace {
 
 class RealScheduler : public Scheduler {
 public:
-  RealScheduler(Libevent::BasePtr& libevent) : libevent_(libevent) {}
-  TimerPtr createTimer(const TimerCb& cb) override {
-    return std::make_unique<TimerImpl>(libevent_, cb);
-  };
+  RealScheduler(Scheduler& base_scheduler) : base_scheduler_(base_scheduler) {}
+  TimerPtr createTimer(const TimerCb& cb) override { return base_scheduler_.createTimer(cb); };
 
 private:
-  Libevent::BasePtr& libevent_;
+  Scheduler& base_scheduler_;
 };
 
 } // namespace
 
-SchedulerPtr RealTimeSystem::createScheduler(Libevent::BasePtr& libevent) {
-  return std::make_unique<RealScheduler>(libevent);
+SchedulerPtr RealTimeSystem::createScheduler(Scheduler& base_scheduler) {
+  return std::make_unique<RealScheduler>(base_scheduler);
 }
 
 } // namespace Event

source/common/event/real_time_system.h

@@ -13,7 +13,7 @@ namespace Event {
 class RealTimeSystem : public TimeSystem {
 public:
   // TimeSystem
-  SchedulerPtr createScheduler(Libevent::BasePtr&) override;
+  SchedulerPtr createScheduler(Scheduler&) override;
 
   // TimeSource
   SystemTime systemTime() override { return time_source_.systemTime(); }

source/common/event/timer_impl.h

@@ -5,6 +5,7 @@
 #include "envoy/event/timer.h"
 
 #include "common/event/event_impl_base.h"
+#include "common/event/libevent.h"
 
 namespace Envoy {
 namespace Event {

test/mocks/common.h

@@ -49,8 +49,8 @@ class MockTimeSystem : public Event::TestTimeSystem {
   // where timer callbacks are triggered by the advancement of time. This implementation
   // matches recent behavior, where real-time timers were created directly in libevent
   // by dispatcher_impl.cc.
-  Event::SchedulerPtr createScheduler(Event::Libevent::BasePtr& base) override {
-    return real_time_.createScheduler(base);
+  Event::SchedulerPtr createScheduler(Event::Scheduler& base_scheduler) override {
+    return real_time_.createScheduler(base_scheduler);
   }
   void sleep(const Duration& duration) override { real_time_.sleep(duration); }
   Thread::CondVar::WaitStatus

test/test_common/BUILD

@@ -196,6 +196,7 @@ envoy_cc_test(
     deps = [
         ":simulated_time_system_lib",
         ":utility_lib",
+        "//source/common/event:libevent_scheduler_lib",
     ],
 )