ZOOKEEPER-1909. removeWatches doesn't return NOWATCHER when there is

no watch set (Raul Gutierrez Segales via rakeshr)

git-svn-id: https://svn.apache.org/repos/asf/zookeeper/trunk@1588141 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -610,6 +610,9 @@ BUGFIXES:
 
   ZOOKEEPER-1840. Server tries to connect to itself during dynamic reconfig
   (Alexander Shraer via michim)
+ 
+  ZOOKEEPER-1909. removeWatches doesn't return NOWATCHER when there is
+  no watch set (Raul Gutierrez Segales via rakeshr)
 
 IMPROVEMENTS:
 

src/java/main/org/apache/zookeeper/ZooKeeper.java

@@ -196,7 +196,7 @@ public class ZooKeeper {
         return cnxn.zooKeeperSaslClient;
     }
 
-    private final ZKWatchManager watchManager = new ZKWatchManager();
+    private final ZKWatchManager watchManager;
 
     List<String> getDataWatches() {
         synchronized(watchManager.dataWatches) {
@@ -374,7 +374,7 @@ public class ZooKeeper {
             }
         }
 
-        private boolean removeWatches(Map<String, Set<Watcher>> pathVsWatcher,
+        protected boolean removeWatches(Map<String, Set<Watcher>> pathVsWatcher,
                 Watcher watcher, String path, boolean local, int rc,
                 Set<Watcher> removedWatchers) throws KeeperException {
             if (!local && rc != Code.OK.intValue()) {
@@ -707,6 +707,7 @@ public class ZooKeeper {
         LOG.info("Initiating client connection, connectString=" + connectString
                 + " sessionTimeout=" + sessionTimeout + " watcher=" + watcher);
 
+        watchManager = defaultWatchManager();
         watchManager.defaultWatcher = watcher;
 
         ConnectStringParser connectStringParser = new ConnectStringParser(
@@ -849,6 +850,7 @@ public class ZooKeeper {
                 + " sessionPasswd="
                 + (sessionPasswd == null ? "<null>" : "<hidden>"));
 
+        watchManager = defaultWatchManager();
         watchManager.defaultWatcher = watcher;
        
         ConnectStringParser connectStringParser = new ConnectStringParser(
@@ -867,6 +869,11 @@ public class ZooKeeper {
         return new ZooKeeperTestable(this, cnxn);
     }
 
+    /* Useful for testing watch handling behavior */
+    protected ZKWatchManager defaultWatchManager() {
+        return new ZKWatchManager();
+    }
+
     /**
      * The session id for this ZooKeeper client instance. The value returned is
      * not valid until the client connects to a server and may change after a

src/java/main/org/apache/zookeeper/server/DataTree.java

@@ -1365,18 +1365,26 @@ public class DataTree {
         }
     }
 
-    public void removeWatch(String path, WatcherType type, Watcher watcher) {
+    public boolean removeWatch(String path, WatcherType type, Watcher watcher) {
+        boolean removed = false;
+
         switch (type) {
         case Children:
-            this.childWatches.removeWatcher(path, watcher);
+            removed = this.childWatches.removeWatcher(path, watcher);
             break;
         case Data:
-            this.dataWatches.removeWatcher(path, watcher);
+            removed = this.dataWatches.removeWatcher(path, watcher);
             break;
         case Any:
-            this.childWatches.removeWatcher(path, watcher);
-            this.dataWatches.removeWatcher(path, watcher);
+            if (this.childWatches.removeWatcher(path, watcher)) {
+                removed = true;
+            }
+            if (this.dataWatches.removeWatcher(path, watcher)) {
+                removed = true;
+            }
             break;
         }
+
+        return removed;
     }
 }

src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java

@@ -21,6 +21,7 @@ package org.apache.zookeeper.server;
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.util.List;
+import java.util.Locale;
 
 import org.apache.jute.Record;
 import org.slf4j.Logger;
@@ -399,8 +400,15 @@ public class FinalRequestProcessor implements RequestProcessor {
                 ByteBufferInputStream.byteBuffer2Record(request.request,
                         removeWatches);
                 WatcherType type = WatcherType.fromInt(removeWatches.getType());
-                zks.getZKDatabase().removeWatch(removeWatches.getPath(), type,
-                        cnxn);
+                boolean removed = zks.getZKDatabase().removeWatch(
+                        removeWatches.getPath(), type, cnxn);
+
+                if (!removed) {
+                    String msg = String.format(Locale.ENGLISH, "%s (type: %s)",
+                            new Object[] { removeWatches.getPath(), type });
+                    throw new KeeperException.NoWatcherException(msg);
+                }
+
                 break;
             }
             }

src/java/main/org/apache/zookeeper/server/WatchManager.java

@@ -169,19 +169,21 @@ class WatchManager {
         }
     }
 
-    synchronized void removeWatcher(String path, Watcher watcher) {
+    synchronized boolean removeWatcher(String path, Watcher watcher) {
         HashSet<String> paths = watch2Paths.get(watcher);
-        if (paths == null) {
-            return;
+        if (paths == null || !paths.remove(path)) {
+            return false;
         }
-        paths.remove(path);
+
         HashSet<Watcher> list = watchTable.get(path);
-        if (list == null) {
-            return;
+        if (list == null || !list.remove(watcher)) {
+            return false;
         }
-        list.remove(watcher);
+
         if (list.size() == 0) {
             watchTable.remove(path);
         }
+
+        return true;
     }
 }

src/java/main/org/apache/zookeeper/server/ZKDatabase.java

@@ -597,7 +597,7 @@ public class ZKDatabase {
      * @param watcher
      *            watcher function to remove
      */
-    public void removeWatch(String path, WatcherType type, Watcher watcher) {
-        dataTree.removeWatch(path, type, watcher);
+    public boolean removeWatch(String path, WatcherType type, Watcher watcher) {
+        return dataTree.removeWatch(path, type, watcher);
     }
 }

src/java/test/org/apache/zookeeper/RemoveWatchesTest.java

@@ -23,10 +23,14 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeoutException;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.zookeeper.KeeperException.Code;
+import org.apache.zookeeper.KeeperException.NoWatcherException;
 import org.apache.zookeeper.Watcher.Event.EventType;
 import org.apache.zookeeper.Watcher.WatcherType;
 import org.apache.zookeeper.ZooDefs.Ids;
@@ -691,6 +695,87 @@ public class RemoveWatchesTest extends ClientBase {
         Assert.assertFalse("Shouldn't remove data watcher", w2.matches());
     }
 
+    /**
+     * Verify that if a given watcher doesn't exist, the server properly
+     * returns an error code for it.
+     *
+     * In our Java client implementation, we check that a given watch exists at
+     * two points:
+     *
+     * 1) before submitting the RemoveWatches request
+     * 2) after a successful server response, when the watcher needs to be
+     *    removed
+     *
+     * Since this can be racy (i.e. a watch can fire while a RemoveWatches
+     * request is in-flight), we need to verify that the watch was actually
+     * removed (i.e. from ZKDatabase and DataTree) and return NOWATCHER if
+     * needed.
+     *
+     * Also, other implementations might not do a client side check before
+     * submitting a RemoveWatches request. If we don't do a server side check,
+     * we would just return ZOK even if no watch was removed.
+     *
+     */
+    @Test(timeout = 90000)
+    public void testNoWatcherServerException()
+            throws InterruptedException, IOException, TimeoutException {
+        CountdownWatcher watcher = new CountdownWatcher();
+        MyZooKeeper zk = new MyZooKeeper(hostPort, CONNECTION_TIMEOUT, watcher);
+        boolean nw = false;
+
+        watcher.waitForConnected(CONNECTION_TIMEOUT);
+
+        try {
+            zk.removeWatches("/nowatchhere", null, WatcherType.Data, false);
+        } catch (KeeperException nwe) {
+            if (nwe.code().intValue() == Code.NOWATCHER.intValue()) {
+                nw = true;
+            }
+        }
+
+        Assert.assertTrue("Server didn't return NOWATCHER",
+                zk.getRemoveWatchesRC() == Code.NOWATCHER.intValue());
+        Assert.assertTrue("NoWatcherException didn't happen", nw);
+    }
+
+    /* a mocked ZK class that doesn't do client-side verification
+     * before/after calling removeWatches */
+    private class MyZooKeeper extends ZooKeeper {
+        class MyWatchManager extends ZKWatchManager {
+            public int lastrc;
+
+            /* Pretend that any watcher exists */
+            void containsWatcher(String path, Watcher watcher,
+                    WatcherType watcherType) throws NoWatcherException {
+            }
+
+            /* save the return error code by the server */
+            protected boolean removeWatches(
+                Map<String, Set<Watcher>> pathVsWatcher,
+                Watcher watcher, String path, boolean local, int rc,
+                Set<Watcher> removedWatchers) throws KeeperException {
+                lastrc = rc;
+                return false;
+            }
+        }
+
+        public MyZooKeeper(String hp, int timeout, Watcher watcher)
+                throws IOException {
+            super(hp, timeout, watcher, false);
+        }
+
+        private MyWatchManager myWatchManager;
+
+        protected ZKWatchManager defaultWatchManager() {
+            myWatchManager = new MyWatchManager();
+            return myWatchManager;
+        }
+
+        public int getRemoveWatchesRC() {
+            return myWatchManager.lastrc;
+        }
+    }
+
     private class MyWatcher implements Watcher {
         private final String path;
         private String eventPath;