From 5a201d633fd90a14ba76795683be36b05baedff0 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Tue, 25 Aug 2026 02:50:59 -0400 Subject: [PATCH 1/4] ZOOKEEPER-4947: Close clients after authentication failure ZooKeeper.close() treated AUTH_FAILED clients as already closed and skipped prompt shutdown of their connection threads. Signed-off-by: 1fanwang <1fannnw@gmail.com> --- .../src/main/java/org/apache/zookeeper/ZooKeeper.java | 2 +- .../org/apache/zookeeper/test/SaslAuthFailTest.java | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java index 239f97b0f70..20e8e2cddd2 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java @@ -1319,7 +1319,7 @@ public synchronized void register(Watcher watcher) { * @throws InterruptedException */ public synchronized void close() throws InterruptedException { - if (!cnxn.getState().isAlive()) { + if (cnxn.getState() == States.CLOSED) { LOG.debug("Close called on already closed client"); return; } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java index 2384cd612ef..3f586c247d3 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java @@ -18,11 +18,14 @@ package org.apache.zookeeper.test; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import java.io.File; import java.io.FileWriter; import java.io.IOException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.WatchedEvent; import org.apache.zookeeper.Watcher.Event.KeeperState; @@ -88,9 +91,12 @@ public void testAuthFail() { @Test public void testBadSaslAuthNotifiesWatch() throws Exception { - try (ZooKeeper ignored = createClient(new MyWatcher(), hostPort)) { + try (ZooKeeper zk = createClient(new MyWatcher(), hostPort)) { // wait for authFailed event from client's EventThread. - authFailed.await(); + assertTrue(authFailed.await(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS)); + zk.close(); + assertEquals(ZooKeeper.States.CLOSED, zk.getState()); + assertTrue(zk.close(CONNECTION_TIMEOUT)); } } From 2dd3c4446b90b701460b5e7efe1c7af5cbc741f8 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Sat, 19 Sep 2026 23:25:41 -0700 Subject: [PATCH 2/4] ZOOKEEPER-4947: Stop send loop after SASL authentication failure Signed-off-by: 1fanwang <1fannnw@gmail.com> --- .../src/main/java/org/apache/zookeeper/ClientCnxn.java | 1 + .../test/java/org/apache/zookeeper/TestableZooKeeper.java | 5 +++++ .../java/org/apache/zookeeper/test/SaslAuthFailTest.java | 8 +++++++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java index 020f9408aab..e167dff1d1b 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java @@ -1217,6 +1217,7 @@ public void run() { eventThread.queueEvent(new WatchedEvent(Watcher.Event.EventType.None, authState, null)); if (state == States.AUTH_FAILED) { eventThread.queueEventOfDeath(); + break; } } } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java b/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java index 7f9e41e3380..da9e5a7cc2e 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java @@ -85,6 +85,11 @@ public void run() { } } + @Override + public boolean testableWaitForShutdown(int wait) throws InterruptedException { + return super.testableWaitForShutdown(wait); + } + public SocketAddress testableLocalSocketAddress() { return super.testableLocalSocketAddress(); } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java index 3f586c247d3..c8b419f0cc7 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java @@ -19,6 +19,7 @@ package org.apache.zookeeper.test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import java.io.File; @@ -27,6 +28,8 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.TestableZooKeeper; import org.apache.zookeeper.WatchedEvent; import org.apache.zookeeper.Watcher.Event.KeeperState; import org.apache.zookeeper.ZooDefs.Ids; @@ -91,9 +94,12 @@ public void testAuthFail() { @Test public void testBadSaslAuthNotifiesWatch() throws Exception { - try (ZooKeeper zk = createClient(new MyWatcher(), hostPort)) { + try (TestableZooKeeper zk = createClient(new MyWatcher(), hostPort)) { // wait for authFailed event from client's EventThread. assertTrue(authFailed.await(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS)); + assertTrue(zk.testableWaitForShutdown(1000), "Client threads should stop after SASL authentication fails"); + assertEquals(ZooKeeper.States.AUTH_FAILED, zk.getState()); + assertThrows(KeeperException.AuthFailedException.class, () -> zk.exists("/", false)); zk.close(); assertEquals(ZooKeeper.States.CLOSED, zk.getState()); assertTrue(zk.close(CONNECTION_TIMEOUT)); From 888306580053b0cbb1f3690688b26e0bbfc5fcca Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Sun, 20 Sep 2026 00:01:41 -0700 Subject: [PATCH 3/4] ZOOKEEPER-4947: Log the no-close SASL shutdown result Signed-off-by: 1fanwang <1fannnw@gmail.com> --- .../java/org/apache/zookeeper/test/SaslAuthFailTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java index c8b419f0cc7..44d936fe054 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java @@ -97,7 +97,10 @@ public void testBadSaslAuthNotifiesWatch() throws Exception { try (TestableZooKeeper zk = createClient(new MyWatcher(), hostPort)) { // wait for authFailed event from client's EventThread. assertTrue(authFailed.await(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS)); - assertTrue(zk.testableWaitForShutdown(1000), "Client threads should stop after SASL authentication fails"); + boolean threadsStopped = zk.testableWaitForShutdown(1000); + LOG.info("SASL failure shutdown without close: threadsStopped={}, state={}", + threadsStopped, zk.getState()); + assertTrue(threadsStopped, "Client threads should stop after SASL authentication fails"); assertEquals(ZooKeeper.States.AUTH_FAILED, zk.getState()); assertThrows(KeeperException.AuthFailedException.class, () -> zk.exists("/", false)); zk.close(); From 60ad965e4b81b9e36e522ea40f708131a02b6f5c Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 23 Sep 2026 10:46:55 -0700 Subject: [PATCH 4/4] ZOOKEEPER-4947: Avoid JMX races in rejected-client tests Wait for AuthFailed on intentionally rejected clients rather than requiring their server connection to remain registered in JMX. Signed-off-by: 1fanwang <1fannnw@gmail.com> --- .../zookeeper/test/SaslAuthFailTest.java | 4 +-- .../test/SaslAuthRequiredMultiClientTest.java | 32 ++++++++++++------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java index 44d936fe054..0dfd2cc955c 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthFailTest.java @@ -83,7 +83,7 @@ public synchronized void process(WatchedEvent event) { @Test public void testAuthFail() { - try (ZooKeeper zk = createClient()) { + try (ZooKeeper zk = new ZooKeeper(hostPort, CONNECTION_TIMEOUT, new CountdownWatcher())) { zk.create("/path1", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); fail("Should have gotten exception."); } catch (Exception e) { @@ -94,7 +94,7 @@ public void testAuthFail() { @Test public void testBadSaslAuthNotifiesWatch() throws Exception { - try (TestableZooKeeper zk = createClient(new MyWatcher(), hostPort)) { + try (TestableZooKeeper zk = new TestableZooKeeper(hostPort, CONNECTION_TIMEOUT, new MyWatcher())) { // wait for authFailed event from client's EventThread. assertTrue(authFailed.await(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS)); boolean threadsStopped = zk.testableWaitForShutdown(1000); diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java index f21d634558b..cd082f51457 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslAuthRequiredMultiClientTest.java @@ -19,10 +19,15 @@ package org.apache.zookeeper.test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import javax.security.auth.login.Configuration; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.Watcher.Event.KeeperState; import org.apache.zookeeper.ZooDefs.Ids; import org.apache.zookeeper.ZooKeeper; import org.junit.jupiter.api.AfterAll; @@ -55,12 +60,7 @@ public void testClientOpWithInvalidSASLUserAuthAfterSuccessLogin() throws Except } resetJaasConfiguration("jaas.conf", "super_wrong", "test"); - try (ZooKeeper wrongUserZk = createClient()) { - wrongUserZk.create("/bar", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); - fail("Client with wrong SASL config should not pass SASL authentication."); - } catch (KeeperException e) { - assertEquals(KeeperException.Code.AUTHFAILED, e.code()); - } + assertClientAuthFailed(); } @Test @@ -73,11 +73,21 @@ public void testClientOpWithInvalidSASLPasswordAuthAfterSuccessLogin() throws Ex } resetJaasConfiguration("jaas.conf", "super", "test_wrongong"); - try (ZooKeeper wrongPasswordZk = createClient()) { - wrongPasswordZk.create("/bar", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); - fail("Client with wrong SASL config should not pass SASL authentication."); - } catch (KeeperException e) { - assertEquals(KeeperException.Code.AUTHFAILED, e.code()); + assertClientAuthFailed(); + } + + private void assertClientAuthFailed() throws Exception { + CountDownLatch authFailed = new CountDownLatch(1); + // A rejected connection may disappear before createClient's JMX check. + try (ZooKeeper zk = new ZooKeeper(hostPort, CONNECTION_TIMEOUT, event -> { + if (event.getState() == KeeperState.AuthFailed) { + authFailed.countDown(); + } + })) { + assertTrue(authFailed.await(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS)); + assertEquals(ZooKeeper.States.AUTH_FAILED, zk.getState()); + assertThrows(KeeperException.AuthFailedException.class, + () -> zk.create("/bar", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT)); } }