Skip to content

Commit 400d217

Browse files
endelendel
committed
Decoder: fix garbage collection of nested Schema instances
1 parent 273baf9 commit 400d217

File tree

6 files changed

+131
-11
lines changed

6 files changed

+131
-11
lines changed

‎package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@colyseus/schema",
3-
"version": "3.0.21",
3+
"version": "3.0.22",
44
"description": "Binary state serializer with delta encoding for games",
55
"bin": {
66
"schema-codegen": "./bin/schema-codegen",

‎src/decoder/ReferenceTracker.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ export class ReferenceTracker {
9898
//
9999
// Ensure child schema instances have their references removed as well.
100100
//
101-
if (Metadata.isValidInstance(ref)) {
101+
if (ref.constructor[Symbol.metadata] !== undefined) {
102102
const metadata: Metadata = ref.constructor[Symbol.metadata];
103103
for (const index in metadata) {
104104
const field = metadata[index as any as number].name;
@@ -109,7 +109,7 @@ export class ReferenceTracker {
109109
}
110110

111111
} else {
112-
if (typeof (Object.values(ref[$childType])[0]) === "function") {
112+
if (typeof (ref[$childType]) === "function") {
113113
Array.from((ref as MapSchema).values())
114114
.forEach((child) => {
115115
const childRefId = this.refIds.get(child);

‎src/encoder/Encoder.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,14 +211,15 @@ export class Encoder<T extends Schema = any> {
211211

212212
if (changeTree === undefined) {
213213
// detached instance, remove from view and skip.
214+
// console.log("detached instance, remove from view and skip.", refId);
214215
view.changes.delete(refId);
215216
continue;
216217
}
217218

218219
const keys = Object.keys(changes);
219220
if (keys.length === 0) {
220221
// FIXME: avoid having empty changes if no changes were made
221-
// console.log("changes.size === 0, skip", changeTree.ref.constructor.name);
222+
// console.log("changes.size === 0, skip", refId, changeTree.ref.constructor.name);
222223
continue;
223224
}
224225

‎src/encoder/StateView.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export class StateView {
2929

3030
// TODO: allow to set multiple tags at once
3131
add(obj: Ref, tag: number = DEFAULT_VIEW_TAG, checkIncludeParent: boolean = true) {
32-
if (!obj[$changes]) {
32+
if (!obj?.[$changes]) {
3333
console.warn("StateView#add(), invalid object:", obj);
3434
return this;
3535
}
@@ -163,7 +163,7 @@ export class StateView {
163163
}
164164

165165
remove(obj: Ref, tag: number = DEFAULT_VIEW_TAG) {
166-
const changeTree = obj[$changes];
166+
const changeTree: ChangeTree = obj[$changes];
167167
if (!changeTree) {
168168
console.warn("StateView#remove(), invalid object:", obj);
169169
return this;
@@ -172,7 +172,7 @@ export class StateView {
172172
this.items.delete(changeTree);
173173

174174
const ref = changeTree.ref;
175-
const metadata: Metadata = ref.constructor[Symbol.metadata];
175+
const metadata: Metadata = ref.constructor[Symbol.metadata]; // ArraySchema/MapSchema do not have metadata
176176

177177
let changes = this.changes.get(changeTree.refId);
178178
if (changes === undefined) {
@@ -195,14 +195,14 @@ export class StateView {
195195

196196
} else {
197197
// delete all "tagged" properties.
198-
metadata[$viewFieldIndexes].forEach((index) =>
198+
metadata?.[$viewFieldIndexes].forEach((index) =>
199199
changes[index] = OPERATION.DELETE);
200200
}
201201

202202

203203
} else {
204204
// delete only tagged properties
205-
metadata[$fieldIndexesByViewTag][tag].forEach((index) =>
205+
metadata?.[$fieldIndexesByViewTag][tag].forEach((index) =>
206206
changes[index] = OPERATION.DELETE);
207207
}
208208

‎test/Schema.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,19 +79,22 @@ Schema.prototype.encodeAll = function() {
7979
return getEncoder(this).encodeAll();
8080
}
8181

82-
export interface ClientWithState<T> {
82+
export interface ClientWithState<T extends Schema> {
8383
state: T;
8484
view: StateView;
85+
decoder: Decoder<T>;
8586
$: SchemaCallbackProxy<T>;
8687
needFullEncode: boolean;
8788
}
8889

8990
export function createClientWithView<T extends Schema>(from: T, stateView: StateView = new StateView(), encoder?: Encoder): ClientWithState<T> {
9091
const state = createInstanceFromReflection(from, encoder);
92+
const decoder = getDecoder(state);
9193
return {
9294
state,
9395
view: stateView,
94-
$: getDecoderStateCallbacks(getDecoder(state)),
96+
decoder,
97+
$: getDecoderStateCallbacks(decoder),
9598
needFullEncode: true,
9699
};
97100
}

‎test/StateView.test.ts

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -627,6 +627,38 @@ describe("StateView", () => {
627627
assert.strictEqual(undefined, client2.state.entities);
628628
});
629629

630+
it("removing a single item from the view", () => {
631+
class Item extends Schema {
632+
@type("number") amount: number;
633+
}
634+
635+
class State extends Schema {
636+
@view() @type({ map: Item }) items = new MapSchema<Item>();
637+
}
638+
639+
const state = new State();
640+
const encoder = getEncoder(state);
641+
642+
for (let i = 0; i < 5; i++) {
643+
state.items.set(i.toString(), new Item().assign({ amount: i }));
644+
}
645+
646+
const client1 = createClientWithView(state);
647+
encodeMultiple(encoder, state, [client1]);
648+
649+
client1.view.add(state.items.get("1"));
650+
client1.view.add(state.items.get("2"));
651+
client1.view.add(state.items.get("3"));
652+
encodeMultiple(encoder, state, [client1]);
653+
654+
assert.strictEqual(client1.state.items.size, 3);
655+
656+
client1.view.remove(state.items.get("2"));
657+
658+
encodeMultiple(encoder, state, [client1]);
659+
assert.strictEqual(client1.state.items.size, 2);
660+
});
661+
630662
it("adding to view item that has been removed from state", () => {
631663
class Item extends Schema {
632664
@type("number") amount: number;
@@ -659,6 +691,90 @@ describe("StateView", () => {
659691
assert.strictEqual(client1.state.items.size, 0);
660692

661693
});
694+
695+
it("should support add + remove + add of complex items", () => {
696+
class Component extends Schema {
697+
@type("string") name: string;
698+
@type("number") value: number;
699+
}
700+
701+
class ListComponent extends Component {
702+
@type(["string"]) list = new ArraySchema<string>();
703+
}
704+
705+
class TagComponent extends Component {
706+
@type("string") tag: string;
707+
}
708+
709+
class Entity extends Schema {
710+
@type("string") id: string = nanoid(9);
711+
@type([Component]) components = new ArraySchema<Component>();
712+
}
713+
714+
class MyRoomState extends Schema {
715+
@view() @type({ map: Entity }) entities = new Map<string, Entity>();
716+
}
717+
718+
const state = new MyRoomState();
719+
const encoder = getEncoder(state);
720+
721+
const client1 = createClientWithView(state);
722+
const client2 = createClientWithView(state);
723+
724+
state.entities.set("one", new Entity().assign({
725+
id: "one",
726+
components: [
727+
new Component().assign({ name: "Health", value: 100 }),
728+
new ListComponent().assign({ name: "List", value: 200, list: ["one", "two"] }),
729+
new TagComponent().assign({ name: "Tag", value: 300, tag: "tag" }),
730+
]
731+
}));
732+
733+
state.entities.set("two", new Entity().assign({
734+
id: "two",
735+
components: [
736+
new Component().assign({ name: "Health", value: 100 }),
737+
new ListComponent().assign({ name: "List", value: 200, list: ["one", "two"] }),
738+
new TagComponent().assign({ name: "Tag", value: 300, tag: "tag" }),
739+
]
740+
}));
741+
742+
encodeMultiple(encoder, state, [client1, client2]);
743+
744+
// add entities for the first time
745+
client1.view.add(state.entities.get("one"));
746+
client2.view.add(state.entities.get("two"));
747+
748+
encodeMultiple(encoder, state, [client1, client2]);
749+
750+
assert.strictEqual(1, client1.state.entities.size);
751+
assert.strictEqual(3, client1.state.entities.get("one").components.length);
752+
753+
assert.strictEqual(1, client2.state.entities.size);
754+
assert.strictEqual(3, client2.state.entities.get("two").components.length);
755+
756+
// remove entity from state view
757+
client1.view.remove(state.entities.get("one"));
758+
client2.view.remove(state.entities.get("two"));
759+
760+
encodeMultiple(encoder, state, [client1, client2]);
761+
762+
assert.strictEqual(0, client1.state.entities.size);
763+
assert.strictEqual(0, client2.state.entities.size);
764+
765+
// re-add entities!
766+
client1.view.add(state.entities.get("one"));
767+
client2.view.add(state.entities.get("two"));
768+
769+
encodeMultiple(encoder, state, [client1, client2]);
770+
771+
assert.strictEqual(1, client1.state.entities.size);
772+
assert.strictEqual(3, client1.state.entities.get("one").components.length);
773+
774+
assert.strictEqual(1, client2.state.entities.size);
775+
assert.strictEqual(3, client2.state.entities.get("two").components.length);
776+
777+
});
662778
});
663779

664780
describe("ArraySchema", () => {

0 commit comments

Comments
 (0)