Fix garbage collection bug (#555)

Make it traverse the whole tree including tombstones
yorkie-team · Jun 29, 2023 · 4bebe31 · 4bebe31
1 parent e9aec8f
commit 4bebe31
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 1 deletion.
diff --git a/src/document/crdt/tree.ts b/src/document/crdt/tree.ts
@@ -663,7 +663,7 @@ export class CRDTTree extends CRDTGCElement {
       }
     }
 
-    this.indexTree.traverse((treeNode) => {
+    this.indexTree.traverseAll((treeNode) => {
       if (nodesToRemoved.has(treeNode)) {
         const parent = treeNode.parent;
 

diff --git a/src/util/index_tree.ts b/src/util/index_tree.ts
@@ -523,6 +523,20 @@ export function traverse<T extends IndexTreeNode<T>>(
   callback(node, depth);
 }
 
+/**
+ * `traverseAll` traverses the whole tree (include tombstones) with postorder traversal.
+ */
+function traverseAll<T extends IndexTreeNode<T>>(
+  node: T,
+  callback: (node: T, depth: number) => void,
+  depth = 0,
+) {
+  for (const child of node._children) {
+    traverseAll(child, callback, depth + 1);
+  }
+  callback(node, depth);
+}
+
 /**
  * `findTreePos` finds the position of the given index in the given node.
  */
@@ -675,6 +689,13 @@ export class IndexTree<T extends IndexTreeNode<T>> {
     traverse(this.root, callback, 0);
   }
 
+  /**
+   * `traverseAll` traverses the whole tree (include tombstones) with postorder traversal.
+   */
+  traverseAll(callback: (node: T) => void): void {
+    traverseAll(this.root, callback, 0);
+  }
+
   /**
    * `split` splits the node at the given index.
    */

diff --git a/test/integration/gc_test.ts b/test/integration/gc_test.ts
@@ -7,6 +7,23 @@ import {
   toDocKey,
 } from '@yorkie-js-sdk/test/integration/integration_helper';
 import { Text } from '@yorkie-js-sdk/src/yorkie';
+import { CRDTTreeNode } from '@yorkie-js-sdk/src/document/crdt/tree';
+import { IndexTreeNode } from '@yorkie-js-sdk/src/util/index_tree';
+
+// `getNodeLength` returns the number of nodes in the given tree.
+function getNodeLength(root: IndexTreeNode<CRDTTreeNode>) {
+  let size = 0;
+
+  size += root._children.length;
+
+  if (root._children.length) {
+    root._children.forEach((child) => {
+      size += getNodeLength(child);
+    });
+  }
+
+  return size;
+}
 
 describe('Garbage Collection', function () {
   it('garbage collection test', function () {
@@ -204,19 +221,31 @@ describe('Garbage Collection', function () {
     });
 
     // [text(a), text(b)]
+    let nodeLengthBeforeGC = getNodeLength(
+      doc.getRoot().t.getIndexTree().getRoot(),
+    );
     assert.equal(doc.getGarbageLen(), 2);
     assert.equal(doc.garbageCollect(MaxTimeTicket), 2);
     assert.equal(doc.getGarbageLen(), 0);
+    let nodeLengthAfterGC = getNodeLength(
+      doc.getRoot().t.getIndexTree().getRoot(),
+    );
+    assert.equal(nodeLengthBeforeGC - nodeLengthAfterGC, 1);
 
     doc.update((root) => {
       root.t.editByPath([0, 0, 0], [0, 0, 2], { type: 'text', value: 'cv' });
       assert.equal(root.t.toXML(), `<doc><p><tn>cv</tn><tn>cd</tn></p></doc>`);
     });
 
     // [text(cd)]
+    nodeLengthBeforeGC = getNodeLength(
+      doc.getRoot().t.getIndexTree().getRoot(),
+    );
     assert.equal(doc.getGarbageLen(), 1);
     assert.equal(doc.garbageCollect(MaxTimeTicket), 1);
     assert.equal(doc.getGarbageLen(), 0);
+    nodeLengthAfterGC = getNodeLength(doc.getRoot().t.getIndexTree().getRoot());
+    assert.equal(nodeLengthBeforeGC - nodeLengthAfterGC, 1);
 
     doc.update((root) => {
       root.t.editByPath([0], [1], {
@@ -227,9 +256,14 @@ describe('Garbage Collection', function () {
     });
 
     // [p, tn, tn, text(cv), text(cd)]
+    nodeLengthBeforeGC = getNodeLength(
+      doc.getRoot().t.getIndexTree().getRoot(),
+    );
     assert.equal(doc.getGarbageLen(), 5);
     assert.equal(doc.garbageCollect(MaxTimeTicket), 5);
     assert.equal(doc.getGarbageLen(), 0);
+    nodeLengthAfterGC = getNodeLength(doc.getRoot().t.getIndexTree().getRoot());
+    assert.equal(nodeLengthBeforeGC - nodeLengthAfterGC, 6);
   });
 
   it('Can handle tree garbage collection for multi client', async function () {