fixes feedbacks from Andres

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
envoyproxy · Aug 22, 2019 · 4ab7005 · 4ab7005
1 parent d26d9b7
commit 4ab7005
Show file tree

Hide file tree

Showing 4 changed files with 15 additions and 14 deletions.
diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h
@@ -222,8 +222,8 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf
       const ConfigUpdateCb& update_fn, const Event::PostCb& complete_cb = []() {}) {
     tls_->runOnAllThreads(
         [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<ThreadLocalConfig>().config_ = update_fn(getConfig());
         },

diff --git 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<RdsRouteConfigProviderImpl> 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
@@ -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<ScopedConfigImpl>()->getRouteConfig(
-                TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}),
-            nullptr);
+  EXPECT_THAT(getScopedRdsProvider()->config<ScopedConfigImpl>()->getRouteConfig(
+                  TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}),
+              IsNull());
   EXPECT_EQ(getScopedRdsProvider()
                 ->config<ScopedConfigImpl>()
                 ->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<ScopedConfigImpl>()->getRouteConfig(
-                TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}),
-            nullptr);
+  EXPECT_THAT(getScopedRdsProvider()->config<ScopedConfigImpl>()->getRouteConfig(
+                  TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}),
+              IsNull());
   EXPECT_EQ(getScopedRdsProvider()
                 ->config<ScopedConfigImpl>()
                 ->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<ScopedConfigImpl>(), nullptr);
-  EXPECT_EQ(getScopedRdsProvider()->config<ScopedConfigImpl>()->getRouteConfig(
-                TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}),
-            nullptr);
+  EXPECT_THAT(getScopedRdsProvider()->config<ScopedConfigImpl>()->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
@@ -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.