xds: Fix shutdownNow() becoming a no-op after shutdown()`

This fixes `XdsServerWrapper.shutdownNow()` silently doing nothing once `shutdown()` had already
been called, permanently hanging threads blocked in `start()`. Previously they shared a single guard
flag, meaning whichever was called first made the other a complete no-op.

The fix gives `shutdownNow()`'s forceful-only work its own independent guard, following the same
two-guard pattern already used by `ManagedChannelImpl`/`ServerImpl` in grpc-java core, so it always
runs exactly once regardless of call order.

Includes a regression test that reproduces the hang against the old code and passes against the fix.

Signed-off-by: Martin Baillie <martin@baillie.id>

Author: martinbaillie

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

@@ -118,6 +118,10 @@ public void uncaughtException(Thread t, Throwable e) {
   private final CountDownLatch internalTerminationLatch = new CountDownLatch(1);
   private final SettableFuture<Exception> initialStartFuture = SettableFuture.create();
   private boolean initialStarted;
+  // Must be accessed in syncContext.
+  // Guards the forceful-shutdown work in shutdownNow(), independently of the shutdown AtomicBoolean
+  // above, so it isn't skipped when shutdown()
+  private boolean shutdownNowed;
   private ScheduledHandle restartTimer;
   private ObjectPool<XdsClient> xdsClientPool;
   private XdsClient xdsClient;
@@ -408,16 +412,15 @@ public void run() {
 
   @Override
   public Server shutdownNow() {
-    if (!shutdown.compareAndSet(false, true)) {
-      return this;
-    }
+    shutdown();
     syncContext.execute(new Runnable() {
       @Override
       public void run() {
-        if (!delegate.isShutdown()) {
-          delegate.shutdownNow();
+        if (shutdownNowed) {
+          return;
         }
-        internalShutdown();
+        shutdownNowed = true;
+        delegate.shutdownNow();
         initialStartFuture.set(new IOException("server is forcefully shut down"));
       }
     });

xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java

@@ -487,6 +487,62 @@ public void run() {
     }
   }
 
+  @Test
+  public void shutdownNow_afterShutdown_stillUnblocksStartThread() throws Exception {
+    final SettableFuture<Server> start = SettableFuture.create();
+    Executors.newSingleThreadExecutor()
+        .execute(
+            new Runnable() {
+              @Override
+              public void run() {
+                try {
+                  start.set(xdsServerWrapper.start());
+                } catch (Exception ex) {
+                  start.setException(ex);
+                }
+              }
+            });
+    assertThat(xdsClient.ldsResource.get(5, TimeUnit.SECONDS))
+        .isEqualTo("grpc/server?udpa.resource.listening_address=0.0.0.0:1");
+    xdsServerWrapper.shutdown();
+    xdsServerWrapper.shutdownNow();
+    try {
+      start.get(5, TimeUnit.SECONDS);
+      fail("should have thrown but not");
+    } catch (ExecutionException ex) {
+      assertThat(ex).hasCauseThat().isInstanceOf(IOException.class);
+      assertThat(ex).hasCauseThat().hasMessageThat().isEqualTo("server is forcefully shut down");
+    }
+  }
+
+  @Test
+  public void shutdownNow_calledTwice_forcefullyShutsDownDelegateOnce() throws Exception {
+    final SettableFuture<Server> start = SettableFuture.create();
+    Executors.newSingleThreadExecutor()
+        .execute(
+            new Runnable() {
+              @Override
+              public void run() {
+                try {
+                  start.set(xdsServerWrapper.start());
+                } catch (Exception ex) {
+                  start.setException(ex);
+                }
+              }
+            });
+    assertThat(xdsClient.ldsResource.get(5, TimeUnit.SECONDS))
+        .isEqualTo("grpc/server?udpa.resource.listening_address=0.0.0.0:1");
+    xdsServerWrapper.shutdownNow();
+    xdsServerWrapper.shutdownNow();
+    try {
+      start.get(5, TimeUnit.SECONDS);
+      fail("should have thrown but not");
+    } catch (ExecutionException ex) {
+      assertThat(ex).hasCauseThat().isInstanceOf(IOException.class);
+    }
+    verify(mockServer, times(1)).shutdownNow();
+  }
+
   @Test
   public void initialStartIoException() throws Exception {
     final SettableFuture<Server> start = SettableFuture.create();