This is an automated email from the ASF dual-hosted git repository.
anmolnar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/master by this push:
new ec016b6ac0 ZOOKEEPER-4946: Fix Login renewal thread not exiting on
shutdown
ec016b6ac0 is described below
commit ec016b6ac0ed72190774c5d5dcb131f7c8f208de
Author: Ivan Khanas <[email protected]>
AuthorDate: Tue Sep 29 20:18:17 2026 +0200
ZOOKEEPER-4946: Fix Login renewal thread not exiting on shutdown
Reviewers: kezhuw
Author: ikxeno
Closes #2446 from ikxeno/ZOOKEEPER-4946
---
.../src/main/java/org/apache/zookeeper/Login.java | 14 +++-
.../src/main/java/org/apache/zookeeper/Shell.java | 2 +
.../zookeeper/KerberosTicketRenewalTest.java | 88 +++++++++++++++++++++-
.../test/java/org/apache/zookeeper/ShellTest.java | 76 +++++++++++++++++++
.../configuration-parameters.mdx | 8 ++
.../_mdx/developer/programmers-guide/bindings.mdx | 7 ++
6 files changed, 192 insertions(+), 3 deletions(-)
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/Login.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/Login.java
index 2c483a5f74..54974fc0fe 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/Login.java
+++ b/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();
}
}
}
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/Shell.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/Shell.java
index f780ff9855..54cf15c671 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/Shell.java
+++ b/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()) {
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java
index d0f52b152a..ea57aae6e5 100644
---
a/zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java
+++
b/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());
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/ShellTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/ShellTest.java
new file mode 100644
index 0000000000..6b7b47e5bf
--- /dev/null
+++ b/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");
+ }
+
+}
diff --git
a/zookeeper-website/app/pages/_docs/docs/_mdx/admin-ops/administrators-guide/configuration-parameters.mdx
b/zookeeper-website/app/pages/_docs/docs/_mdx/admin-ops/administrators-guide/configuration-parameters.mdx
index 7606196b36..ab7eb90504 100644
---
a/zookeeper-website/app/pages/_docs/docs/_mdx/admin-ops/administrators-guide/configuration-parameters.mdx
+++
b/zookeeper-website/app/pages/_docs/docs/_mdx/admin-ops/administrators-guide/configuration-parameters.mdx
@@ -1411,6 +1411,14 @@ the integrity of IP-based access controls.
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:**
diff --git
a/zookeeper-website/app/pages/_docs/docs/_mdx/developer/programmers-guide/bindings.mdx
b/zookeeper-website/app/pages/_docs/docs/_mdx/developer/programmers-guide/bindings.mdx
index 3e6cb734b0..5dbb821538 100644
---
a/zookeeper-website/app/pages/_docs/docs/_mdx/developer/programmers-guide/bindings.mdx
+++
b/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
