From 181741ca4aedd8b748aeb8e8c628fb94a0d7c695 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Wed, 11 Jan 2023 14:42:40 -0800 Subject: [PATCH 1/2] xds:fix cancel xds watcher accidentally removes the url --- .../main/java/io/grpc/xds/XdsClientImpl.java | 4 +-- .../io/grpc/xds/XdsClientImplTestBase.java | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java index 26392acc733..fe61ef95eb8 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java +++ b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java @@ -328,13 +328,13 @@ public void run() { if (!subscriber.isWatched()) { subscriber.cancelResourceWatch(); resourceSubscribers.get(type).remove(resourceName); - subscribedResourceTypeUrls.remove(type.typeUrl()); - subscribedResourceTypeUrls.remove(type.typeUrlV2()); if (subscriber.xdsChannel != null) { subscriber.xdsChannel.adjustResourceSubscription(type); } if (resourceSubscribers.get(type).isEmpty()) { resourceSubscribers.remove(type); + subscribedResourceTypeUrls.remove(type.typeUrl()); + subscribedResourceTypeUrls.remove(type.typeUrlV2()); } } } diff --git a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java index bc91f39cb2f..e4ca9ce1f79 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java @@ -921,6 +921,33 @@ public void ldsResourceUpdated() { assertThat(channelForEmptyAuthority).isNull(); } + @Test + public void ldsResourceUpdated_() { + DiscoveryRpcCall call = startResourceWatcher(XdsListenerResource.getInstance(), LDS_RESOURCE, + ldsResourceWatcher); + verifyResourceMetadataRequested(LDS, LDS_RESOURCE); + + // Initial LDS response. + call.sendResponse(LDS, testListenerVhosts, VERSION_1, "0000"); + call.verifyRequest(LDS, LDS_RESOURCE, VERSION_1, "0000", NODE); + verify(ldsResourceWatcher).onChanged(ldsUpdateCaptor.capture()); + verifyGoldenListenerVhosts(ldsUpdateCaptor.getValue()); + verifyResourceMetadataAcked(LDS, LDS_RESOURCE, testListenerVhosts, VERSION_1, TIME_INCREMENT); + + xdsClient.watchXdsResource(XdsListenerResource.getInstance(), + LDS_RESOURCE + "1", ldsResourceWatcher); + xdsClient.cancelXdsResourceWatch(XdsListenerResource.getInstance(), LDS_RESOURCE + "1",ldsResourceWatcher); + + // Updated LDS response. + Any testListenerVhosts2 = Any.pack(mf.buildListenerWithApiListener(LDS_RESOURCE, + mf.buildRouteConfiguration("new", mf.buildOpaqueVirtualHosts(VHOST_SIZE)))); + call.sendResponse(LDS, testListenerVhosts2, VERSION_2, "0001"); + call.verifyRequest(LDS, LDS_RESOURCE, VERSION_2, "0001", NODE); + verify(ldsResourceWatcher).onChanged(ldsUpdateCaptor.capture()); + verifyGoldenListenerVhosts(ldsUpdateCaptor.getValue()); + verifyResourceMetadataAcked(LDS, LDS_RESOURCE, testListenerVhosts2, VERSION_2, TIME_INCREMENT * 2); + } + @Test public void ldsResourceUpdated_withXdstpResourceName() { BootstrapperImpl.enableFederation = true; From b69a57dfe9ef0fa4c6834fa9ed9e7e19af425f76 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Wed, 11 Jan 2023 15:16:26 -0800 Subject: [PATCH 2/2] fix style --- xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java index e4ca9ce1f79..2a91d7b5caf 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java @@ -922,7 +922,7 @@ public void ldsResourceUpdated() { } @Test - public void ldsResourceUpdated_() { + public void cancelResourceWatcherNotRemoveUrlSubscribers() { DiscoveryRpcCall call = startResourceWatcher(XdsListenerResource.getInstance(), LDS_RESOURCE, ldsResourceWatcher); verifyResourceMetadataRequested(LDS, LDS_RESOURCE); @@ -936,7 +936,8 @@ public void ldsResourceUpdated_() { xdsClient.watchXdsResource(XdsListenerResource.getInstance(), LDS_RESOURCE + "1", ldsResourceWatcher); - xdsClient.cancelXdsResourceWatch(XdsListenerResource.getInstance(), LDS_RESOURCE + "1",ldsResourceWatcher); + xdsClient.cancelXdsResourceWatch(XdsListenerResource.getInstance(), LDS_RESOURCE + "1", + ldsResourceWatcher); // Updated LDS response. Any testListenerVhosts2 = Any.pack(mf.buildListenerWithApiListener(LDS_RESOURCE, @@ -945,7 +946,8 @@ public void ldsResourceUpdated_() { call.verifyRequest(LDS, LDS_RESOURCE, VERSION_2, "0001", NODE); verify(ldsResourceWatcher).onChanged(ldsUpdateCaptor.capture()); verifyGoldenListenerVhosts(ldsUpdateCaptor.getValue()); - verifyResourceMetadataAcked(LDS, LDS_RESOURCE, testListenerVhosts2, VERSION_2, TIME_INCREMENT * 2); + verifyResourceMetadataAcked(LDS, LDS_RESOURCE, testListenerVhosts2, VERSION_2, + TIME_INCREMENT * 2); } @Test