ZOOKEEPER-4946: Fix Login renewal thread not exiting on shutdown

zookeeper-server/src/main/java/org/apache/zookeeper/Login.java

@@ -69,6 +69,11 @@ public class Login {
     private static final long MIN_TIME_BEFORE_RELOGIN = Long.getLong(
       MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY, DEFAULT_MIN_TIME_BEFORE_RELOGIN);
 
+    private static final long DEFAULT_SHUTDOWN_TIMEOUT = 5 * 1000L;
+    public static final String SHUTDOWN_TIMEOUT_CONFIG_KEY = "zookeeper.kerberos.shutdownTimeoutMs";
+    private static final long SHUTDOWN_TIMEOUT = Math.max(0L,
+            Long.getLong(SHUTDOWN_TIMEOUT_CONFIG_KEY, DEFAULT_SHUTDOWN_TIMEOUT));
+
     private Subject subject = null;
     private Thread t = null;
     private boolean isKrbTicket = false;
@@ -132,7 +137,7 @@ public Login(final String loginContextName, Supplier<CallbackHandler> callbackHa
         t = new Thread(new Runnable() {
             public void run() {
                 LOG.info("TGT refresh thread started.");
-                while (true) {  // renewal thread's main loop. if it exits from here, thread will exit.
+                while (!Thread.currentThread().isInterrupted()) {  // renewal thread's main loop. if it exits from here, thread will exit.
                     KerberosTicket tgt = getTGT();
                     long now = Time.currentWallTime();
                     long nextRefresh;
@@ -262,6 +267,7 @@ public void run() {
                                     }
                                 } else {
                                     LOG.error("Could not refresh TGT for principal: {}.", principal, le);
+                                    break;
                                 }
                             }
                         }
@@ -297,9 +303,13 @@ public void shutdown() {
         if ((t != null) && (t.isAlive())) {
             t.interrupt();
             try {
-                t.join();
+                t.join(SHUTDOWN_TIMEOUT);
+                if (t.isAlive()) {
+                    LOG.warn("TGT renewal thread did not exit within {} ms", SHUTDOWN_TIMEOUT);
+                }
             } catch (InterruptedException e) {
                 LOG.warn("error while waiting for Login thread to shutdown.", e);
+                Thread.currentThread().interrupt();
             }
         }
     }

zookeeper-server/src/main/java/org/apache/zookeeper/Shell.java

@@ -235,6 +235,7 @@ public void run() {
                 errThread.join();
             } catch (InterruptedException ie) {
                 LOG.warn("Interrupted while reading the error stream", ie);
+                Thread.currentThread().interrupt();
             }
             completed.set(true);
             //the timeout thread handling
@@ -243,6 +244,7 @@ public void run() {
                 throw new ExitCodeException(exitCode, errMsg.toString());
             }
         } catch (InterruptedException ie) {
+            Thread.currentThread().interrupt();
             throw new IOException(ie.toString());
         } finally {
             if ((timeOutTimer != null) && !timedOut.get()) {

zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java

@@ -24,6 +24,7 @@
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import java.io.File;
 import java.io.FileWriter;
@@ -34,11 +35,13 @@
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Supplier;
 import javax.security.auth.login.Configuration;
 import javax.security.auth.login.LoginException;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.FilenameUtils;
+import org.apache.zookeeper.common.Time;
 import org.apache.zookeeper.common.ZKConfig;
 import org.apache.zookeeper.server.quorum.auth.KerberosTestUtils;
 import org.apache.zookeeper.server.quorum.auth.MiniKdc;
@@ -72,6 +75,7 @@ public static void setupClass() throws Exception {
     // by default, we should wait at least 1 minute between subsequent TGT renewals.
     // changing it to 500ms.
     System.setProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY, "500");
+    System.setProperty(Login.SHUTDOWN_TIMEOUT_CONFIG_KEY, "200");
 
     testTempDir = ClientBase.createTmpDir();
     startMiniKdcAndAddPrincipal();
@@ -99,6 +103,7 @@ public static void setupClass() throws Exception {
   @AfterAll
   public static void tearDownClass() {
     System.clearProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY);
+    System.clearProperty(Login.SHUTDOWN_TIMEOUT_CONFIG_KEY);
     System.clearProperty("java.security.auth.login.config");
     stopMiniKdc();
     if (testTempDir != null) {
@@ -125,6 +130,12 @@ private static class TestableKerberosLogin extends Login {
 
     private AtomicBoolean refreshFailed = new AtomicBoolean(false);
     private CountDownLatch continueRefreshThread = new CountDownLatch(1);
+    private volatile boolean hangUninterruptibly = false;
+    private final CountDownLatch hungThreadLatch = new CountDownLatch(1);
+    private volatile boolean attemptEveryReLogin = false;
+    private final CountDownLatch retrySleeps = new CountDownLatch(2);
+    private final AtomicInteger reLoginAttempts = new AtomicInteger();
+    private volatile int attemptsAtSecondRetrySleep;
 
     public TestableKerberosLogin() throws LoginException {
       super(JAAS_CONFIG_SECTION, () -> {
@@ -136,10 +147,52 @@ public TestableKerberosLogin() throws LoginException {
     protected void sleepBeforeRetryFailedRefresh() throws InterruptedException {
       LOG.info("sleep started due to failed refresh");
       refreshFailed.set(true);
-      continueRefreshThread.await(20, TimeUnit.SECONDS);
+      if (retrySleeps.getCount() == 1) {
+        attemptsAtSecondRetrySleep = reLoginAttempts.get();
+      }
+      retrySleeps.countDown();
+      if (hangUninterruptibly) {
+        while (hungThreadLatch.getCount() > 0) {
+          try {
+            hungThreadLatch.await();
+          } catch (InterruptedException ignored) {
+          }
+        }
+      } else if (!attemptEveryReLogin) {
+        continueRefreshThread.await(20, TimeUnit.SECONDS);
+      }
       LOG.info("sleep due to failed refresh finished");
     }
 
+    @Override
+    protected synchronized void logout() throws LoginException {
+      reLoginAttempts.incrementAndGet();
+      super.logout();
+    }
+
+    @Override
+    public long getLastLogin() {
+      return attemptEveryReLogin ? Time.currentElapsedTime() - TimeUnit.HOURS.toMillis(1) : super.getLastLogin();
+    }
+
+    public void attemptEveryReLogin() {
+      attemptEveryReLogin = true;
+    }
+
+    public void assertRenewalLoopCameRoundAgain(Duration timeout) throws InterruptedException {
+      assertTrue(retrySleeps.await(timeout.toMillis(), TimeUnit.MILLISECONDS),
+          "renewal thread never left the reLogin retry loop");
+      assertTrue(attemptsAtSecondRetrySleep >= 3, "some reLogin attempts were skipped");
+    }
+
+    public void hangUninterruptiblyOnFailedRefresh() {
+      hangUninterruptibly = true;
+    }
+
+    public void releaseHungThread() {
+      hungThreadLatch.countDown();
+    }
+
     public void assertRefreshFailsEventually(Duration timeout) {
       assertEventually(timeout, () -> refreshFailed.get());
     }
@@ -200,6 +253,39 @@ public void shouldRecoverIfKerberosNotAvailableForSomeTime() throws Exception {
   }
 
 
+  @Test
+  public void shouldNotBlockForeverWhenRenewalThreadDoesNotExit() throws Exception {
+    login = new TestableKerberosLogin();
+    login.hangUninterruptiblyOnFailedRefresh();
+    login.startThreadIfNeeded();
+
+    stopMiniKdc();
+    login.assertRefreshFailsEventually(Duration.ofSeconds(15));
+
+    try {
+      assertTimeoutPreemptively(Duration.ofSeconds(10), () -> login.shutdown());
+    } finally {
+      startMiniKdcAndAddPrincipal();
+      login.releaseHungThread();
+    }
+  }
+
+
+  @Test
+  public void shouldLeaveRetryLoopWhenReLoginKeepsFailing() throws Exception {
+    login = new TestableKerberosLogin();
+    login.attemptEveryReLogin();
+    login.startThreadIfNeeded();
+
+    stopMiniKdc();
+    try {
+      login.assertRenewalLoopCameRoundAgain(Duration.ofSeconds(15));
+    } finally {
+      startMiniKdcAndAddPrincipal();
+    }
+  }
+
+
   private void assertPrincipalLoggedIn() {
     assertEquals(PRINCIPAL, login.getUserName());
     assertNotNull(login.getSubject());

zookeeper-server/src/test/java/org/apache/zookeeper/ShellTest.java

@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledOnOs;
+import org.junit.jupiter.api.condition.OS;
+
+public class ShellTest {
+
+    @Test
+    @DisabledOnOs(OS.WINDOWS)
+    public void shouldRestoreInterruptStatusWhenInterruptedWhileWaitingForProcess() throws Exception {
+        CountDownLatch started = new CountDownLatch(1);
+        CountDownLatch finished = new CountDownLatch(1);
+        AtomicBoolean interruptedAfterwards = new AtomicBoolean(false);
+        AtomicReference<IOException> expectedFailure = new AtomicReference<>();
+        AtomicReference<Throwable> unexpectedFailure = new AtomicReference<>();
+
+        Thread runner = new Thread(() -> {
+            started.countDown();
+            try {
+                Shell.execCommand("sh", "-c", "exec >/dev/null 2>&1; sleep 2");
+            } catch (IOException e) {
+                expectedFailure.set(e);
+            } catch (Throwable t) {
+                unexpectedFailure.set(t);
+            } finally {
+                interruptedAfterwards.set(Thread.currentThread().isInterrupted());
+                finished.countDown();
+            }
+        });
+        runner.start();
+
+        assertTrue(started.await(10, TimeUnit.SECONDS), "runner thread did not start");
+
+        long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10);
+        while (runner.getState() != Thread.State.WAITING) {
+            assertTrue(System.nanoTime() < deadline, "runner thread never reached Process.waitFor()");
+            Thread.onSpinWait();
+        }
+        runner.interrupt();
+
+        assertTrue(finished.await(30, TimeUnit.SECONDS), "execCommand did not return");
+        runner.join();
+
+        assertNull(unexpectedFailure.get(), "unexpected failure in runner thread");
+        assertNotNull(expectedFailure.get(), "execCommand was expected to fail with an IOException");
+        assertTrue(interruptedAfterwards.get(), "execCommand cleared the interrupt status");
+    }
+
+}

zookeeper-website/app/pages/_docs/docs/_mdx/admin-ops/administrators-guide/configuration-parameters.mdx

@@ -1350,6 +1350,14 @@ server if this feature is enabled with sasl as authentication scheme.
   It is essentially the quorum equivalent of the _zookeeper.sasl.client.canonicalize.hostname_ property for clients.
   The default value is **false** for backwards compatibility.
 
+- _kerberos.shutdownTimeoutMs_
+  (Java system property: **zookeeper.kerberos.shutdownTimeoutMs**)
+  **New in 3.10.0:**
+  The time in milliseconds ZooKeeper waits for the Kerberos TGT renewal thread to exit while shutting down.
+  If the thread is still alive after that, ZooKeeper logs a warning and completes the shutdown.
+  Set it to 0 to wait without a timeout, which is how ZooKeeper behaved before this setting existed.
+  Default: 5000
+
 - _multiAddress.enabled_ :
   (Java system property: **zookeeper.multiAddress.enabled**)
   **New in 3.6.0:**

zookeeper-website/app/pages/_docs/docs/_mdx/developer/programmers-guide/bindings.mdx

@@ -105,6 +105,13 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 - _zookeeper.server.realm_ :
   Realm part of the server principal. By default it is the client principal realm.
 
+- _zookeeper.kerberos.shutdownTimeoutMs_ :
+  **New in 3.10.0:**
+  The time in milliseconds a client waits for the Kerberos TGT renewal thread to exit while closing.
+  If the thread is still alive after that, the client logs a warning and completes the close.
+  Set it to 0 to wait without a timeout, which is how the client behaved before this setting existed.
+  Default: 5000
+
 - _zookeeper.disableAutoWatchReset_ :
   This switch controls whether automatic watch resetting is enabled. Clients automatically
   reset watches during session reconnect by default, this option allows the client to turn off