From 81c70cb839588801c307a62e2bc1e0370fb06d42 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Sun, 13 Dec 2020 22:04:03 +0000 Subject: [PATCH] aggregate cluster: fix TLS init issue Fixes #14119 Signed-off-by: Matt Klein --- .../extensions/clusters/aggregate/cluster.cc | 4 +++- .../http/jwt_authn/filter_config_test.cc | 2 +- .../redis_proxy/conn_pool_impl_test.cc | 2 +- .../lightstep/lightstep_tracer_impl_test.cc | 2 +- test/mocks/thread_local/mocks.h | 20 +++++++++++++------ 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/source/extensions/clusters/aggregate/cluster.cc b/source/extensions/clusters/aggregate/cluster.cc index b53278880addb..59bcbb72f6021 100644 --- a/source/extensions/clusters/aggregate/cluster.cc +++ b/source/extensions/clusters/aggregate/cluster.cc @@ -20,7 +20,9 @@ Cluster::Cluster(const envoy::config::cluster::v3::Cluster& cluster, : Upstream::ClusterImplBase(cluster, runtime, factory_context, std::move(stats_scope), added_via_api, factory_context.dispatcher().timeSource()), cluster_manager_(cluster_manager), runtime_(runtime), random_(random), tls_(tls), - clusters_(config.clusters().begin(), config.clusters().end()) {} + clusters_(config.clusters().begin(), config.clusters().end()) { + tls_.set([](Event::Dispatcher&) { return nullptr; }); +} PriorityContextPtr Cluster::linearizePrioritySet(const std::function& skip_predicate) { diff --git a/test/extensions/filters/http/jwt_authn/filter_config_test.cc b/test/extensions/filters/http/jwt_authn/filter_config_test.cc index 6aae1a8e1b39e..8406afd8c50d5 100644 --- a/test/extensions/filters/http/jwt_authn/filter_config_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_config_test.cc @@ -168,7 +168,7 @@ TEST(HttpJwtAuthnFilterConfigTest, VerifyTLSLifetime) { NiceMock server_context; // Make sure that the thread callbacks are not invoked inline. - server_context.thread_local_.defer_data = true; + server_context.thread_local_.defer_data_ = true; { // Scope in all the things that the filter depends on, so they are destroyed as we leave the // scope. diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index cec880ba92897..1211390c625d8 100644 --- a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc @@ -1174,7 +1174,7 @@ TEST_F(RedisConnPoolImplTest, AskRedirectionFailure) { TEST_F(RedisConnPoolImplTest, MakeRequestAndRedirectFollowedByDelete) { cm_.initializeThreadLocalClusters({"fake_cluster"}); - tls_.defer_delete = true; + tls_.defer_delete_ = true; std::unique_ptr> store = std::make_unique>(); cluster_refresh_manager_ = diff --git a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc index 239e30a856244..6513314c0ba05 100644 --- a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc +++ b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc @@ -201,7 +201,7 @@ TEST_F(LightStepDriverTest, DeferredTlsInitialization) { auto propagation_mode = Common::Ot::OpenTracingDriver::PropagationMode::TracerNative; - tls_.defer_data = true; + tls_.defer_data_ = true; cm_.initializeClusters({"fake_cluster"}, {}); ON_CALL(*cm_.active_clusters_["fake_cluster"]->info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); diff --git a/test/mocks/thread_local/mocks.h b/test/mocks/thread_local/mocks.h index 7b3097f5e0fa1..e735c543b0e1b 100644 --- a/test/mocks/thread_local/mocks.h +++ b/test/mocks/thread_local/mocks.h @@ -50,25 +50,32 @@ class MockInstance : public Instance { ~SlotImpl() override { // Do not actually clear slot data during shutdown. This mimics the production code. - // The defer_delete mimics the recycle() code with Bookkeeper. - if (!parent_.shutdown_ && !parent_.defer_delete) { + // The defer_delete mimics the slot being deleted on the main thread but the update not yet + // getting to a worker. + if (!parent_.shutdown_ && !parent_.defer_delete_) { EXPECT_LT(index_, parent_.data_.size()); parent_.data_[index_].reset(); } } // ThreadLocal::Slot - ThreadLocalObjectSharedPtr get() override { return parent_.data_[index_]; } + ThreadLocalObjectSharedPtr get() override { + EXPECT_TRUE(was_set_); + return parent_.data_[index_]; + } bool currentThreadRegistered() override { return parent_.registered_; } void runOnAllThreads(const UpdateCb& cb) override { + EXPECT_TRUE(was_set_); parent_.runOnAllThreads([cb, this]() { cb(parent_.data_[index_]); }); } void runOnAllThreads(const UpdateCb& cb, const Event::PostCb& main_callback) override { + EXPECT_TRUE(was_set_); parent_.runOnAllThreads([cb, this]() { cb(parent_.data_[index_]); }, main_callback); } void set(InitializeCb cb) override { - if (parent_.defer_data) { + was_set_ = true; + if (parent_.defer_data_) { parent_.deferred_data_[index_] = cb; } else { parent_.data_[index_] = cb(parent_.dispatcher_); @@ -77,6 +84,7 @@ class MockInstance : public Instance { MockInstance& parent_; const uint32_t index_; + bool was_set_{}; // set() must be called before other functions. }; void call() { @@ -90,10 +98,10 @@ class MockInstance : public Instance { testing::NiceMock dispatcher_; std::vector data_; std::vector deferred_data_; - bool defer_data{}; + bool defer_data_{}; bool shutdown_{}; bool registered_{true}; - bool defer_delete{}; + bool defer_delete_{}; }; } // namespace ThreadLocal