Uploaded image for project: 'Apache Curator'
  1. Apache Curator
  2. CURATOR-430

deletingChildrenIfNeeded maybe cannot delete children completely when multi-client delete concurrently

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Client
    • Labels:
      None

      Description

      use curatorFramework.delete().deletingChildrenIfNeeded().forPath(path), this sync api doesn't ignore the NoNodeException, causes the rest of children nodes will not be deleted perhaps.

      zookeeper.getChildren(path, null) maybe throw NoNodeException, if the path doesn't exist.

        Issue Links

          Activity

          Hide
          randgalt Jordan Zimmerman added a comment -

          Do you have a test that shows this? Internally, it calls ZKPaths.deleteChildren() which ignores KeeperException.NoNodeException.

          Show
          randgalt Jordan Zimmerman added a comment - Do you have a test that shows this? Internally, it calls ZKPaths.deleteChildren() which ignores KeeperException.NoNodeException.
          Hide
          hebelala hebelala added a comment -

          Yes, I have test. Please see the code, and debug it.
          The debug breakpoints are step1, step2, and ZkPaths.deleteChildren -> List<String> children = zookeeper.getChildren(path, null);

          public static void main(String[] args) {
                  try {
          
                      TestingServer server = new TestingServer();
                      server.start();
          
                      final CuratorFramework curatorFramework = CuratorFrameworkFactory.builder().connectString(server.getConnectString()).sessionTimeoutMs(600 * 1000).retryPolicy(new RetryNTimes(3, 1000)).build();
                      curatorFramework.start();
                      curatorFramework.blockUntilConnected();
          
                      curatorFramework.create().creatingParentsIfNeeded().forPath("/a/1");
                      curatorFramework.create().creatingParentsIfNeeded().forPath("/a/2");
          
                      // try to delete /a
                      new Thread(new Runnable() {
                          public void run() {
                              try {
                                  // step1
                                  // deleting sequence is: /a/2, /a/1, /a
                                  curatorFramework.delete().deletingChildrenIfNeeded().forPath("/a");
                              } catch (Exception e) {
                                  e.printStackTrace();
                                  try {
                                      // the /a/1 is existing, because step2 delete /a/2, causes step1 throw NoNodeException(/a/2).  It's not expected.
                                      Stat stat = curatorFramework.checkExists().forPath("/a/1");
                                      System.out.println("checkExits:" + stat);
                                  } catch (Exception e1) {
                                      e1.printStackTrace();
                                  }
                              }
                          }
                      }).start();
          
                      Thread.sleep(1000L);
                      // step2
                      // delete /a/2, when step1 try to delete /a/2 and is going to zookeeper.getChildren("/a/2", null)
                      curatorFramework.delete().forPath("/a/2");
          
                      System.out.println("ok");
          
                      Thread.sleep(1000L);
          
                  } catch (Exception e) {
                      e.printStackTrace();
                  } finally {
                      System.exit(0);
                  }
          
              }
          
          Show
          hebelala hebelala added a comment - Yes, I have test. Please see the code, and debug it. The debug breakpoints are step1 , step2 , and ZkPaths.deleteChildren -> List<String> children = zookeeper.getChildren(path, null); public static void main( String [] args) { try { TestingServer server = new TestingServer(); server.start(); final CuratorFramework curatorFramework = CuratorFrameworkFactory.builder().connectString(server.getConnectString()).sessionTimeoutMs(600 * 1000).retryPolicy( new RetryNTimes(3, 1000)).build(); curatorFramework.start(); curatorFramework.blockUntilConnected(); curatorFramework.create().creatingParentsIfNeeded().forPath( "/a/1" ); curatorFramework.create().creatingParentsIfNeeded().forPath( "/a/2" ); // try to delete /a new Thread ( new Runnable () { public void run() { try { // step1 // deleting sequence is: /a/2, /a/1, /a curatorFramework.delete().deletingChildrenIfNeeded().forPath( "/a" ); } catch (Exception e) { e.printStackTrace(); try { // the /a/1 is existing, because step2 delete /a/2, causes step1 throw NoNodeException(/a/2). It's not expected. Stat stat = curatorFramework.checkExists().forPath( "/a/1" ); System .out.println( "checkExits:" + stat); } catch (Exception e1) { e1.printStackTrace(); } } } }).start(); Thread .sleep(1000L); // step2 // delete /a/2, when step1 try to delete /a/2 and is going to zookeeper.getChildren( "/a/2" , null ) curatorFramework.delete().forPath( "/a/2" ); System .out.println( "ok" ); Thread .sleep(1000L); } catch (Exception e) { e.printStackTrace(); } finally { System .exit(0); } }
          Show
          hebelala hebelala added a comment - https://github.com/apache/curator/pull/235
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Randgalt commented on the issue:

          https://github.com/apache/curator/pull/235

          Please include a test (something like the code you posted in the issue) for this. A good place to put the test is TestFrameworkEdges.java.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/235 Please include a test (something like the code you posted in the issue) for this. A good place to put the test is TestFrameworkEdges.java.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lvfangmin commented on a diff in the pull request:

          https://github.com/apache/curator/pull/235#discussion_r133328503

          — Diff: curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java —
          @@ -313,7 +313,13 @@ public static void deleteChildren(ZooKeeper zookeeper, String path, boolean dele
          {
          PathUtils.validatePath(path);

          • List<String> children = zookeeper.getChildren(path, null);
            + List<String> children;
            + try { + children = zookeeper.getChildren(path, null); + }

            catch (KeeperException.NoNodeException e) {
            + // someone else has deleted the node it since we checked

              • End diff –

          // someone else has deleted the node since we checked

          Show
          githubbot ASF GitHub Bot added a comment - Github user lvfangmin commented on a diff in the pull request: https://github.com/apache/curator/pull/235#discussion_r133328503 — Diff: curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java — @@ -313,7 +313,13 @@ public static void deleteChildren(ZooKeeper zookeeper, String path, boolean dele { PathUtils.validatePath(path); List<String> children = zookeeper.getChildren(path, null); + List<String> children; + try { + children = zookeeper.getChildren(path, null); + } catch (KeeperException.NoNodeException e) { + // someone else has deleted the node it since we checked End diff – // someone else has deleted the node since we checked
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hebelala commented on a diff in the pull request:

          https://github.com/apache/curator/pull/235#discussion_r133357174

          — Diff: curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java —
          @@ -313,7 +313,13 @@ public static void deleteChildren(ZooKeeper zookeeper, String path, boolean dele
          {
          PathUtils.validatePath(path);

          • List<String> children = zookeeper.getChildren(path, null);
            + List<String> children;
            + try { + children = zookeeper.getChildren(path, null); + }

            catch (KeeperException.NoNodeException e) {
            + // someone else has deleted the node it since we checked

              • End diff –

          haha... it's copied

          Show
          githubbot ASF GitHub Bot added a comment - Github user hebelala commented on a diff in the pull request: https://github.com/apache/curator/pull/235#discussion_r133357174 — Diff: curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java — @@ -313,7 +313,13 @@ public static void deleteChildren(ZooKeeper zookeeper, String path, boolean dele { PathUtils.validatePath(path); List<String> children = zookeeper.getChildren(path, null); + List<String> children; + try { + children = zookeeper.getChildren(path, null); + } catch (KeeperException.NoNodeException e) { + // someone else has deleted the node it since we checked End diff – haha... it's copied
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hebelala commented on the issue:

          https://github.com/apache/curator/pull/235

          Be sure this issue is bug? If yes, could you publish a fixed version based on v2.10.0?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hebelala commented on the issue: https://github.com/apache/curator/pull/235 Be sure this issue is bug? If yes, could you publish a fixed version based on v2.10.0?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user JunjianS commented on the issue:

          https://github.com/apache/curator/pull/235

          i met the same problem, could u please provide a fixed version ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user JunjianS commented on the issue: https://github.com/apache/curator/pull/235 i met the same problem, could u please provide a fixed version ?

            People

            • Assignee:
              Unassigned
              Reporter:
              hebelala hebelala
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development