Injectable singleton class (#1974)

Description: -Adds a ThreadSafe singleton class which can be accessed from all threads and overridden with an injected instance for custom implementations for test code or custom Envoy builds. -Replaces OsSysCallsImpl being plumbed everywhere it is needed with a new ThreadSafe singleton as a sample use case (and to avoid having to plumb the OsSysCallsImpl to all users for test overrides) -Adds abseil as a dependency Fixes #1808 Step 1 of #1754 Risk Level: Low: mainly a refactor to how OsSysCallsImpl is accessed Testing: Existing OS tests continue to pass, now using an inject-able mock singleton test/common/common/singleton_test.cc unit testing the new class Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Nov 20, 2017 · cbff0f7 · cbff0f7
1 parent 29f62bf
commit cbff0f7
Show file tree

Hide file tree

Showing 43 changed files with 273 additions and 67 deletions.
diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl
@@ -232,6 +232,7 @@ def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
     # The long repo names (`com_github_fmtlib_fmt` instead of `fmtlib`) are
     # semi-standard in the Bazel community, intended to avoid both duplicate
     # dependencies and name conflicts.
+    _com_google_absl()
     _com_github_bombela_backward()
     _com_github_cyan4973_xxhash()
     _com_github_eile_tclap()
@@ -358,6 +359,13 @@ def _com_google_googletest():
         actual = "@com_google_googletest//:gtest",
     )
 
+def _com_google_absl():
+    _repository_impl("com_google_absl")
+    native.bind(
+        name = "abseil_base",
+        actual = "@com_google_absl//absl/base:base",
+    )
+
 def _com_google_protobuf():
     _repository_impl("com_google_protobuf")
 

diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl
@@ -1,4 +1,8 @@
 REPOSITORY_LOCATIONS = dict(
+    com_google_absl = dict(
+        commit = "6de53819a7173bd446156237a37f53464b7732cc",
+        remote = "https://github.com/abseil/abseil-cpp",
+    ),
     com_github_bombela_backward = dict(
         commit = "cd1c4bd9e48afe812a0e996d335298c455afcd92",  # v1.3
         remote = "https://github.com/bombela/backward-cpp",

diff --git a/source/common/api/BUILD b/source/common/api/BUILD
@@ -26,5 +26,6 @@ envoy_cc_library(
     hdrs = ["os_sys_calls_impl.h"],
     deps = [
         "//include/envoy/api:os_sys_calls_interface",
+        "//source/common/singleton:threadsafe_singleton",
     ],
 )
diff --git a/source/common/api/api_impl.cc b/source/common/api/api_impl.cc
@@ -3,7 +3,6 @@
 #include <chrono>
 #include <string>
 
-#include "common/api/os_sys_calls_impl.h"
 #include "common/event/dispatcher_impl.h"
 #include "common/filesystem/filesystem_impl.h"
 
@@ -15,11 +14,11 @@ Event::DispatcherPtr Impl::allocateDispatcher() {
 }
 
 Impl::Impl(std::chrono::milliseconds file_flush_interval_msec)
-    : os_sys_calls_(new OsSysCallsImpl()), file_flush_interval_msec_(file_flush_interval_msec) {}
+    : file_flush_interval_msec_(file_flush_interval_msec) {}
 
 Filesystem::FileSharedPtr Impl::createFile(const std::string& path, Event::Dispatcher& dispatcher,
                                            Thread::BasicLockable& lock, Stats::Store& stats_store) {
-  return std::make_shared<Filesystem::FileImpl>(path, dispatcher, lock, *os_sys_calls_, stats_store,
+  return std::make_shared<Filesystem::FileImpl>(path, dispatcher, lock, stats_store,
                                                 file_flush_interval_msec_);
 }
 

diff --git a/source/common/api/api_impl.h b/source/common/api/api_impl.h
@@ -4,7 +4,6 @@
 #include <string>
 
 #include "envoy/api/api.h"
-#include "envoy/api/os_sys_calls.h"
 #include "envoy/filesystem/filesystem.h"
 
 namespace Envoy {
@@ -26,7 +25,6 @@ class Impl : public Api::Api {
   std::string fileReadToEnd(const std::string& path) override;
 
 private:
-  OsSysCallsPtr os_sys_calls_;
   std::chrono::milliseconds file_flush_interval_msec_;
 };
 

diff --git a/source/common/api/os_sys_calls_impl.h b/source/common/api/os_sys_calls_impl.h
@@ -2,6 +2,8 @@
 
 #include "envoy/api/os_sys_calls.h"
 
+#include "common/singleton/threadsafe_singleton.h"
+
 namespace Envoy {
 namespace Api {
 
@@ -19,5 +21,7 @@ class OsSysCallsImpl : public OsSysCalls {
   int stat(const char* pathname, struct stat* buf) override;
 };
 
+typedef ThreadSafeSingleton<OsSysCallsImpl> OsSysCallsSingleton;
+
 } // namespace Api
 } // namespace Envoy
diff --git a/source/common/common/BUILD b/source/common/common/BUILD
@@ -97,11 +97,6 @@ envoy_cc_library(
     hdrs = ["non_copyable.h"],
 )
 
-envoy_cc_library(
-    name = "singleton",
-    hdrs = ["singleton.h"],
-)
-
 envoy_cc_library(
     name = "stl_helpers",
     hdrs = ["stl_helpers.h"],

diff --git a/source/common/config/BUILD b/source/common/config/BUILD
@@ -208,8 +208,8 @@ envoy_cc_library(
     hdrs = ["metadata.h"],
     external_deps = ["envoy_base"],
     deps = [
-        "//source/common/common:singleton",
         "//source/common/protobuf",
+        "//source/common/singleton:const_singleton",
     ],
 )
 
@@ -227,7 +227,7 @@ envoy_cc_library(
 envoy_cc_library(
     name = "resources_lib",
     hdrs = ["resources.h"],
-    deps = ["//source/common/common:singleton"],
+    deps = ["//source/common/singleton:const_singleton"],
 )
 
 envoy_cc_library(
@@ -298,11 +298,11 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/common:hash_lib",
         "//source/common/common:hex_lib",
-        "//source/common/common:singleton",
         "//source/common/grpc:common_lib",
         "//source/common/json:config_schemas_lib",
         "//source/common/protobuf",
         "//source/common/protobuf:utility_lib",
+        "//source/common/singleton:const_singleton",
         "//source/common/stats:stats_lib",
     ],
 )
@@ -311,5 +311,5 @@ envoy_cc_library(
     name = "well_known_names",
     srcs = ["well_known_names.cc"],
     hdrs = ["well_known_names.h"],
-    deps = ["//source/common/common:singleton"],
+    deps = ["//source/common/singleton:const_singleton"],
 )
diff --git a/source/common/config/metadata.h b/source/common/config/metadata.h
@@ -2,8 +2,8 @@
 
 #include <string>
 
-#include "common/common/singleton.h"
 #include "common/protobuf/protobuf.h"
+#include "common/singleton/const_singleton.h"
 
 #include "api/base.pb.h"
 

diff --git a/source/common/config/resources.h b/source/common/config/resources.h
@@ -2,7 +2,7 @@
 
 #include <string>
 
-#include "common/common/singleton.h"
+#include "common/singleton/const_singleton.h"
 
 namespace Envoy {
 namespace Config {

diff --git a/source/common/config/utility.h b/source/common/config/utility.h
@@ -11,10 +11,10 @@
 #include "common/common/assert.h"
 #include "common/common/hash.h"
 #include "common/common/hex.h"
-#include "common/common/singleton.h"
 #include "common/grpc/common.h"
 #include "common/protobuf/protobuf.h"
 #include "common/protobuf/utility.h"
+#include "common/singleton/const_singleton.h"
 
 #include "api/base.pb.h"
 #include "api/cds.pb.h"

diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h
@@ -6,7 +6,7 @@
 
 #include "envoy/common/exception.h"
 
-#include "common/common/singleton.h"
+#include "common/singleton/const_singleton.h"
 
 #include "fmt/format.h"
 

diff --git a/source/common/filesystem/BUILD b/source/common/filesystem/BUILD
@@ -16,6 +16,7 @@ envoy_cc_library(
         "//include/envoy/api:os_sys_calls_interface",
         "//include/envoy/event:dispatcher_interface",
         "//include/envoy/filesystem:filesystem_interface",
+        "//source/common/api:os_sys_calls_lib",
         "//source/common/buffer:buffer_lib",
         "//source/common/common:thread_lib",
     ],

diff --git a/source/common/filesystem/filesystem_impl.cc b/source/common/filesystem/filesystem_impl.cc
@@ -15,6 +15,7 @@
 #include "envoy/event/timer.h"
 #include "envoy/stats/stats.h"
 
+#include "common/api/os_sys_calls_impl.h"
 #include "common/common/assert.h"
 #include "common/common/thread.h"
 
@@ -53,14 +54,14 @@ std::string fileReadToEnd(const std::string& path) {
 }
 
 FileImpl::FileImpl(const std::string& path, Event::Dispatcher& dispatcher,
-                   Thread::BasicLockable& lock, Api::OsSysCalls& os_sys_calls,
-                   Stats::Store& stats_store, std::chrono::milliseconds flush_interval_msec)
+                   Thread::BasicLockable& lock, Stats::Store& stats_store,
+                   std::chrono::milliseconds flush_interval_msec)
     : path_(path), file_lock_(lock), flush_timer_(dispatcher.createTimer([this]() -> void {
         stats_.flushed_by_timer_.inc();
         flush_event_.notify_one();
         flush_timer_->enableTimer(flush_interval_msec_);
       })),
-      os_sys_calls_(os_sys_calls), flush_interval_msec_(flush_interval_msec),
+      os_sys_calls_(Api::OsSysCallsSingleton::get()), flush_interval_msec_(flush_interval_msec),
       stats_{FILESYSTEM_STATS(POOL_COUNTER_PREFIX(stats_store, "filesystem."),
                               POOL_GAUGE_PREFIX(stats_store, "filesystem."))} {
   open();

diff --git a/source/common/filesystem/filesystem_impl.h b/source/common/filesystem/filesystem_impl.h
@@ -57,8 +57,7 @@ std::string fileReadToEnd(const std::string& path);
 class FileImpl : public File {
 public:
   FileImpl(const std::string& path, Event::Dispatcher& dispatcher, Thread::BasicLockable& lock,
-           Api::OsSysCalls& osSysCalls, Stats::Store& stats_store,
-           std::chrono::milliseconds flush_interval_msec);
+           Stats::Store& stats_store, std::chrono::milliseconds flush_interval_msec);
   ~FileImpl();
 
   // Filesystem::File

diff --git a/source/common/http/BUILD b/source/common/http/BUILD
@@ -166,8 +166,8 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/common:empty_string",
         "//source/common/common:non_copyable",
-        "//source/common/common:singleton",
         "//source/common/common:utility_lib",
+        "//source/common/singleton:const_singleton",
     ],
 )
 
@@ -176,7 +176,7 @@ envoy_cc_library(
     hdrs = ["headers.h"],
     deps = [
         "//include/envoy/http:header_map_interface",
-        "//source/common/common:singleton",
+        "//source/common/singleton:const_singleton",
     ],
 )
 

diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc
@@ -6,8 +6,8 @@
 
 #include "common/common/assert.h"
 #include "common/common/empty_string.h"
-#include "common/common/singleton.h"
 #include "common/common/utility.h"
+#include "common/singleton/const_singleton.h"
 
 namespace Envoy {
 namespace Http {

diff --git a/source/common/http/headers.h b/source/common/http/headers.h
@@ -4,7 +4,7 @@
 
 #include "envoy/http/header_map.h"
 
-#include "common/common/singleton.h"
+#include "common/singleton/const_singleton.h"
 
 namespace Envoy {
 namespace Http {

diff --git a/source/common/mongo/BUILD b/source/common/mongo/BUILD
@@ -57,11 +57,11 @@ envoy_cc_library(
         "//source/common/buffer:buffer_lib",
         "//source/common/common:assert_lib",
         "//source/common/common:logger_lib",
-        "//source/common/common:singleton",
         "//source/common/common:utility_lib",
         "//source/common/config:filter_json_lib",
         "//source/common/network:filter_lib",
         "//source/common/protobuf:utility_lib",
+        "//source/common/singleton:const_singleton",
     ],
 )
 

diff --git a/source/common/mongo/proxy.h b/source/common/mongo/proxy.h
@@ -19,10 +19,10 @@
 
 #include "common/buffer/buffer_impl.h"
 #include "common/common/logger.h"
-#include "common/common/singleton.h"
 #include "common/mongo/utility.h"
 #include "common/network/filter_impl.h"
 #include "common/protobuf/utility.h"
+#include "common/singleton/const_singleton.h"
 
 #include "api/filter/network/mongo_proxy.pb.h"
 

diff --git a/source/common/ratelimit/ratelimit_impl.h b/source/common/ratelimit/ratelimit_impl.h
@@ -10,7 +10,7 @@
 #include "envoy/tracing/http_tracer.h"
 #include "envoy/upstream/cluster_manager.h"
 
-#include "common/common/singleton.h"
+#include "common/singleton/const_singleton.h"
 
 #include "source/common/ratelimit/ratelimit.pb.h"
 

diff --git a/source/common/singleton/BUILD b/source/common/singleton/BUILD
@@ -8,6 +8,11 @@ load(
 
 envoy_package()
 
+envoy_cc_library(
+    name = "const_singleton",
+    hdrs = ["const_singleton.h"],
+)
+
 envoy_cc_library(
     name = "manager_impl_lib",
     srcs = ["manager_impl.cc"],
@@ -19,3 +24,9 @@ envoy_cc_library(
         "//source/common/common:thread_lib",
     ],
 )
+
+envoy_cc_library(
+    name = "threadsafe_singleton",
+    hdrs = ["threadsafe_singleton.h"],
+    external_deps = ["abseil_base"],
+)
diff --git a/source/common/common/singleton.h → source/common/singleton/const_singleton.h b/source/common/common/singleton.h → source/common/singleton/const_singleton.h
@@ -3,7 +3,10 @@
 namespace Envoy {
 
 /**
- * Immutable singleton pattern. See singleton/manager.h for mutable/destroyable singletons.
+ * ConstSingleton allows easy global cross-thread access to a const object.
+ *
+ * This singleton should be used for data which is initialized once at
+ * start-up and then be treated as immutable const data thereafter.
  */
 template <class T> class ConstSingleton {
 public:

diff --git a/source/common/singleton/threadsafe_singleton.h b/source/common/singleton/threadsafe_singleton.h
@@ -0,0 +1,44 @@
+#pragma once
+
+#include "absl/base/call_once.h"
+
+namespace Envoy {
+
+/**
+ * ThreadSafeSingleton allows easy global cross-thread access to a non-const object.
+ *
+ * This singleton class should be used for singletons which must be globally
+ * accessible but can not be marked as const. All functions in the singleton class
+ * *must* be threadsafe.
+ *
+ * Note that there is heavy resistence in Envoy to adding this type of singleton
+ * if data will persist with state changes across tests, as it becomes difficult
+ * to write clean unit tests if a state change in one test will persist into
+ * another test. Be wary of using it. A example of acceptable usage is OsSyscallsImpl,
+ * where the functions are not strictly speaking const, but affect the OS rather than the
+ * class itself. An example of unacceptable usage upstream would be for
+ * globally accessible stat counters, it would have the aforementioned problem
+ * where state "leaks" across tests.
+ *
+ * */
+template <class T> class ThreadSafeSingleton {
+public:
+  static T& get() {
+    absl::call_once(ThreadSafeSingleton<T>::create_once_, &ThreadSafeSingleton<T>::Create);
+    return *ThreadSafeSingleton<T>::instance_;
+  }
+
+protected:
+  template <typename A> friend class TestThreadsafeSingletonInjector;
+
+  static void Create() { instance_ = new T(); }
+
+  static absl::once_flag create_once_;
+  static T* instance_;
+};
+
+template <class T> absl::once_flag ThreadSafeSingleton<T>::create_once_;
+
+template <class T> T* ThreadSafeSingleton<T>::instance_ = nullptr;
+
+} // namespace Envoy
diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD
@@ -18,10 +18,10 @@ envoy_cc_library(
         "//include/envoy/server:options_interface",
         "//include/envoy/stats:stats_interface",
         "//source/common/common:assert_lib",
-        "//source/common/common:singleton",
         "//source/common/common:utility_lib",
         "//source/common/config:well_known_names",
         "//source/common/protobuf",
+        "//source/common/singleton:const_singleton",
     ],
 )
 

diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h
@@ -15,7 +15,6 @@
 #include "envoy/stats/stats.h"
 
 #include "common/common/assert.h"
-#include "common/common/singleton.h"
 #include "common/protobuf/protobuf.h"
 
 #include "api/bootstrap.pb.h"