From 4ab7005fadeabc1ce924cb064d53cc0f6cce375e Mon Sep 17 00:00:00 2001 From: Xin Zhuang Date: Thu, 22 Aug 2019 08:54:12 -0400 Subject: [PATCH] fixes feedbacks from Andres Signed-off-by: Xin Zhuang --- source/common/config/config_provider_impl.h | 4 ++-- source/common/router/scoped_rds.h | 4 +++- test/common/router/scoped_rds_test.cc | 19 ++++++++++--------- .../scoped_rds_integration_test.cc | 2 -- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 6d083c164a40..4a7499c5abdb 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -222,8 +222,8 @@ class ConfigSubscriptionCommonBase : protected Logger::LoggablerunOnAllThreads( [this, update_fn]() { - // NOTE: there is a known race condition between *this* subscription be teared down in - // main thread and the posted callback been executed before the destruction. See more + // NOTE: there is a known race condition between *this* subscription being teared down in + // main thread and the posted callback being executed before the destruction. See more // details in https://github.com/envoyproxy/envoy/issues/7902 tls_->getTyped().config_ = update_fn(getConfig()); }, diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index bde1b1969ea7..107921c738ad 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -105,7 +105,7 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio const ScopedRouteMap& scopedRouteMap() const { return scoped_route_map_; } private: - // A helper class that takes care the life circle management of a RDS route provider and the + // A helper class that takes care the life cycle management of a RDS route provider and the // update callback handle. struct RdsRouteConfigProviderHelper { RdsRouteConfigProviderHelper( @@ -118,6 +118,8 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio ScopedRdsConfigSubscription& parent_; std::string scope_name_; std::unique_ptr route_provider_; + // This handle_ is owned by the route config provider's RDS subscrption, when the helper + // destructs, the handle is deleted as well. Common::CallbackHandle* rds_update_callback_handle_; }; diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index dfe3c4634806..a01a494a0b7e 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -25,6 +25,7 @@ using testing::DoAll; using testing::Eq; using testing::InSequence; using testing::Invoke; +using testing::IsNull; using testing::NiceMock; using testing::Return; using testing::ReturnRefOfCopy; @@ -309,9 +310,9 @@ route_configuration_name: foo_routes 2UL, factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); // now scope key "x-bar-key" points to nowhere. - EXPECT_EQ(getScopedRdsProvider()->config()->getRouteConfig( - TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}), - nullptr); + EXPECT_THAT(getScopedRdsProvider()->config()->getRouteConfig( + TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}), + IsNull()); EXPECT_EQ(getScopedRdsProvider() ->config() ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) @@ -388,9 +389,9 @@ route_configuration_name: foo_routes 2UL, factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); // now scope key "x-bar-key" points to nowhere. - EXPECT_EQ(getScopedRdsProvider()->config()->getRouteConfig( - TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}), - nullptr); + EXPECT_THAT(getScopedRdsProvider()->config()->getRouteConfig( + TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}), + IsNull()); EXPECT_EQ(getScopedRdsProvider() ->config() ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) @@ -429,9 +430,9 @@ route_configuration_name: foo_routes // Scope key "x-foo-key" points to nowhere. EXPECT_NE(getScopedRdsProvider(), nullptr); EXPECT_NE(getScopedRdsProvider()->config(), nullptr); - EXPECT_EQ(getScopedRdsProvider()->config()->getRouteConfig( - TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}), - nullptr); + EXPECT_THAT(getScopedRdsProvider()->config()->getRouteConfig( + TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}), + IsNull()); factory_context_.init_manager_.initialize(init_watcher_); init_watcher_.expectReady().Times( 1); // Just SRDS, RDS "foo_routes" will initialized by the noop init-manager. diff --git a/test/integration/scoped_rds_integration_test.cc b/test/integration/scoped_rds_integration_test.cc index 11ff06916cc3..e14e87d08fa7 100644 --- a/test/integration/scoped_rds_integration_test.cc +++ b/test/integration/scoped_rds_integration_test.cc @@ -219,8 +219,6 @@ route_configuration_name: foo_route1 // RDS updates won't affect SRDS. test_server_->waitForGaugeEq("http.config_test.scoped_rds.foo-scoped-routes.version", 6927017134761466251UL); - // TODO(AndresGuedez): test actual scoped routing logic; only the config handling is implemented - // at this point. } // Test that a bad config update updates the corresponding stats.