fixup: code review comments

eugeneo · Dec 19, 2023 · 8c4dcbc · 8c4dcbc
1 parent 7e6328c
commit 8c4dcbc
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 8 deletions.
diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc
@@ -729,8 +729,6 @@ absl::Status XdsClient::ChannelState::AdsCallState::AdsResponseParser::
         fields.type_url.c_str(), fields.version.c_str(), fields.nonce.c_str(),
         fields.num_resources);
   }
-  result_.read_delay_handle =
-      MakeRefCounted<AdsReadDelayHandle>(ads_call_state_->Ref());
   result_.type =
       ads_call_state_->xds_client()->GetResourceTypeLocked(fields.type_url);
   if (result_.type == nullptr) {
@@ -740,6 +738,8 @@ absl::Status XdsClient::ChannelState::AdsCallState::AdsResponseParser::
   result_.type_url = std::move(fields.type_url);
   result_.version = std::move(fields.version);
   result_.nonce = std::move(fields.nonce);
+  result_.read_delay_handle =
+      MakeRefCounted<AdsReadDelayHandle>(ads_call_state_->Ref());
   return absl::OkStatus();
 }
 

diff --git a/test/core/xds/xds_client_test.cc b/test/core/xds/xds_client_test.cc
@@ -2740,7 +2740,7 @@ TEST_F(XdsClientTest, AdsReadWaitsForHandleRelease) {
           .set_version_info("1")
           .set_nonce("A")
           .AddFooResource(XdsFooResource("foo1", 6))
-          .AddFooResource(XdsFooResource("foo2", 6))
+          .AddFooResource(XdsFooResource("foo2", 10))
           .Serialize());
   // Send a response with a single resource, will not be read until the handle
   // is released
@@ -2753,8 +2753,12 @@ TEST_F(XdsClientTest, AdsReadWaitsForHandleRelease) {
   // XdsClient should have delivered the response to the watcher.
   auto resource1 = watcher1->WaitForNextResourceAndHandle();
   ASSERT_NE(resource1, absl::nullopt);
+  EXPECT_EQ(resource1->resource->name, "foo1");
+  EXPECT_EQ(resource1->resource->value, 6);
   auto resource2 = watcher2->WaitForNextResourceAndHandle();
   ASSERT_NE(resource2, absl::nullopt);
+  EXPECT_EQ(resource2->resource->name, "foo2");
+  EXPECT_EQ(resource2->resource->value, 10);
   // XdsClient should have sent an ACK message to the xDS server.
   request = WaitForRequest(stream.get());
   ASSERT_TRUE(request.has_value());
@@ -2763,21 +2767,23 @@ TEST_F(XdsClientTest, AdsReadWaitsForHandleRelease) {
                /*error_detail=*/absl::OkStatus(),
                /*resource_names=*/{"foo1", "foo2"});
   EXPECT_EQ(stream->reads_started(), 1);
-  resource1 = absl::nullopt;
-  resource2 = absl::nullopt;
+  resource1->read_delay_handle.reset();
+  EXPECT_EQ(stream->reads_started(), 1);
+  resource2->read_delay_handle.reset();
   EXPECT_EQ(stream->reads_started(), 2);
   resource1 = watcher1->WaitForNextResourceAndHandle();
-  resource2 = watcher2->WaitForNextResourceAndHandle();
   ASSERT_NE(resource1, absl::nullopt);
-  EXPECT_EQ(resource2, absl::nullopt);
+  EXPECT_EQ(resource1->resource->name, "foo1");
+  EXPECT_EQ(resource1->resource->value, 8);
+  EXPECT_EQ(watcher2->WaitForNextResourceAndHandle(), absl::nullopt);
   // XdsClient should have sent an ACK message to the xDS server.
   request = WaitForRequest(stream.get());
   ASSERT_TRUE(request.has_value());
   CheckRequest(*request, XdsFooResourceType::Get()->type_url(),
                /*version_info=*/"2", /*response_nonce=*/"B",
                /*error_detail=*/absl::OkStatus(),
                /*resource_names=*/{"foo1", "foo2"});
-  resource1 = absl::nullopt;
+  resource1->read_delay_handle.reset();
   EXPECT_EQ(stream->reads_started(), 3);
   // Cancel watch.
   CancelFooWatch(watcher1.get(), "foo1");