ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes #1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471e3 [Mohammad Arshad] Corrected impacted test cases
12f44d62e [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

(cherry picked from commit 86690ff40cb5c9f5782f0971db16e8fd1c3528e6)
Signed-off-by: Mohammad Arshad <arshad@apache.org>

4 files changed, 93 insertions, 13 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 096020f18..c724bc7ea 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
@@ -812,7 +812,12 @@ public class ZooKeeper implements AutoCloseable {
      * @throws IOException
      *             in cases of network failure
      * @throws IllegalArgumentException
-     *             if an invalid chroot path is specified
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(String connectString, int sessionTimeout, Watcher watcher) throws IOException {
         this(connectString, sessionTimeout, watcher, false);
@@ -861,7 +866,12 @@ public class ZooKeeper implements AutoCloseable {
      * @throws IOException
      *             in cases of network failure
      * @throws IllegalArgumentException
-     *             if an invalid chroot path is specified
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -926,7 +936,12 @@ public class ZooKeeper implements AutoCloseable {
      * @throws IOException
      *             in cases of network failure
      * @throws IllegalArgumentException
-     *             if an invalid chroot path is specified
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -994,7 +1009,12 @@ public class ZooKeeper implements AutoCloseable {
      * @throws IOException
      *             in cases of network failure
      * @throws IllegalArgumentException
-     *             if an invalid chroot path is specified
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -1009,6 +1029,7 @@ public class ZooKeeper implements AutoCloseable {
             sessionTimeout,
             watcher);
 
+        validateWatcher(watcher);
         if (clientConfig == null) {
             clientConfig = new ZKClientConfig();
         }
@@ -1098,7 +1119,12 @@ public class ZooKeeper implements AutoCloseable {
      * @throws IOException
      *             in cases of network failure
      * @throws IllegalArgumentException
-     *             if an invalid chroot path is specified
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -1160,7 +1186,12 @@ public class ZooKeeper implements AutoCloseable {
      * @throws IOException
      *             in cases of network failure
      * @throws IllegalArgumentException
-     *             if an invalid chroot path is specified
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -1226,8 +1257,13 @@ public class ZooKeeper implements AutoCloseable {
      *            password for this session
      *
      * @throws IOException in cases of network failure
-     * @throws IllegalArgumentException if an invalid chroot path is specified
-     * @throws IllegalArgumentException for an invalid list of ZooKeeper hosts
+     * @throws IllegalArgumentException
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -1300,7 +1336,13 @@ public class ZooKeeper implements AutoCloseable {
      * @param aHostProvider
      *            use this as HostProvider to enable custom behaviour.
      * @throws IOException in cases of network failure
-     * @throws IllegalArgumentException if an invalid chroot path is specified
+     * @throws IllegalArgumentException
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -1387,7 +1429,12 @@ public class ZooKeeper implements AutoCloseable {
      *            configuring properties differently compared to other instances
      * @throws IOException in cases of network failure
      * @throws IllegalArgumentException if an invalid chroot path is specified
-     *
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      * @since 3.5.5
      */
     public ZooKeeper(
@@ -1408,6 +1455,7 @@ public class ZooKeeper implements AutoCloseable {
             Long.toHexString(sessionId),
             (sessionPasswd == null ? "<null>" : "<hidden>"));
 
+        validateWatcher(watcher);
         if (clientConfig == null) {
             clientConfig = new ZKClientConfig();
         }
@@ -1491,7 +1539,13 @@ public class ZooKeeper implements AutoCloseable {
      *            allowed while write requests are not. It continues seeking for
      *            majority in the background.
      * @throws IOException in cases of network failure
-     * @throws IllegalArgumentException if an invalid chroot path is specified
+     * @throws IllegalArgumentException
+     *             if any of the following is true:
+     *             <ul>
+     *             <li> if an invalid chroot path is specified
+     *             <li> for an invalid list of ZooKeeper hosts
+     *             <li> watcher is null
+     *             </ul>
      */
     public ZooKeeper(
         String connectString,
@@ -1579,10 +1633,12 @@ public class ZooKeeper implements AutoCloseable {
     /**
      * Specify the default watcher for the connection (overrides the one
      * specified during construction).
+     * @throws IllegalArgumentException if watcher is null
      *
      * @param watcher
      */
     public synchronized void register(Watcher watcher) {
+        validateWatcher(watcher);
         watchManager.defaultWatcher = watcher;
     }
 
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/ZooKeeperTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/ZooKeeperTest.java
index 8fc3df250..48e2468d1 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/ZooKeeperTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/ZooKeeperTest.java
@@ -726,4 +726,28 @@ public class ZooKeeperTest extends ClientBase {
         assertTrue("ZooKeeperMain does not wait until the specified timeout",
                 endTime - startTime >= timeout);
     }
+
+    @Test
+    public void testInvalidWatcherRegistration() throws Exception {
+        String nullWatcherMsg = "Invalid Watcher, shouldn't be null!";
+        final ZooKeeper zk = createClient();
+        try {
+            zk.register(null);
+            fail("Should validate null watcher!");
+        } catch (IllegalArgumentException iae) {
+            assertEquals(nullWatcherMsg, iae.getMessage());
+        }
+        try {
+            new ZooKeeper(hostPort, 10000, null, false);
+            fail("Should validate null watcher!");
+        } catch (IllegalArgumentException iae) {
+            assertEquals(nullWatcherMsg, iae.getMessage());
+        }
+        try {
+            new ZooKeeper(hostPort, 10000, null, 10L, "".getBytes(), false);
+            fail("Should validate null watcher!");
+        } catch (IllegalArgumentException iae) {
+            assertEquals(nullWatcherMsg, iae.getMessage());
+        }
+    }
 }
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumDigestTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumDigestTest.java
index 691b45513..2f7873de5 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumDigestTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumDigestTest.java
@@ -203,7 +203,7 @@ public class QuorumDigestTest extends QuorumPeerTestBase {
 
     private void triggerOps(int sid, String prefix) throws Exception {
         TxnLogDigestTest.performOperations(servers.zk[sid], prefix);
-        servers.restartClient(sid, null);
+        servers.restartClient(sid, event -> { });
         waitForOne(servers.zk[sid], States.CONNECTED);
     }
 
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java
index ee54a7ae5..2d7735cdd 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java
@@ -413,7 +413,7 @@ public class ObserverMasterTest extends QuorumPeerTestBase implements Watcher {
     public void testInOrderCommits() throws Exception {
         setUp(-1);
 
-        zk = new ZooKeeper("127.0.0.1:" + CLIENT_PORT_QP1, ClientBase.CONNECTION_TIMEOUT, null);
+        zk = new ZooKeeper("127.0.0.1:" + CLIENT_PORT_QP1, ClientBase.CONNECTION_TIMEOUT, event -> { });
         for (int i = 0; i < 10; i++) {
             zk.create("/bulk"
                               + i, ("Initial data of some size").getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);