From 080ea476e604011db11c93c1ad77f19e7d91b020 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 18 Sep 2019 23:53:37 -0400 Subject: [PATCH 1/8] Separate out grpc init to a separate class. Signed-off-by: Joshua Marantz --- source/common/grpc/BUILD | 9 +++++++++ source/common/grpc/google_async_client_impl.cc | 2 ++ source/common/grpc/google_async_client_impl.h | 2 ++ source/exe/BUILD | 3 +-- source/exe/process_wide.cc | 10 ---------- source/exe/process_wide.h | 2 +- source/extensions/transport_sockets/alts/BUILD | 1 + source/extensions/transport_sockets/alts/config.cc | 4 ++++ 8 files changed, 20 insertions(+), 13 deletions(-) diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 603474289767..9f97f7aee58c 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -142,6 +142,7 @@ envoy_cc_library( ], deps = [ ":context_lib", + ":google_grpc_context_lib", ":google_grpc_creds_lib", ":google_grpc_utils_lib", ":typed_async_client_lib", @@ -156,6 +157,14 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "google_grpc_context_lib", + srcs = ["google_grpc_context.cc"], + hdrs = ["google_grpc_context.h"], + external_deps = ["grpc"], + deps = ["//source/common/common:assert_lib"], +) + envoy_cc_library( name = "google_grpc_creds_lib", srcs = ["google_grpc_creds_impl.cc"], diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index 9f89a70ab7f0..e8b23eb37246 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -74,6 +74,8 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher, Api::Api& api) : dispatcher_(dispatcher), tls_(tls), stat_prefix_(config.google_grpc().stat_prefix()), initial_metadata_(config.initial_metadata()), scope_(scope) { + grpc_init(); + // We rebuild the channel each time we construct the channel. It appears that the gRPC library is // smart enough to do connection pooling and reuse with identical channel args, so this should // have comparable overhead to what we are doing in Grpc::AsyncClientImpl, i.e. no expensive diff --git a/source/common/grpc/google_async_client_impl.h b/source/common/grpc/google_async_client_impl.h index da834f1b82a9..b3a1db4e4943 100644 --- a/source/common/grpc/google_async_client_impl.h +++ b/source/common/grpc/google_async_client_impl.h @@ -12,6 +12,7 @@ #include "common/common/linked_object.h" #include "common/common/thread.h" #include "common/common/thread_annotations.h" +#include "common/grpc/google_grpc_context.h" #include "common/grpc/typed_async_client.h" #include "common/tracing/http_tracer_impl.h" @@ -174,6 +175,7 @@ class GoogleAsyncClientImpl final : public RawAsyncClient, Logger::Loggable createChannel(const envoy::api::v2::core::GrpcService::GoogleGrpc& config); + GoogleGrpcContext google_grpc_context_; Event::Dispatcher& dispatcher_; GoogleAsyncClientThreadLocal& tls_; // This is shared with child streams, so that they can cleanup independent of diff --git a/source/exe/BUILD b/source/exe/BUILD index 7ed716a317a6..2b5106daad79 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -7,7 +7,6 @@ load( "envoy_cc_platform_dep", "envoy_cc_posix_library", "envoy_cc_win32_library", - "envoy_google_grpc_external_deps", "envoy_package", ) load( @@ -95,7 +94,7 @@ envoy_cc_library( "//source/common/event:libevent_lib", "//source/common/http/http2:nghttp2_lib", "//source/server:proto_descriptors_lib", - ] + envoy_google_grpc_external_deps(), + ], ) envoy_cc_library( diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 62ae2a7515ea..30eca7e45762 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -8,10 +8,6 @@ #include "ares.h" -#ifdef ENVOY_GOOGLE_GRPC -#include "grpc/grpc.h" -#endif - namespace Envoy { namespace { // Static variable to count initialization pairs. For tests like @@ -22,9 +18,6 @@ uint32_t process_wide_initialized; ProcessWide::ProcessWide() : initialization_depth_(process_wide_initialized) { if (process_wide_initialized++ == 0) { -#ifdef ENVOY_GOOGLE_GRPC - grpc_init(); -#endif ares_library_init(ARES_LIB_INIT_ALL); Event::Libevent::Global::initialize(); Envoy::Server::validateProtoDescriptors(); @@ -37,9 +30,6 @@ ProcessWide::~ProcessWide() { if (--process_wide_initialized == 0) { process_wide_initialized = false; ares_library_cleanup(); -#ifdef ENVOY_GOOGLE_GRPC - grpc_shutdown(); -#endif } ASSERT(process_wide_initialized == initialization_depth_); } diff --git a/source/exe/process_wide.h b/source/exe/process_wide.h index 2c305e8f1c27..bf77f6650e83 100644 --- a/source/exe/process_wide.h +++ b/source/exe/process_wide.h @@ -5,7 +5,7 @@ namespace Envoy { // Process-wide lifecycle events for global state in third-party dependencies, -// e.g. gRPC, c-ares. There should only ever be a single instance of this. +// e.g. c-ares. There should only ever be a single instance of this. class ProcessWide { public: ProcessWide(); diff --git a/source/extensions/transport_sockets/alts/BUILD b/source/extensions/transport_sockets/alts/BUILD index 2f65e098c4b8..a13af2b2c3ea 100644 --- a/source/extensions/transport_sockets/alts/BUILD +++ b/source/extensions/transport_sockets/alts/BUILD @@ -38,6 +38,7 @@ envoy_cc_library( ":tsi_socket", "//include/envoy/registry", "//include/envoy/server:transport_socket_config_interface", + "//source/common/grpc:google_grpc_context_lib", "//source/extensions/transport_sockets:well_known_names", "@envoy_api//envoy/config/transport_socket/alts/v2alpha:alts_cc", ], diff --git a/source/extensions/transport_sockets/alts/config.cc b/source/extensions/transport_sockets/alts/config.cc index 712f69282078..65ca6297eb07 100644 --- a/source/extensions/transport_sockets/alts/config.cc +++ b/source/extensions/transport_sockets/alts/config.cc @@ -6,6 +6,7 @@ #include "envoy/server/transport_socket_config.h" #include "common/common/assert.h" +#include "common/grpc/google_grpc_context.h" #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" @@ -65,6 +66,9 @@ class AltsSharedState : public Singleton::Instance { AltsSharedState() { grpc_alts_shared_resource_dedicated_init(); } ~AltsSharedState() override { grpc_alts_shared_resource_dedicated_shutdown(); } + +private: + Grpc::GoogleGrpcContext google_grpc_context_; }; SINGLETON_MANAGER_REGISTRATION(alts_shared_state); From 80569b986c60c813dce51ad09bc206855a30a4a9 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Sep 2019 06:48:15 -0400 Subject: [PATCH 2/8] add new files Signed-off-by: Joshua Marantz --- source/common/grpc/google_grpc_context.cc | 28 +++++++++++++++++++++++ source/common/grpc/google_grpc_context.h | 26 +++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 source/common/grpc/google_grpc_context.cc create mode 100644 source/common/grpc/google_grpc_context.h diff --git a/source/common/grpc/google_grpc_context.cc b/source/common/grpc/google_grpc_context.cc new file mode 100644 index 000000000000..ca23f9a7423d --- /dev/null +++ b/source/common/grpc/google_grpc_context.cc @@ -0,0 +1,28 @@ +#include "common/grpc/google_grpc_context.h" + +#include + +#include "common/common/assert.h" + +#include "grpcpp/grpcpp.h" + +namespace Envoy { +namespace Grpc { + +std::atomic GoogleGrpcContext::live_instances_{0}; + +GoogleGrpcContext::GoogleGrpcContext() { + if (++live_instances_ == 1) { + grpc_init(); + } +} + +GoogleGrpcContext::~GoogleGrpcContext() { + ASSERT(live_instances_ > 0); + if (--live_instances_ == 0) { + grpc_shutdown(); + } +} + +} // namespace Grpc +} // namespace Envoy diff --git a/source/common/grpc/google_grpc_context.h b/source/common/grpc/google_grpc_context.h new file mode 100644 index 000000000000..7f78855d857e --- /dev/null +++ b/source/common/grpc/google_grpc_context.h @@ -0,0 +1,26 @@ +#pragma once + +#include + +namespace Envoy { +namespace Grpc { + +// Captures global grpc initialization and shutdown. Note that grpc +// initialization starts several threads, so it is a little annoying to run them +// alongside unrelated tests, particularly if they are trying to track memory +// usage, or you are exploiting otherwise consistent run-to-run pointer values +// during debug. +// +// Instantiating this class makes it easy to ensure classes that depend on grpc +// libraries get them initialized. +class GoogleGrpcContext { +public: + GoogleGrpcContext(); + ~GoogleGrpcContext(); + +private: + static std::atomic live_instances_; +}; + +} // namespace Grpc +} // namespace Envoy From a59d3f8b705d929f38c16f378bed759c18335f31 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Sep 2019 07:01:02 -0400 Subject: [PATCH 3/8] Remove extra grpc_init(). Signed-off-by: Joshua Marantz --- source/common/grpc/google_async_client_impl.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index e8b23eb37246..9f89a70ab7f0 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -74,8 +74,6 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher, Api::Api& api) : dispatcher_(dispatcher), tls_(tls), stat_prefix_(config.google_grpc().stat_prefix()), initial_metadata_(config.initial_metadata()), scope_(scope) { - grpc_init(); - // We rebuild the channel each time we construct the channel. It appears that the gRPC library is // smart enough to do connection pooling and reuse with identical channel args, so this should // have comparable overhead to what we are doing in Grpc::AsyncClientImpl, i.e. no expensive From 9625a481620f785b092dca6ddfbb82c7f64b9a59 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Sep 2019 09:10:59 -0400 Subject: [PATCH 4/8] Ensure we initiatlize GRPC in MainCommon and add format check to ensure no one calls grpc_init directly. Signed-off-by: Joshua Marantz --- source/common/grpc/google_grpc_context.cc | 4 ++++ source/exe/BUILD | 1 + source/exe/main_common.h | 6 +++++- source/exe/process_wide.cc | 15 +++++++++++++++ tools/check_format.py | 15 +++++++++++++++ tools/check_format_test_helper.py | 3 +++ tools/testdata/check_format/grpc_init.cc | 7 +++++++ 7 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 tools/testdata/check_format/grpc_init.cc diff --git a/source/common/grpc/google_grpc_context.cc b/source/common/grpc/google_grpc_context.cc index ca23f9a7423d..c86e1851e386 100644 --- a/source/common/grpc/google_grpc_context.cc +++ b/source/common/grpc/google_grpc_context.cc @@ -13,14 +13,18 @@ std::atomic GoogleGrpcContext::live_instances_{0}; GoogleGrpcContext::GoogleGrpcContext() { if (++live_instances_ == 1) { +#ifdef ENVOY_GOOGLE_GRPC grpc_init(); +#endif } } GoogleGrpcContext::~GoogleGrpcContext() { ASSERT(live_instances_ > 0); if (--live_instances_ == 0) { +#ifdef ENVOY_GOOGLE_GRPC grpc_shutdown(); +#endif } } diff --git a/source/exe/BUILD b/source/exe/BUILD index 2b5106daad79..719cded40cc6 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -71,6 +71,7 @@ envoy_cc_library( "//source/common/api:os_sys_calls_lib", "//source/common/common:compiler_requirements_lib", "//source/common/common:perf_annotation_lib", + "//source/common/grpc:google_grpc_context_lib", "//source/common/stats:symbol_table_creator_lib", "//source/server:hot_restart_lib", "//source/server:hot_restart_nop_lib", diff --git a/source/exe/main_common.h b/source/exe/main_common.h index a0a4796de18e..96a9e9dedf88 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -5,6 +5,7 @@ #include "common/common/thread.h" #include "common/event/real_time_system.h" +#include "common/grpc/google_grpc_context.h" #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/thread_local_store.h" #include "common/thread_local/thread_local_impl.h" @@ -65,7 +66,10 @@ class MainCommonBase { const AdminRequestFn& handler); protected: - ProcessWide process_wide_; // Process-wide state setup/teardown. + ProcessWide process_wide_; // Process-wide state setup/teardown (excluding grpc). +#ifdef ENVOY_GOOGLE_GRPC + Grpc::GoogleGrpcContext google_grpc_context_; +#endif const Envoy::OptionsImpl& options_; Server::ComponentFactory& component_factory_; Thread::ThreadFactory& thread_factory_; diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 30eca7e45762..2078225290a0 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -22,6 +22,21 @@ ProcessWide::ProcessWide() : initialization_depth_(process_wide_initialized) { Event::Libevent::Global::initialize(); Envoy::Server::validateProtoDescriptors(); Http::Http2::initializeNghttp2Logging(); + + // We do not initialize Google grpc here -- we instead instantiate + // Grpc::GoogleGrpcContext in MainCommon immediately after instantiating + // ProcessWide. This is because ProcessWide is instantiated in the unit-test + // flow in test/test_runner.h, and grpc_init() instantiates threads which + // allocate memory asynchronous to running tests, making it hard to + // accurately measure memory consumption, and making unit-test debugging + // non-deterministic. See https://github.com/envoyproxy/envoy/issues/8282 + // for details. Of course we also need grpc_init called in unit-tests that + // test Google grpc, and the relevant classes must also instantiate + // Grpc::GoogleGrpcContext, which allows for nested instantiation. + // + // It appears that grpc_init() started instantiating threads in grpc 1.22.1, + // which was integrated in https://github.com/envoyproxy/envoy/pull/8196, + // around the time the flakes in #8282 started being reported. } } diff --git a/tools/check_format.py b/tools/check_format.py index 21bf410ae5fd..99e67a55be3d 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -59,6 +59,9 @@ "./source/extensions/filters/http/squash/squash_filter.cc", "./source/server/http/admin.h", "./source/server/http/admin.cc") +# Only one C++ file should instantiate grpc_init +GRPC_INIT_WHITELIST = ("./source/common/grpc/google_grpc_context.cc") + CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-8") BUILDIFIER_PATH = os.getenv("BUILDIFIER_BIN", "$GOPATH/bin/buildifier") ENVOY_BUILD_FIXER_PATH = os.path.join(os.path.dirname(os.path.abspath(sys.argv[0])), @@ -330,6 +333,10 @@ def whitelistedForStdRegex(file_path): return file_path.startswith("./test") or file_path in STD_REGEX_WHITELIST +def whitelistedForGrpcInit(file_path): + return file_path in GRPC_INIT_WHITELIST + + def findSubstringAndReturnError(pattern, file_path, error_message): with open(file_path) as f: text = f.read() @@ -583,6 +590,14 @@ def checkSourceLine(line, file_path, reportError): if not whitelistedForStdRegex(file_path) and "std::regex" in line: reportError("Don't use std::regex in code that handles untrusted input. Use RegexMatcher") + if not whitelistedForGrpcInit(file_path): + grpc_init = line.find("grpc_init()") + if grpc_init != -1: + comment = line.find("// ") + if comment == -1 or comment > grpc_init: + reportError( + "Don't call grpc_init() directly, instantiate Grpc::GoogleGrpcContext. See #8282") + def checkBuildLine(line, file_path, reportError): if "@bazel_tools" in line and not (isSkylarkFile(file_path) or file_path.startswith("./bazel/")): diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index 844488a8aa35..ee4dd5869136 100755 --- a/tools/check_format_test_helper.py +++ b/tools/check_format_test_helper.py @@ -219,6 +219,9 @@ def checkFileExpectingOK(filename): "Don't lookup stats by name at runtime; use StatName saved during construction") errors += checkUnfixableError( "regex.cc", "Don't use std::regex in code that handles untrusted input. Use RegexMatcher") + errors += checkUnfixableError( + "grpc_init.cc", + "Don't call grpc_init() directly, instantiate Grpc::GoogleGrpcContext. See #8282") errors += fixFileExpectingFailure( "api/missing_package.proto", diff --git a/tools/testdata/check_format/grpc_init.cc b/tools/testdata/check_format/grpc_init.cc new file mode 100644 index 000000000000..5f4d96fe8ca2 --- /dev/null +++ b/tools/testdata/check_format/grpc_init.cc @@ -0,0 +1,7 @@ +namespace Envoy { + +void foo() { + grpc_init(); +} + +} // namespace Envoy From 8c8abc59984720968d8a0f576744d049a47051c1 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Sep 2019 09:45:39 -0400 Subject: [PATCH 5/8] try using blocking grpc shutdown. Signed-off-by: Joshua Marantz --- source/common/grpc/google_async_client_impl.h | 4 +++- source/common/grpc/google_grpc_context.cc | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/grpc/google_async_client_impl.h b/source/common/grpc/google_async_client_impl.h index b3a1db4e4943..688c846cb2df 100644 --- a/source/common/grpc/google_async_client_impl.h +++ b/source/common/grpc/google_async_client_impl.h @@ -85,6 +85,9 @@ class GoogleAsyncClientThreadLocal : public ThreadLocal::ThreadLocalObject, private: void completionThread(); + // Instantiate this first to ensure grpc_init() is called. + GoogleGrpcContext google_grpc_context_; + // The CompletionQueue for in-flight operations. This must precede completion_thread_ to ensure it // is constructed before the thread runs. grpc::CompletionQueue cq_; @@ -175,7 +178,6 @@ class GoogleAsyncClientImpl final : public RawAsyncClient, Logger::Loggable createChannel(const envoy::api::v2::core::GrpcService::GoogleGrpc& config); - GoogleGrpcContext google_grpc_context_; Event::Dispatcher& dispatcher_; GoogleAsyncClientThreadLocal& tls_; // This is shared with child streams, so that they can cleanup independent of diff --git a/source/common/grpc/google_grpc_context.cc b/source/common/grpc/google_grpc_context.cc index c86e1851e386..6487ae2d6a5b 100644 --- a/source/common/grpc/google_grpc_context.cc +++ b/source/common/grpc/google_grpc_context.cc @@ -23,7 +23,7 @@ GoogleGrpcContext::~GoogleGrpcContext() { ASSERT(live_instances_ > 0); if (--live_instances_ == 0) { #ifdef ENVOY_GOOGLE_GRPC - grpc_shutdown(); + grpc_shutdown_blocking(); // Waiting for quiescence avoids non-determinism in tests. #endif } } From b86d9e4980b93ad8c677c93ecd157b248ce19359 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Sep 2019 19:27:47 -0400 Subject: [PATCH 6/8] use a mutex rather than a std::atomic, and also CONSTRUCT_ON_FIRST_USE macro rather than a static. Signed-off-by: Joshua Marantz --- source/common/grpc/BUILD | 7 ++++++- source/common/grpc/google_grpc_context.cc | 24 +++++++++++++++++------ source/common/grpc/google_grpc_context.h | 11 +++++++++-- source/exe/main_common.h | 2 -- tools/check_format.py | 14 ++++++++----- tools/check_format_test_helper.py | 7 ++++++- 6 files changed, 48 insertions(+), 17 deletions(-) diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 1ae02c68c428..f4d6d906e70c 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -163,7 +163,12 @@ envoy_cc_library( srcs = ["google_grpc_context.cc"], hdrs = ["google_grpc_context.h"], external_deps = ["grpc"], - deps = ["//source/common/common:assert_lib"], + deps = [ + "//source/common/common:assert_lib", + "//source/common/common:lock_guard_lib", + "//source/common/common:macros", + "//source/common/common:thread_lib", + ], ) envoy_cc_library( diff --git a/source/common/grpc/google_grpc_context.cc b/source/common/grpc/google_grpc_context.cc index 6487ae2d6a5b..0a337ec590c9 100644 --- a/source/common/grpc/google_grpc_context.cc +++ b/source/common/grpc/google_grpc_context.cc @@ -3,16 +3,18 @@ #include #include "common/common/assert.h" +#include "common/common/lock_guard.h" +#include "common/common/macros.h" +#include "common/common/thread.h" #include "grpcpp/grpcpp.h" namespace Envoy { namespace Grpc { -std::atomic GoogleGrpcContext::live_instances_{0}; - -GoogleGrpcContext::GoogleGrpcContext() { - if (++live_instances_ == 1) { +GoogleGrpcContext::GoogleGrpcContext() : instance_tracker_(instanceTracker()) { + Thread::LockGuard lock(instance_tracker_.mutex_); + if (++instance_tracker_.live_instances_ == 1) { #ifdef ENVOY_GOOGLE_GRPC grpc_init(); #endif @@ -20,13 +22,23 @@ GoogleGrpcContext::GoogleGrpcContext() { } GoogleGrpcContext::~GoogleGrpcContext() { - ASSERT(live_instances_ > 0); - if (--live_instances_ == 0) { + // Per https://github.com/grpc/grpc/issues/20303 it is OK to call + // grpc_shutdown_blocking() as long as no one can concurrently call + // grpc_init(). We use check_format.py to ensure that this file contains the + // only callers to grpc_init(), and the mutex to then make that guarantee + // across users of this class. + Thread::LockGuard lock(instance_tracker_.mutex_); + ASSERT(instance_tracker_.live_instances_ > 0); + if (--instance_tracker_.live_instances_ == 0) { #ifdef ENVOY_GOOGLE_GRPC grpc_shutdown_blocking(); // Waiting for quiescence avoids non-determinism in tests. #endif } } +GoogleGrpcContext::InstanceTracker& GoogleGrpcContext::instanceTracker() { + MUTABLE_CONSTRUCT_ON_FIRST_USE(InstanceTracker); +} + } // namespace Grpc } // namespace Envoy diff --git a/source/common/grpc/google_grpc_context.h b/source/common/grpc/google_grpc_context.h index 7f78855d857e..78a2a7218a3e 100644 --- a/source/common/grpc/google_grpc_context.h +++ b/source/common/grpc/google_grpc_context.h @@ -1,6 +1,6 @@ #pragma once -#include +#include "common/common/thread.h" namespace Envoy { namespace Grpc { @@ -19,7 +19,14 @@ class GoogleGrpcContext { ~GoogleGrpcContext(); private: - static std::atomic live_instances_; + struct InstanceTracker { + Thread::MutexBasicLockable mutex_; + uint64_t live_instances_{0}; + }; + + static InstanceTracker& instanceTracker(); + + InstanceTracker& instance_tracker_; }; } // namespace Grpc diff --git a/source/exe/main_common.h b/source/exe/main_common.h index 96a9e9dedf88..228f6e5e04a2 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -67,9 +67,7 @@ class MainCommonBase { protected: ProcessWide process_wide_; // Process-wide state setup/teardown (excluding grpc). -#ifdef ENVOY_GOOGLE_GRPC Grpc::GoogleGrpcContext google_grpc_context_; -#endif const Envoy::OptionsImpl& options_; Server::ComponentFactory& component_factory_; Thread::ThreadFactory& thread_factory_; diff --git a/tools/check_format.py b/tools/check_format.py index 63701c06f685..fed3f520dff5 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -592,12 +592,16 @@ def checkSourceLine(line, file_path, reportError): reportError("Don't use std::regex in code that handles untrusted input. Use RegexMatcher") if not whitelistedForGrpcInit(file_path): - grpc_init = line.find("grpc_init()") - if grpc_init != -1: + grpc_init_or_shutdown = line.find("grpc_init()") + grpc_shutdown = line.find("grpc_shutdown()") + if grpc_init_or_shutdown == -1 or (grpc_shutdown != -1 and + grpc_shutdown < grpc_init_or_shutdown): + grpc_init_or_shutdown = grpc_shutdown + if grpc_init_or_shutdown != -1: comment = line.find("// ") - if comment == -1 or comment > grpc_init: - reportError( - "Don't call grpc_init() directly, instantiate Grpc::GoogleGrpcContext. See #8282") + if comment == -1 or comment > grpc_init_or_shutdown: + reportError("Don't call grpc_init() or grpc_shutdown() directly, instantiate " + + "Grpc::GoogleGrpcContext. See #8282") def checkBuildLine(line, file_path, reportError): diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index ee4dd5869136..2cd0121e62de 100755 --- a/tools/check_format_test_helper.py +++ b/tools/check_format_test_helper.py @@ -221,7 +221,12 @@ def checkFileExpectingOK(filename): "regex.cc", "Don't use std::regex in code that handles untrusted input. Use RegexMatcher") errors += checkUnfixableError( "grpc_init.cc", - "Don't call grpc_init() directly, instantiate Grpc::GoogleGrpcContext. See #8282") + "Don't call grpc_init() or grpc_shutdown() directly, instantiate Grpc::GoogleGrpcContext. " + + "See #8282") + errors += checkUnfixableError( + "grpc_shutdown.cc", + "Don't call grpc_init() or grpc_shutdown() directly, instantiate Grpc::GoogleGrpcContext. " + + "See #8282") errors += fixFileExpectingFailure( "api/missing_package.proto", From ffb477cfaedd04838c2b7cf7d27db0c31a13aa36 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Sep 2019 19:32:43 -0400 Subject: [PATCH 7/8] add another test file. Signed-off-by: Joshua Marantz --- tools/testdata/check_format/grpc_shutdown.cc | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 tools/testdata/check_format/grpc_shutdown.cc diff --git a/tools/testdata/check_format/grpc_shutdown.cc b/tools/testdata/check_format/grpc_shutdown.cc new file mode 100644 index 000000000000..ff25bad98a25 --- /dev/null +++ b/tools/testdata/check_format/grpc_shutdown.cc @@ -0,0 +1,7 @@ +namespace Envoy { + +void foo() { + grpc_shutdown(); +} + +} // namespace Envoy From 1a0fc54a95019872e1fb6f00b3ebaf3d5e79b004 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 20 Sep 2019 08:15:04 -0400 Subject: [PATCH 8/8] Thread annotation and Google gRPC spelling. Signed-off-by: Joshua Marantz --- source/common/grpc/google_grpc_context.h | 2 +- source/exe/process_wide.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/grpc/google_grpc_context.h b/source/common/grpc/google_grpc_context.h index 78a2a7218a3e..b83f29a12214 100644 --- a/source/common/grpc/google_grpc_context.h +++ b/source/common/grpc/google_grpc_context.h @@ -21,7 +21,7 @@ class GoogleGrpcContext { private: struct InstanceTracker { Thread::MutexBasicLockable mutex_; - uint64_t live_instances_{0}; + uint64_t live_instances_ GUARDED_BY(mutex_) = 0; }; static InstanceTracker& instanceTracker(); diff --git a/source/exe/process_wide.cc b/source/exe/process_wide.cc index 2078225290a0..219921c081bb 100644 --- a/source/exe/process_wide.cc +++ b/source/exe/process_wide.cc @@ -23,7 +23,7 @@ ProcessWide::ProcessWide() : initialization_depth_(process_wide_initialized) { Envoy::Server::validateProtoDescriptors(); Http::Http2::initializeNghttp2Logging(); - // We do not initialize Google grpc here -- we instead instantiate + // We do not initialize Google gRPC here -- we instead instantiate // Grpc::GoogleGrpcContext in MainCommon immediately after instantiating // ProcessWide. This is because ProcessWide is instantiated in the unit-test // flow in test/test_runner.h, and grpc_init() instantiates threads which @@ -31,7 +31,7 @@ ProcessWide::ProcessWide() : initialization_depth_(process_wide_initialized) { // accurately measure memory consumption, and making unit-test debugging // non-deterministic. See https://github.com/envoyproxy/envoy/issues/8282 // for details. Of course we also need grpc_init called in unit-tests that - // test Google grpc, and the relevant classes must also instantiate + // test Google gRPC, and the relevant classes must also instantiate // Grpc::GoogleGrpcContext, which allows for nested instantiation. // // It appears that grpc_init() started instantiating threads in grpc 1.22.1,