Skip to content

Commit 20d67f5

Browse files
committed
xds: Improve shutdown handling of XdsDepManager
The most important change here is to handle subscribeToCluster() calls after shutdown(), and preventing the internal state from being heavily confused as the assumption is there are no watchers after shutdown(). ClusterSubscription.closed isn't strictly necessary, but I don't want the code to depend on double-deregistration being safe. maybePublishConfig() isn't being called after shutdown(), but adding the protection avoids a class of bugs that would cause channel panic.
1 parent 83538cd commit 20d67f5

1 file changed

Lines changed: 23 additions & 8 deletions

File tree

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,15 @@ public static String toContextStr(String typeName, String resourceName) {
9393

9494
@Override
9595
public Closeable subscribeToCluster(String clusterName) {
96-
9796
checkNotNull(clusterName, "clusterName");
9897
ClusterSubscription subscription = new ClusterSubscription(clusterName);
9998

10099
syncContext.execute(() -> {
101-
addClusterWatcher(clusterName, subscription, 1);
102-
maybePublishConfig();
100+
if (getWatchers(XdsListenerResource.getInstance()).isEmpty()) {
101+
subscription.closed = true;
102+
return; // shutdown() called
103+
}
104+
addClusterWatcher(clusterName, subscription);
103105
});
104106

105107
return subscription;
@@ -207,10 +209,14 @@ private void releaseSubscription(ClusterSubscription subscription) {
207209
checkNotNull(subscription, "subscription");
208210
String clusterName = subscription.getClusterName();
209211
syncContext.execute(() -> {
212+
if (subscription.closed) {
213+
return;
214+
}
215+
subscription.closed = true;
210216
XdsWatcherBase<?> cdsWatcher =
211217
resourceWatchers.get(CLUSTER_RESOURCE).watchers.get(clusterName);
212218
if (cdsWatcher == null) {
213-
return; // already released while waiting for the syncContext
219+
return; // shutdown() called
214220
}
215221
cancelClusterWatcherTree((CdsWatcher) cdsWatcher, subscription);
216222
maybePublishConfig();
@@ -263,6 +269,9 @@ private void maybePublishConfig() {
263269
if (waitingOnResource) {
264270
return;
265271
}
272+
if (getWatchers(XdsListenerResource.getInstance()).isEmpty()) {
273+
return; // shutdown() called
274+
}
266275

267276
StatusOr<XdsConfig> newUpdate = buildUpdate();
268277
if (Objects.equals(newUpdate, lastUpdate)) {
@@ -293,6 +302,11 @@ StatusOr<XdsConfig> buildUpdate() {
293302
routeSource = ((LdsWatcher) ldsWatcher).getRouteSource();
294303
}
295304

305+
if (routeSource == null) {
306+
return StatusOr.fromStatus(Status.UNAVAILABLE.withDescription(
307+
"Bug: No route source found for listener " + dataPlaneAuthority));
308+
}
309+
296310
StatusOr<RdsUpdate> statusOrRdsUpdate = routeSource.getRdsUpdate();
297311
if (!statusOrRdsUpdate.hasValue()) {
298312
return StatusOr.fromStatus(statusOrRdsUpdate.getStatus());
@@ -557,14 +571,15 @@ public interface XdsConfigWatcher {
557571
void onUpdate(StatusOr<XdsConfig> config);
558572
}
559573

560-
private class ClusterSubscription implements Closeable {
561-
String clusterName;
574+
private final class ClusterSubscription implements Closeable {
575+
private final String clusterName;
576+
boolean closed; // Accessed from syncContext
562577

563578
public ClusterSubscription(String clusterName) {
564-
this.clusterName = clusterName;
579+
this.clusterName = checkNotNull(clusterName, "clusterName");
565580
}
566581

567-
public String getClusterName() {
582+
String getClusterName() {
568583
return clusterName;
569584
}
570585

0 commit comments

Comments
 (0)