fixes decoder refId garbage collection when calling .clear(). #75

colyseus · Jul 14, 2020 · 6cd1693 · 6cd1693
1 parent c239aa6
commit 6cd1693
Show file tree

Hide file tree

Showing 7 changed files with 71 additions and 16 deletions.
diff --git a/src/Schema.ts b/src/Schema.ts
@@ -25,7 +25,7 @@ export interface SchemaDecoderCallbacks {
     onAdd?: (item: any, key: any) => void;
     onRemove?: (item: any, key: any) => void;
     onChange?: (item: any, key: any) => void;
-    clear();
+    clear(decoding?: boolean);
     decode?(byte, it: decode.Iterator);
 }
 
@@ -166,9 +166,6 @@ export abstract class Schema {
     public assign(
         props: { [prop in NonFunctionPropNames<this>]?: this[prop] }
     ) {
-        //
-        // TODO: recursivelly assign child Schema structures.
-        //
         Object.assign(this, props);
         return this;
     }
@@ -247,14 +244,11 @@ export abstract class Schema {
 
             if (operation === OPERATION.CLEAR) {
                 //
-                // TODO:
-                //
-                // flag all children refId's for garbage collection.
-                // (if not a collection of primitive type)
-                //
-                // $root.removeRef(refId);
+                // TODO: refactor me!
+                // The `.clear()` method is calling `$root.removeRef(refId)` is
+                // being called for each item inside this collection
                 //
-                (ref as SchemaDecoderCallbacks).clear();
+                (ref as SchemaDecoderCallbacks).clear(true);
                 continue;
             }
 

diff --git a/src/changes/ChangeTree.ts b/src/changes/ChangeTree.ts
@@ -221,7 +221,7 @@ export class ChangeTree {
         }
     }
 
-    getType(index: number) {
+    getType(index?: number) {
         if (this.ref['_definition']) {
             const definition = (this.ref as Schema)['_definition'];
             return definition.schema[ definition.fieldsByIndex[index] ];

diff --git a/src/types/ArraySchema.ts b/src/types/ArraySchema.ts
@@ -166,13 +166,20 @@ export class ArraySchema<V=any> implements Array<V>, SchemaDecoderCallbacks {
         return this.$items.delete(index);
     }
 
-    clear() {
+    clear(isDecoding?: boolean) {
         // discard previous operations.
         this.$changes.discard(true);
 
         // clear previous indexes
         this.$indexes.clear();
 
+        // flag child items for garbage collection.
+        if (isDecoding && typeof (this.$changes.getType()) !== "string") {
+            this.$items.forEach((item: V) => {
+                this.$changes.root.removeRef(item['$changes'].refId);
+            });
+        }
+
         // clear items
         this.$items.clear();
 

diff --git a/src/types/CollectionSchema.ts b/src/types/CollectionSchema.ts
@@ -77,13 +77,20 @@ export class CollectionSchema<V=any> implements SchemaDecoderCallbacks {
         return this.$items.delete(index);
     }
 
-    clear() {
+    clear(isDecoding?: boolean) {
         // discard previous operations.
         this.$changes.discard(true);
 
         // clear previous indexes
         this.$indexes.clear();
 
+        // flag child items for garbage collection.
+        if (isDecoding && typeof (this.$changes.getType()) !== "string") {
+            this.$items.forEach((item: V) => {
+                this.$changes.root.removeRef(item['$changes'].refId);
+            });
+        }
+
         // clear items
         this.$items.clear();
 

diff --git a/src/types/MapSchema.ts b/src/types/MapSchema.ts
@@ -133,13 +133,20 @@ export class MapSchema<V=any> implements Map<string, V>, SchemaDecoderCallbacks
         return this.$items.delete(key);
     }
 
-    clear() {
+    clear(isDecoding?: boolean) {
         // discard previous operations.
         this.$changes.discard(true);
 
         // clear previous indexes
         this.$indexes.clear();
 
+        // flag child items for garbage collection.
+        if (isDecoding && typeof (this.$changes.getType()) !== "string") {
+            this.$items.forEach((item: V) => {
+                this.$changes.root.removeRef(item['$changes'].refId);
+            });
+        }
+
         // clear items
         this.$items.clear();
 

diff --git a/src/types/SetSchema.ts b/src/types/SetSchema.ts
@@ -76,13 +76,20 @@ export class SetSchema<V=any> implements SchemaDecoderCallbacks {
         return this.$items.delete(index);
     }
 
-    clear() {
+    clear(isDecoding?: boolean) {
         // discard previous operations.
         this.$changes.discard(true);
 
         // clear previous indexes
         this.$indexes.clear();
 
+        // flag child items for garbage collection.
+        if (isDecoding && typeof (this.$changes.getType()) !== "string") {
+            this.$items.forEach((item: V) => {
+                this.$changes.root.removeRef(item['$changes'].refId);
+            });
+        }
+
         // clear items
         this.$items.clear();
 

diff --git a/test/InstanceSharingTest.ts b/test/InstanceSharingTest.ts
@@ -132,4 +132,37 @@ describe("Instance sharing", () => {
         const newRefCount = decodedState['$changes'].root.refs.size;
         assert.equal(refCount - 4, newRefCount);
     });
+
+    it("clearing ArraySchema", () => {
+        const state = new State();
+
+        const player1 = new Player().assign({
+            position: new Position().assign({
+                x: 10, y: 10
+            })
+        });
+        state.arrayOfPlayers.push(player1);
+        state.arrayOfPlayers.push(player1);
+        state.arrayOfPlayers.push(player1);
+
+        const player2 = new Player().assign({
+            position: new Position().assign({
+                x: 10, y: 10
+            })
+        });
+        state.arrayOfPlayers.push(player2);
+
+        const decodedState = new State();
+        decodedState.decode(state.encode());
+
+        const refCount = decodedState['$changes'].root.refs.size;
+        assert.equal(7, refCount);
+
+        state.arrayOfPlayers.clear();
+
+        decodedState.decode(state.encode());
+
+        const newRefCount = decodedState['$changes'].root.refs.size;
+        assert.equal(refCount - 4, newRefCount);
+    });
 });