diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java index fa0c53a4257..2e3c5838cfe 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java @@ -21,9 +21,11 @@ import java.io.EOFException; import java.io.IOException; import java.io.PrintWriter; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Deque; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; @@ -1318,29 +1320,44 @@ private void setupQuota() { * @throws IOException */ void serializeNode(OutputArchive oa, StringBuilder path) throws IOException { - String pathString = path.toString(); - DataNode node = getNode(pathString); - if (node == null) { - return; - } - String[] children; - DataNode nodeCopy; - synchronized (node) { - StatPersisted statCopy = new StatPersisted(); - statCopy.copyFrom(node.stat); - //we do not need to make a copy of node.data because the contents - //are never changed - nodeCopy = new DataNode(node.data, node.acl, statCopy); - children = node.getChildren().toArray(new String[0]); - } - serializeNodeData(oa, pathString, nodeCopy); - path.append('/'); - int off = path.length(); - for (String child : children) { - // Since this is single buffer being reused, we need to truncate the previous bytes of string. - path.delete(off, Integer.MAX_VALUE); - path.append(child); - serializeNode(oa, path); + // Iterative, explicit-stack pre-order traversal of the subtree rooted + // at `path`. This was previously written as recursion with one frame + // per znode level, which does not scale to very deep trees; walking the + // tree with a heap-allocated stack removes the dependency on tree depth. + // + // The traversal order (pre-order, children in the same order as + // before), the per-node locking, and the emitted records are unchanged, + // so the produced snapshot bytes are identical and the on-disk format is + // unchanged. The explicit stack holds at most the unvisited frontier of + // the tree (a single entry for a chain), all of which the in-memory + // DataTree already holds. + Deque pathStack = new ArrayDeque<>(); + pathStack.push(path.toString()); + while (!pathStack.isEmpty()) { + String pathString = pathStack.pop(); + DataNode node = getNode(pathString); + if (node == null) { + continue; + } + String[] children; + DataNode nodeCopy; + synchronized (node) { + StatPersisted statCopy = new StatPersisted(); + statCopy.copyFrom(node.stat); + //we do not need to make a copy of node.data because the contents + //are never changed + nodeCopy = new DataNode(node.data, node.acl, statCopy); + children = node.getChildren().toArray(new String[0]); + } + // Called outside the node lock: writeRecord can block and holding + // the lock would stall writers (ZOOKEEPER-2201). + serializeNodeData(oa, pathString, nodeCopy); + // Push children in reverse so that, popped from this LIFO stack, + // they are visited in the same order as the previous recursion. + String childPathPrefix = pathString + "/"; + for (int i = children.length - 1; i >= 0; i--) { + pathStack.push(childPathPrefix + children[i]); + } } } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java index 57f8de41716..0069dddfc43 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java @@ -418,6 +418,47 @@ public void run() { assertTrue(ranTestCase.get(), "Didn't find the expected node"); } + /** + * Snapshot serialization must not depend on tree depth. This builds a very + * deep znode chain and serializes it on a thread with a small (512 KiB) + * stack, so the test is independent of the runner's default -Xss, then + * checks the result round-trips back to an equivalent tree. + */ + @Test + @Timeout(value = 120) + public void testSerializeDeepTree() throws Exception { + final int depth = 4000; + final DataTree tree = new DataTree(); + final StringBuilder path = new StringBuilder(); + for (int i = 0; i < depth; i++) { + path.append("/a"); + tree.createNode(path.toString(), new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, -1L, i + 1, i + 1, 1); + } + + final ByteArrayOutputStream baos = new ByteArrayOutputStream(); + final BinaryOutputArchive oa = BinaryOutputArchive.getArchive(baos); + final Throwable[] failure = new Throwable[1]; + final Thread serializer = new Thread(null, () -> { + try { + tree.serialize(oa, "tree"); + } catch (Throwable t) { + failure[0] = t; + } + }, "deep-tree-serializer", 512 * 1024L); + serializer.start(); + serializer.join(); + + assertNull(failure[0], "serialize() must not fail on a deep tree, but threw: " + failure[0]); + + // The produced bytes must load back into an equivalent tree. + final DataTree restored = new DataTree(); + final BinaryInputArchive ia = + BinaryInputArchive.getArchive(new ByteArrayInputStream(baos.toByteArray())); + restored.deserialize(ia, "tree"); + assertNotNull(restored.getNode(path.toString()), + "the deepest node must survive a serialize/deserialize round-trip"); + } + @Test @Timeout(value = 60) public void testReconfigACLClearOnDeserialize() throws Exception {