You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zookeeper.apache.org by nk...@apache.org on 2021/10/20 07:32:24 UTC
[zookeeper] branch branch-3.5 updated: ZOOKEEPER-4367:
Zookeeper#Login thread leak in case of Sasl AuthFailed.
This is an automated email from the ASF dual-hosted git repository.
nkalmar pushed a commit to branch branch-3.5
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/branch-3.5 by this push:
new 1741656 ZOOKEEPER-4367: Zookeeper#Login thread leak in case of Sasl AuthFailed.
1741656 is described below
commit 17416567b77d6602dc3b46298c7c989cd4f15701
Author: Rushabh Shah <ru...@rushabh-ltmflld.internal.salesforce.com>
AuthorDate: Wed Oct 20 09:32:18 2021 +0200
ZOOKEEPER-4367: Zookeeper#Login thread leak in case of Sasl AuthFailed.
Backport of https://github.com/apache/zookeeper/pull/1755
Author: Rushabh Shah <ru...@rushabh-ltmflld.internal.salesforce.com>
Reviewers: Norbert Kalmar <nk...@apache.org>
Closes #1767 from shahrs87/ZOOKEEPER-4367-branch-3.5
---
.../main/java/org/apache/zookeeper/ClientCnxn.java | 21 +++++++++++++++------
.../main/java/org/apache/zookeeper/ZooKeeper.java | 2 +-
.../zookeeper/client/ZooKeeperSaslClient.java | 3 ++-
.../java/org/apache/zookeeper/SaslAuthTest.java | 21 +++++++++++++++------
4 files changed, 33 insertions(+), 14 deletions(-)
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 2e369fc..4f9d18b 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
@@ -199,9 +199,6 @@ public class ClientCnxn {
*/
volatile boolean seenRwServerBefore = false;
-
- public ZooKeeperSaslClient zooKeeperSaslClient;
-
private final ZKClientConfig clientConfig;
/**
* If any request's response in not received in configured requestTimeout
@@ -807,6 +804,8 @@ public class ClientCnxn {
private final ClientCnxnSocket clientCnxnSocket;
private Random r = new Random();
private boolean isFirstConnect = true;
+ private volatile ZooKeeperSaslClient zooKeeperSaslClient;
+
void readResponse(ByteBuffer incomingBuffer) throws IOException {
ByteBufferInputStream bbis = new ByteBufferInputStream(
@@ -1267,8 +1266,13 @@ public class ClientCnxn {
eventThread.queueEvent(new WatchedEvent(Event.EventType.None,
Event.KeeperState.Disconnected, null));
}
+
eventThread.queueEvent(new WatchedEvent(Event.EventType.None,
Event.KeeperState.Closed, null));
+
+ if (zooKeeperSaslClient != null) {
+ zooKeeperSaslClient.shutdown();
+ }
ZooTrace.logTraceMessage(LOG, ZooTrace.getTextTraceLevel(),
"SendThread exited loop for session: 0x"
+ Long.toHexString(getSessionId()));
@@ -1438,6 +1442,10 @@ public class ClientCnxn {
public void sendPacket(Packet p) throws IOException {
clientCnxnSocket.sendPacket(p);
}
+
+ public ZooKeeperSaslClient getZooKeeperSaslClient() {
+ return zooKeeperSaslClient;
+ }
}
/**
@@ -1459,9 +1467,6 @@ public class ClientCnxn {
LOG.warn("Got interrupted while waiting for the sender thread to close", ex);
}
eventThread.queueEventOfDeath();
- if (zooKeeperSaslClient != null) {
- zooKeeperSaslClient.shutdown();
- }
}
/**
@@ -1664,4 +1669,8 @@ public class ClientCnxn {
throw e;
}
}
+
+ public ZooKeeperSaslClient getZooKeeperSaslClient() {
+ return sendThread.getZooKeeperSaslClient();
+ }
}
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 8372dfe..4ee58a9 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
@@ -222,7 +222,7 @@ public class ZooKeeper implements AutoCloseable {
}
public ZooKeeperSaslClient getSaslClient() {
- return cnxn.zooKeeperSaslClient;
+ return cnxn.getZooKeeperSaslClient();
}
protected final ZKWatchManager watchManager;
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/client/ZooKeeperSaslClient.java b/zookeeper-server/src/main/java/org/apache/zookeeper/client/ZooKeeperSaslClient.java
index 0f5f307..29446a9 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/client/ZooKeeperSaslClient.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/client/ZooKeeperSaslClient.java
@@ -218,7 +218,7 @@ public class ZooKeeperSaslClient {
// data[] contains the Zookeeper Server's SASL token.
// ctx is the ZooKeeperSaslClient object. We use this object's respondToServer() method
// to reply to the Zookeeper Server's SASL token
- ZooKeeperSaslClient client = ((ClientCnxn)ctx).zooKeeperSaslClient;
+ ZooKeeperSaslClient client = ((ClientCnxn) ctx).getZooKeeperSaslClient();
if (client == null) {
LOG.warn("sasl client was unexpectedly null: cannot respond to Zookeeper server.");
return;
@@ -469,6 +469,7 @@ public class ZooKeeperSaslClient {
public void shutdown() {
if (null != login) {
login.shutdown();
+ login = null;
}
}
}
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/SaslAuthTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/SaslAuthTest.java
index 088fe1f..0cb6b56 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/SaslAuthTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/SaslAuthTest.java
@@ -18,8 +18,6 @@
package org.apache.zookeeper;
-import static org.junit.Assert.assertTrue;
-
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
@@ -33,6 +31,7 @@ import org.apache.zookeeper.ClientCnxn.EventThread;
import org.apache.zookeeper.ClientCnxn.SendThread;
import org.apache.zookeeper.Watcher.Event.KeeperState;
import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.client.ZooKeeperSaslClient;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Id;
import org.apache.zookeeper.test.ClientBase;
@@ -41,6 +40,10 @@ import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
public class SaslAuthTest extends ClientBase {
@BeforeClass
public static void init() {
@@ -237,16 +240,22 @@ public class SaslAuthTest extends ClientBase {
Field eventThreadField = clientCnxn.getClass().getDeclaredField("eventThread");
eventThreadField.setAccessible(true);
EventThread eventThread = (EventThread) eventThreadField.get(clientCnxn);
+ ZooKeeperSaslClient zooKeeperSaslClient = clientCnxn.getZooKeeperSaslClient();
+ assertNotNull(zooKeeperSaslClient);
sendThread.join(CONNECTION_TIMEOUT);
eventThread.join(CONNECTION_TIMEOUT);
+ Field loginField = zooKeeperSaslClient.getClass().getDeclaredField("login");
+ loginField.setAccessible(true);
+ Login login = (Login) loginField.get(zooKeeperSaslClient);
+ // If login is null, this means ZooKeeperSaslClient#shutdown method has been called which in turns
+ // means that Login#shutdown has been called.
+ assertNull(login);
Assert.assertFalse("SendThread did not shutdown after authFail", sendThread.isAlive());
- Assert.assertFalse("EventThread did not shutdown after authFail",
- eventThread.isAlive());
+ Assert.assertFalse("EventThread did not shutdown after authFail", eventThread.isAlive());
} finally {
if (zk != null) {
zk.close();
}
}
}
-
-}
+}
\ No newline at end of file