StructureSpawn causing "circular structure" error on JSON.stringify()

[SemperRabbit]

I have seen multiple instances of folks asking for help in slack with a TypeError: Converting circular structure to JSON error that was resolved when they identified they had accidentally saved a spawn object to Memory. I decided to dig into it, and recreated the error only when a creep was being spawned.

It is being caused by .spawning object holding an instance of the spawn itself. Note that all of these commands were executed while a creep was spawning.

Object.getOwnPropertyNames(Game.getObjectById('5806f971aab76b20199db452'))
room,pos,id,_my,_name,_structureType,_spawning,_energy,_energyCapacity,_hits,_hitsMax

Object.getOwnPropertyNames(Game.getObjectById('5806f971aab76b20199db452')._spawning)
spawn,name,needTime,remainingTime,directions

Object.getOwnPropertyNames(Game.getObjectById('5806f971aab76b20199db452')._spawning.spawn)
room,pos,id,_my,_name,_structureType,_spawning,_energy,_energyCapacity,_hits,_hitsMax

Game.getObjectById('5806f971aab76b20199db452')._spawning.spawn.id
5806f971aab76b20199db452

Game.getObjectById('5806f971aab76b20199db452')._spawning.spawn === Game.getObjectById('5806f971aab76b20199db452')
true

I do not know how you would want to handle this, but an option I have thought of was overwriting StructureSpawn.prototype.toJSON to ensure that this does not occur. If you are ok with this option, I can work on a PR.

[artch]

Actually, I think we should disable stringifying live game objects at all, and replace all toJSON with something like {_id: 'abc123', _prototype: 'StructureSpawn'}. It would make it more obvious that serialized objects from Memory don't work as expected.

[SemperRabbit]

@artch is there any way to do that solely for the Memory stringify at the end of the tick? I know multiple people that use stringify for troubleshooting. I, personally, use @WarInternal's convenience function of global.ex = (x) => JSON.stringify(x, null, 2); for troubleshooting. I feel like a broad "no one gets to stringify," may not be the best option. I would be ok if it were a published option that stringify is disabled by default and an "enable stringify" would be an "advanced" option, but that would take additional setting stored/retrieved from the DB, and I know you have been wary performance-wise of accepting PRs that do so.

[Knightshade]

@artch So to be clear, will all of the old data still be accessible, but simply under a different name, or are you suggesting removing our ability to access the data entirely.

[WarInternal]

I believe we can reach a compromise on this. Allow game objects to be serialized still, maybe mark a few properties as non-enumerable to prevent the circular errors. But not all of them, we still want to be able to inspect an object on the console. Then for Memory serialize use the 2nd parameter of JSON.stringify, the replacer, to exclude game objects or throw an error that they have a game object in memory.

[artch]

@warinternal This sounds reasonable.

[SemperRabbit]

I think that would be a feasible solution, but I'm concerned about a couple of things:

1. Adding a replacer would add extra load on the backend (remember, this function would be run recursively on every object in every players' Memory, every tick)
2. Protecting players from themselves isn't the best option. The best way to learn is from failure. If you take away their ability to fail, they will never learn. Education in the tutorial and docs would be a better solution for aiding the newer players.

I still think that simply adding StructureSpawn.Spawning.prototype.toJSON to prevent the recursion would be the best bet. That will not add additional burden on the servers.

[artch]

@semperrabbit This spawning circular issue is just a small part of the problem. A much bigger issue is when a player stores live objects in Memory, uses them as usual (since they have all the same properties) and accidentally gets weird bugs which are hard to identify. And yes, we even created an its own section for it in the docs and mentioned in the tutorial, but not many people read such things carefully. I strongly believe that preventing potentially error prone behavior is much better than teaching novice players to avoid it.

[Gankdalf]

@artch If it costs me extra CPU to store my Memory to make sure a newbie's mistakes are handled properly, I probably won't like it.

[Knightshade]

@gankdalf Agreed. Making the game more newbie friendly is something I think we can all get behind, but not at the cost of existing players. I hate to say it, but not reading the documentation is on the player, not us.

[artch]

I don't think it will add extra CPU cost, since all objects already have toJSON handlers, and JSON.parse callback cost is negligible, especially since we're adding global persistence soon.

[SemperRabbit]

@artch I've been playing with this inside of my code, and the CPU ramifications are atrocious... I narrowed the replacer down to the minimum number of object types I think you would be ok with. If statements instead of a switch show no CPU difference. instanceof does not work inside the function, due to the fact that all of their .toJSON() functions are executed prior to being passed into the replacer. The code I used is below and I welcome other players to post their results. I recommend running runTest() in the console once every 3-5 ticks, due to the CPU load. At the end of a number of runTest()s, run getTestResults() to view the stats.

For reference, my stats are as follows:

number of tests: 50
avg without replacer: 4.4156
avg with replacer: 42.0070
avg difference: 37.5914

Code as follows:

global.replacer = function replacer(key, val) {
	if(!val ||  _.isFunction(val)) return;
	switch(true){
		case (val.constructor === Creep):
			return {id: val.id, name: val.name, type: 'Creep'};
		case (val.constructor === ConstructionSite):
			return {id: val.id, name: val.name, type: 'ConstructionSite'};
		case (!!val.structureType): // Structure subclasses
			return {id: val.id, name: val.name, type: val.structureType};
		case (val.constructor === Flag):
			return {id: val.id, name: val.name, type: 'Flag'};
		case (val.constructor === Mineral):
			return {id: val.id, name: val.name, type: 'Mineral'};
		case (val.constructor === Nuke):
			return {id: val.id, name: val.name, type: 'Nuke'};
		case (val.constructor === Resource):
			return {id: val.id, name: val.name, type: 'Resource'};
		case (val.constructor === Source):
			return {id: val.id, name: val.name, type: 'Source'};
		case (val.constructor === Tombstone):
			return {id: val.id, name: val.name, type: 'Tombstone'};
		default:
			return val;
	}
};

global.runTest = function runTest(){
	if(!global.statTest)global.statTest=[];
	var stat = [];

	// no replacer test
	var start=Game.cpu.getUsed();
	JSON.stringify(Memory);
	stat.push(Game.cpu.getUsed()-start);

	// replacer test
	start=Game.cpu.getUsed();
	JSON.stringify(Memory, replacer);
	stat.push(Game.cpu.getUsed()-start);
	statTest.push(stat);
}

global.getTestResults = function getTestResults(){
	if(!global.statTest || !_.isArray(statTest)) return false;

	var len = statTest.length;
	var lowT = 0;
	var higT = 0;
	var diff = 0;

	for(var i=0;i<len;i++){
		var [low, high] = statTest[i];
		lowT += low;
		higT += high;
		diff += high-low;
	}
	return [(lowT/len).toFixed(4), (diff/len).toFixed(4), (higT/len).toFixed(4), len];
}

[Gankdalf]

Another option might be to just print a warning to a player's log if they try to serialize a game object. I haven't looked at what it takes to post to the player's log, but if every toJSON() simply put a warning into a player's console I think it would get the message across pretty strongly.

And player's actually attempting to debug by directly serializing objects to their console would know to expect the warning, since they know what serialization is.

[SemperRabbit]

I really like that option. That would not penalize players that do not store game objects in Memory, and would negligibly affect the CPU of players who do. It would also alert newer players that they are doing something they shouldn't, allowing for player education.

That being said, this option would also affect players who are using the mem hack to prevent parsing on any tick that is not a global reset. I know @o4kapuk mentioned in slack that you are looking at implementing that on the backend too.

[artch]

We're going to implement it this way. We'll add new non-enumerable property to .toJSON here:

var result = {__objId: this.id};

And then JSON.parse callback could simple check for this property and try calling Game.getObjectById(val.__objId) if it's found. Storing prototypes is actually not neccessary. Features:

• Plain and simple.
• No need for a replacer function in JSON.stringify at all.
• All debugging capabilities are remaining.
• Parsed live objects will look the same as in Memory JSON inspector, but with attached prototypes and the live state.
• Players who do not store live objects in Memory won't be affected by this change at all.

[Gankdalf]

@artch I'm not sure I understand. How does JSON.parse check for the new property without adding extra logic that costs CPU for everyone?

[SemperRabbit]

@artch I still feel like your solution causes undue CPU usage for all players. This only enables bad behavior (saving game objects in memory) instead of preventing it or educating the players that it is the wrong thing to do.

[SemperRabbit]

btw, while we're still discussing the whole objects in Memory thing, I submitted a PR for the error described in my original post... https://github.com/screeps/engine/pull/92

[artch]

@gankdalf This cost is negligible, JSON.parse will be called only on global resets.

[artch]

@semperrabbit I don't think that saving game objects to memory will be considered bad behavior after this change. We may even remove this warning from the docs then. It's perfectly safe to store objects in memory if they are parsed correctly. Yes, it makes memory serialization process a bit more abstract for players who are new to JavaScript, but Screeps is not about teaching people how to JavaScript, it's a MMO RTS game.

[SemperRabbit]

@artch ok, so I finally got around to testing adding a function as a 2nd param to JSON.parse for creating Memory... I used the same test code above, except for editing runTest(). below are my results, and the edited portion of the test code. Although the CPU usage is not as awful as the stringify, its still pretty bad...

number of tests: 50
avg without parser: 12.5632
avg with parser: 22.9115
avg difference: 10.3483

global.parser = function parser(key, val) {
	return ((_.isObject(val) && val.id) ? Game.getObjectById(val.id) : val);
};
global.runTest = function runTest(){
	if(!global.statTest)global.statTest=[];
	var stat = [];

	/* no replacer test */
	var start=Game.cpu.getUsed();
	JSON.parse(RawMemory.get());
	stat.push(Game.cpu.getUsed()-start);

	/* replacer test */
	start=Game.cpu.getUsed();
	JSON.parse(RawMemory.get(), parser);
	stat.push(Game.cpu.getUsed()-start);
	statTest.push(stat);
};