diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 9bfdadad67ea..c2a9d3925434 100644 --- 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 index edcac34ac57d..efa570258910 100644 --- 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 index 3e54452e2511..31b3d4c61eeb 100644 --- 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 index 190eb723e5fe..86f418939e10 100644 --- a/source/common/api/api_impl.cc +++ b/source/common/api/api_impl.cc @@ -3,7 +3,6 @@ #include #include -#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(path, dispatcher, lock, *os_sys_calls_, stats_store, + return std::make_shared(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 index f0532bac9d81..b7f0f6fb609a 100644 --- a/source/common/api/api_impl.h +++ b/source/common/api/api_impl.h @@ -4,7 +4,6 @@ #include #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 index edbf658f5655..b6cdc5639428 100644 --- 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 OsSysCallsSingleton; + } // namespace Api } // namespace Envoy diff --git a/source/common/common/BUILD b/source/common/common/BUILD index a51a1e968595..e0b61b3758ed 100644 --- 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 index 8ecb8f40a765..48c24ca8b2ff 100644 --- 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 index 3eba46565251..b77ffb7f2cc7 100644 --- a/source/common/config/metadata.h +++ b/source/common/config/metadata.h @@ -2,8 +2,8 @@ #include -#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 index ae8fff8d1715..03f0c9a2efd8 100644 --- a/source/common/config/resources.h +++ b/source/common/config/resources.h @@ -2,7 +2,7 @@ #include -#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 index 64a6c2c8df61..3a91b1ea818e 100644 --- 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 index 08a694603d66..23a49ad8a65e 100644 --- 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 index 9c386440935b..d08e3a73c53d 100644 --- 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 index 8ea9b3e6c8a2..3c77556da258 100644 --- 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 index 84dce1a58111..4d154a295011 100644 --- 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 index d4f02605a0ae..0e57732a4525 100644 --- 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 index e5fdc8b046bd..0eb8d6830f87 100644 --- 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 index 905b39878b21..e427803a6323 100644 --- 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 index 1755742687a7..5cd548cf277a 100644 --- 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 index 799f0195e5eb..78103b71b5b2 100644 --- 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 index cce5cd04025f..8b5459a923e9 100644 --- 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 index f11a14b080ec..3dad3f52a689 100644 --- 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 b/source/common/singleton/const_singleton.h similarity index 60% rename from source/common/common/singleton.h rename to source/common/singleton/const_singleton.h index ee50c0a75195..f10603b1e5fa 100644 --- a/source/common/common/singleton.h +++ b/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 ConstSingleton { public: diff --git a/source/common/singleton/threadsafe_singleton.h b/source/common/singleton/threadsafe_singleton.h new file mode 100644 index 000000000000..2786a195f185 --- /dev/null +++ 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 ThreadSafeSingleton { +public: + static T& get() { + absl::call_once(ThreadSafeSingleton::create_once_, &ThreadSafeSingleton::Create); + return *ThreadSafeSingleton::instance_; + } + +protected: + template friend class TestThreadsafeSingletonInjector; + + static void Create() { instance_ = new T(); } + + static absl::once_flag create_once_; + static T* instance_; +}; + +template absl::once_flag ThreadSafeSingleton::create_once_; + +template T* ThreadSafeSingleton::instance_ = nullptr; + +} // namespace Envoy diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 8e8a835157b4..d3fc782fa4fb 100644 --- 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 index 34e35fe72f46..45ff54e8ba9b 100644 --- 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" diff --git a/source/common/tracing/zipkin/BUILD b/source/common/tracing/zipkin/BUILD index 764c3928b9d0..52c1ea52d1dc 100644 --- a/source/common/tracing/zipkin/BUILD +++ b/source/common/tracing/zipkin/BUILD @@ -41,13 +41,13 @@ envoy_cc_library( "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:enum_to_int", "//source/common/common:hex_lib", - "//source/common/common:singleton", "//source/common/common:utility_lib", "//source/common/http:header_map_lib", "//source/common/http:message_lib", "//source/common/http:utility_lib", "//source/common/json:json_loader_lib", "//source/common/network:address_lib", + "//source/common/singleton:const_singleton", "//source/common/tracing:http_tracer_lib", ], ) diff --git a/source/common/tracing/zipkin/zipkin_core_constants.h b/source/common/tracing/zipkin/zipkin_core_constants.h index 61696a26c981..84c0bcea7c66 100644 --- a/source/common/tracing/zipkin/zipkin_core_constants.h +++ b/source/common/tracing/zipkin/zipkin_core_constants.h @@ -2,7 +2,7 @@ #include -#include "common/common/singleton.h" +#include "common/singleton/const_singleton.h" namespace Envoy { namespace Zipkin { diff --git a/source/common/tracing/zipkin/zipkin_json_field_names.h b/source/common/tracing/zipkin/zipkin_json_field_names.h index 9ffb92886be0..09b841ca3192 100644 --- a/source/common/tracing/zipkin/zipkin_json_field_names.h +++ b/source/common/tracing/zipkin/zipkin_json_field_names.h @@ -2,7 +2,7 @@ #include -#include "common/common/singleton.h" +#include "common/singleton/const_singleton.h" namespace Envoy { namespace Zipkin { diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index e3ce501ebdaa..d9efcf1951ab 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -15,7 +15,6 @@ #include "server/test_hooks.h" #ifdef ENVOY_HOT_RESTART -#include "common/api/os_sys_calls_impl.h" #include "server/hot_restart_impl.h" #endif @@ -47,10 +46,9 @@ int main_common(OptionsImpl& options) { Stats::RawStatData::configure(options); #ifdef ENVOY_HOT_RESTART - Api::OsSysCallsImpl os_sys_calls_impl; std::unique_ptr restarter; try { - restarter.reset(new Server::HotRestartImpl(options, os_sys_calls_impl)); + restarter.reset(new Server::HotRestartImpl(options)); } catch (Envoy::EnvoyException& e) { std::cerr << "unable to initialize hot restart: " << e.what() << std::endl; return 1; diff --git a/source/server/BUILD b/source/server/BUILD index 77a3eedbcf4a..a0ff52b9c5b2 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -113,6 +113,7 @@ envoy_cc_library( "//include/envoy/server:hot_restart_interface", "//include/envoy/server:instance_interface", "//include/envoy/server:options_interface", + "//source/common/api:os_sys_calls_lib", "//source/common/common:assert_lib", "//source/common/common:utility_lib", "//source/common/network:utility_lib", diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 23461920c379..b643538968f7 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -13,6 +13,7 @@ #include "envoy/server/instance.h" #include "envoy/server/options.h" +#include "common/api/os_sys_calls_impl.h" #include "common/common/utility.h" #include "common/network/utility.h" @@ -25,7 +26,9 @@ namespace Server { // from working. Operations code can then cope with this and do a full restart. const uint64_t SharedMemory::VERSION = 9; -SharedMemory& SharedMemory::initialize(Options& options, Api::OsSysCalls& os_sys_calls) { +SharedMemory& SharedMemory::initialize(Options& options) { + Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); + const uint64_t entry_size = Stats::RawStatData::size(); const uint64_t total_size = sizeof(SharedMemory) + (entry_size * options.maxStats()); @@ -105,12 +108,11 @@ std::string SharedMemory::version() { return version(num_stats_, Stats::RawStatData::maxNameLength()); } -HotRestartImpl::HotRestartImpl(Options& options, Api::OsSysCalls& os_sys_calls) - : options_(options), shmem_(SharedMemory::initialize(options, os_sys_calls)), - log_lock_(shmem_.log_lock_), access_log_lock_(shmem_.access_log_lock_), - stat_lock_(shmem_.stat_lock_), init_lock_(shmem_.init_lock_) { - - my_domain_socket_ = bindDomainSocket(options.restartEpoch(), os_sys_calls); +HotRestartImpl::HotRestartImpl(Options& options) + : options_(options), shmem_(SharedMemory::initialize(options)), log_lock_(shmem_.log_lock_), + access_log_lock_(shmem_.access_log_lock_), stat_lock_(shmem_.stat_lock_), + init_lock_(shmem_.init_lock_) { + my_domain_socket_ = bindDomainSocket(options.restartEpoch()); child_address_ = createDomainSocketAddress((options.restartEpoch() + 1)); initDomainSocketAddress(&parent_address_); if (options.restartEpoch() != 0) { @@ -158,7 +160,8 @@ void HotRestartImpl::free(Stats::RawStatData& data) { memset(&data, 0, Stats::RawStatData::size()); } -int HotRestartImpl::bindDomainSocket(uint64_t id, Api::OsSysCalls& os_sys_calls) { +int HotRestartImpl::bindDomainSocket(uint64_t id) { + Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); // This actually creates the socket and binds it. We use the socket in datagram mode so we can // easily read single messages. int fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0); diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index a050980600f6..99c1a1c5f0f7 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -8,7 +8,6 @@ #include #include -#include "envoy/api/os_sys_calls.h" #include "envoy/server/hot_restart.h" #include "envoy/server/options.h" @@ -42,7 +41,7 @@ class SharedMemory { * Initialize the shared memory segment, depending on whether we should be the first running * envoy, or a host restarted envoy process. */ - static SharedMemory& initialize(Options& options, Api::OsSysCalls& os_sys_calls); + static SharedMemory& initialize(Options& options); /** * Initialize a pthread mutex for process shared locking. @@ -116,7 +115,7 @@ class HotRestartImpl : public HotRestart, public Stats::RawStatDataAllocator, Logger::Loggable { public: - HotRestartImpl(Options& options, Api::OsSysCalls& os_sys_calls); + HotRestartImpl(Options& options); Thread::BasicLockable& logLock() { return log_lock_; } Thread::BasicLockable& accessLogLock() { return access_log_lock_; } @@ -189,7 +188,7 @@ class HotRestartImpl : public HotRestart, return reinterpret_cast(base_message); } - int bindDomainSocket(uint64_t id, Api::OsSysCalls& os_sys_calls); + int bindDomainSocket(uint64_t id); void initDomainSocketAddress(sockaddr_un* address); sockaddr_un createDomainSocketAddress(uint64_t id); void onGetListenSocket(RpcGetListenSocketRequest& rpc); diff --git a/test/common/filesystem/BUILD b/test/common/filesystem/BUILD index d7f9a8c471f1..5db2ed03a0cb 100644 --- a/test/common/filesystem/BUILD +++ b/test/common/filesystem/BUILD @@ -22,6 +22,7 @@ envoy_cc_test( "//test/mocks/event:event_mocks", "//test/mocks/filesystem:filesystem_mocks", "//test/test_common:environment_lib", + "//test/test_common:threadsafe_singleton_injector_lib", ], ) diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index 36606c5277eb..a59d0fa44fdc 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -11,6 +11,7 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/filesystem/mocks.h" #include "test/test_common/environment.h" +#include "test/test_common/threadsafe_singleton_injector.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -30,10 +31,8 @@ TEST(FileSystemImpl, BadFile) { Event::MockDispatcher dispatcher; Thread::MutexBasicLockable lock; Stats::IsolatedStoreImpl store; - Api::OsSysCallsImpl os_sys_calls; EXPECT_CALL(dispatcher, createTimer_(_)); - EXPECT_THROW(Filesystem::FileImpl("", dispatcher, lock, os_sys_calls, store, - std::chrono::milliseconds(10000)), + EXPECT_THROW(Filesystem::FileImpl("", dispatcher, lock, store, std::chrono::milliseconds(10000)), EnvoyException); } @@ -85,10 +84,10 @@ TEST(FileSystemImpl, flushToLogFilePeriodically) { Thread::MutexBasicLockable mutex; Stats::IsolatedStoreImpl stats_store; NiceMock os_sys_calls; + TestThreadsafeSingletonInjector os_calls(&os_sys_calls); EXPECT_CALL(os_sys_calls, open_(_, _, _)).WillOnce(Return(5)); - Filesystem::FileImpl file("", dispatcher, mutex, os_sys_calls, stats_store, - std::chrono::milliseconds(40)); + Filesystem::FileImpl file("", dispatcher, mutex, stats_store, std::chrono::milliseconds(40)); EXPECT_CALL(*timer, enableTimer(std::chrono::milliseconds(40))); EXPECT_CALL(os_sys_calls, write_(_, _, _)) @@ -138,10 +137,10 @@ TEST(FileSystemImpl, flushToLogFileOnDemand) { Thread::MutexBasicLockable mutex; Stats::IsolatedStoreImpl stats_store; NiceMock os_sys_calls; + TestThreadsafeSingletonInjector os_calls(&os_sys_calls); EXPECT_CALL(os_sys_calls, open_(_, _, _)).WillOnce(Return(5)); - Filesystem::FileImpl file("", dispatcher, mutex, os_sys_calls, stats_store, - std::chrono::milliseconds(40)); + Filesystem::FileImpl file("", dispatcher, mutex, stats_store, std::chrono::milliseconds(40)); EXPECT_CALL(*timer, enableTimer(std::chrono::milliseconds(40))); @@ -211,11 +210,11 @@ TEST(FileSystemImpl, reopenFile) { Thread::MutexBasicLockable mutex; Stats::IsolatedStoreImpl stats_store; NiceMock os_sys_calls; + TestThreadsafeSingletonInjector os_calls(&os_sys_calls); Sequence sq; EXPECT_CALL(os_sys_calls, open_(_, _, _)).InSequence(sq).WillOnce(Return(5)); - Filesystem::FileImpl file("", dispatcher, mutex, os_sys_calls, stats_store, - std::chrono::milliseconds(40)); + Filesystem::FileImpl file("", dispatcher, mutex, stats_store, std::chrono::milliseconds(40)); EXPECT_CALL(os_sys_calls, write_(_, _, _)) .InSequence(sq) @@ -271,6 +270,7 @@ TEST(FilesystemImpl, reopenThrows) { Thread::MutexBasicLockable mutex; Stats::IsolatedStoreImpl stats_store; NiceMock os_sys_calls; + TestThreadsafeSingletonInjector os_calls(&os_sys_calls); EXPECT_CALL(os_sys_calls, write_(_, _, _)) .WillRepeatedly(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { @@ -283,8 +283,7 @@ TEST(FilesystemImpl, reopenThrows) { Sequence sq; EXPECT_CALL(os_sys_calls, open_(_, _, _)).InSequence(sq).WillOnce(Return(5)); - Filesystem::FileImpl file("", dispatcher, mutex, os_sys_calls, stats_store, - std::chrono::milliseconds(40)); + Filesystem::FileImpl file("", dispatcher, mutex, stats_store, std::chrono::milliseconds(40)); EXPECT_CALL(os_sys_calls, close(5)).InSequence(sq); EXPECT_CALL(os_sys_calls, open_(_, _, _)).InSequence(sq).WillOnce(Return(-1)); @@ -318,9 +317,9 @@ TEST(FilesystemImpl, bigDataChunkShouldBeFlushedWithoutTimer) { Thread::MutexBasicLockable mutex; Stats::IsolatedStoreImpl stats_store; NiceMock os_sys_calls; + TestThreadsafeSingletonInjector os_calls(&os_sys_calls); - Filesystem::FileImpl file("", dispatcher, mutex, os_sys_calls, stats_store, - std::chrono::milliseconds(40)); + Filesystem::FileImpl file("", dispatcher, mutex, stats_store, std::chrono::milliseconds(40)); EXPECT_CALL(os_sys_calls, write_(_, _, _)) .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { diff --git a/test/common/singleton/BUILD b/test/common/singleton/BUILD index 629da8f04ce1..dd1422d9370c 100644 --- a/test/common/singleton/BUILD +++ b/test/common/singleton/BUILD @@ -15,3 +15,13 @@ envoy_cc_test( "//source/common/singleton:manager_impl_lib", ], ) + +envoy_cc_test( + name = "threadsafe_singleton_test", + srcs = ["threadsafe_singleton_test.cc"], + deps = [ + "//source/common/common:thread_lib", + "//source/common/singleton:threadsafe_singleton", + "//test/test_common:threadsafe_singleton_injector_lib", + ], +) diff --git a/test/common/singleton/threadsafe_singleton_test.cc b/test/common/singleton/threadsafe_singleton_test.cc new file mode 100644 index 000000000000..12011b7fae8c --- /dev/null +++ b/test/common/singleton/threadsafe_singleton_test.cc @@ -0,0 +1,94 @@ +#include + +#include "common/common/thread.h" +#include "common/singleton/threadsafe_singleton.h" + +#include "test/test_common/threadsafe_singleton_injector.h" + +#include "gtest/gtest.h" + +namespace Envoy { + +class TestSingleton { +public: + virtual ~TestSingleton() {} + + virtual void addOne() { + std::unique_lock lock(lock_); + ++value_; + } + + virtual int value() { + std::unique_lock lock(lock_); + return value_; + } + +protected: + std::mutex lock_; + int value_{0}; +}; + +class EvilMathSingleton : public TestSingleton { +public: + EvilMathSingleton() { value_ = -50; } + virtual void addOne() { + std::unique_lock lock(lock_); + ++value_; + ++value_; + } +}; + +class AddTen { +public: + AddTen() { + thread_.reset(new Thread::Thread([this]() -> void { threadRoutine(); })); + } + ~AddTen() { + thread_->join(); + thread_.reset(); + } + +private: + void threadRoutine() { + auto& singleton = ThreadSafeSingleton::get(); + for (int i = 0; i < 10; ++i) { + singleton.addOne(); + } + } + Thread::ThreadPtr thread_; +}; + +TEST(ThreadSafeSingleton, BasicCreationAndMutation) { + auto& singleton = ThreadSafeSingleton::get(); + EXPECT_EQ(&singleton, &ThreadSafeSingleton::get()); + EXPECT_EQ(0, singleton.value()); + singleton.addOne(); + EXPECT_EQ(1, singleton.value()); + + { + AddTen ten; + AddTen twenty; + AddTen thirty; + } + EXPECT_EQ(31, singleton.value()); +} + +TEST(ThreadSafeSingleton, Injection) { + EvilMathSingleton evil_singleton; + + // Sanity check that other tests didn't cause the main singleton to overflow. + int latched_value = ThreadSafeSingleton::get().value(); + ASSERT_GE(latched_value, 0); + + { + TestThreadsafeSingletonInjector override(&evil_singleton); + auto& evil_math_reference = ThreadSafeSingleton::get(); + EXPECT_NE(latched_value, evil_math_reference.value()); + EXPECT_EQ(-50, evil_math_reference.value()); + evil_math_reference.addOne(); + EXPECT_EQ(-48, evil_math_reference.value()); + } + EXPECT_EQ(latched_value, ThreadSafeSingleton::get().value()); +} + +} // namespace Envoy diff --git a/test/mocks/api/BUILD b/test/mocks/api/BUILD index def769daa439..5325b4865934 100644 --- a/test/mocks/api/BUILD +++ b/test/mocks/api/BUILD @@ -15,6 +15,7 @@ envoy_cc_mock( deps = [ "//include/envoy/api:api_interface", "//include/envoy/api:os_sys_calls_interface", + "//source/common/api:os_sys_calls_lib", "//test/mocks/filesystem:filesystem_mocks", ], ) diff --git a/test/mocks/api/mocks.h b/test/mocks/api/mocks.h index 0d15602e8974..46dcacb9c430 100644 --- a/test/mocks/api/mocks.h +++ b/test/mocks/api/mocks.h @@ -7,6 +7,8 @@ #include "envoy/api/os_sys_calls.h" #include "envoy/event/dispatcher.h" +#include "common/api/os_sys_calls_impl.h" + #include "test/mocks/filesystem/mocks.h" #include "gmock/gmock.h" @@ -34,7 +36,7 @@ class MockApi : public Api { std::shared_ptr file_{new Filesystem::MockFile()}; }; -class MockOsSysCalls : public OsSysCalls { +class MockOsSysCalls : public OsSysCallsImpl { public: MockOsSysCalls(); ~MockOsSysCalls(); diff --git a/test/server/BUILD b/test/server/BUILD index 5c86a7640778..f1204e3b5ae8 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -59,9 +59,11 @@ envoy_cc_test( name = "hot_restart_impl_test", srcs = envoy_select_hot_restart(["hot_restart_impl_test.cc"]), deps = [ + "//source/common/api:os_sys_calls_lib", "//source/common/stats:stats_lib", "//source/server:hot_restart_lib", "//test/mocks/server:server_mocks", + "//test/test_common:threadsafe_singleton_injector_lib", ], ) diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index a76ce8618193..fe6f4d0efa39 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -1,9 +1,11 @@ +#include "common/api/os_sys_calls_impl.h" #include "common/stats/stats_impl.h" #include "server/hot_restart_impl.h" #include "test/mocks/api/mocks.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/threadsafe_singleton_injector.h" #include "gtest/gtest.h" @@ -33,7 +35,7 @@ class HotRestartImplTest : public testing::Test { Stats::RawStatData::configureForTestsOnly(options_); // Test we match the correct stat with empty-slots before, after, or both. - hot_restart_.reset(new HotRestartImpl(options_, os_sys_calls_)); + hot_restart_.reset(new HotRestartImpl(options_)); hot_restart_->drainParentListeners(); } @@ -45,6 +47,7 @@ class HotRestartImplTest : public testing::Test { } Api::MockOsSysCalls os_sys_calls_; + TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; NiceMock options_; std::vector buffer_; std::unique_ptr hot_restart_; @@ -75,7 +78,7 @@ TEST_F(HotRestartImplTest, crossAlloc) { EXPECT_CALL(os_sys_calls_, shmOpen(_, _, _)); EXPECT_CALL(os_sys_calls_, mmap(_, _, _, _, _, _)).WillOnce(Return(buffer_.data())); EXPECT_CALL(os_sys_calls_, bind(_, _, _)); - HotRestartImpl hot_restart2(options_, os_sys_calls_); + HotRestartImpl hot_restart2(options_); Stats::RawStatData* stat1_prime = hot_restart2.alloc("stat1"); Stats::RawStatData* stat3_prime = hot_restart2.alloc("stat3"); Stats::RawStatData* stat5_prime = hot_restart2.alloc("stat5"); diff --git a/test/test_common/BUILD b/test/test_common/BUILD index f86d23d4d28a..f467c8a19108 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -90,3 +90,11 @@ envoy_cc_test_library( "//test/mocks/thread_local:thread_local_mocks", ], ) + +envoy_cc_library( + name = "threadsafe_singleton_injector_lib", + hdrs = ["threadsafe_singleton_injector.h"], + deps = [ + "//source/common/singleton:threadsafe_singleton", + ], +) diff --git a/test/test_common/threadsafe_singleton_injector.h b/test/test_common/threadsafe_singleton_injector.h new file mode 100644 index 000000000000..d4ca322e02e5 --- /dev/null +++ b/test/test_common/threadsafe_singleton_injector.h @@ -0,0 +1,18 @@ +#include "common/singleton/threadsafe_singleton.h" + +namespace Envoy { + +// Note this class is not thread-safe, and should be called exceedingly carefully. +template class TestThreadsafeSingletonInjector { +public: + TestThreadsafeSingletonInjector(T* instance) { + latched_instance_ = &ThreadSafeSingleton::get(); + ThreadSafeSingleton::instance_ = instance; + } + ~TestThreadsafeSingletonInjector() { ThreadSafeSingleton::instance_ = latched_instance_; } + +private: + T* latched_instance_; +}; + +} // namespace Envoy