For some database schemas, PgIntrospectionPlugin.js abruptly crashes NodeJS (with --track-heap-objects flag, or memory profiling)

[Venryx]

Summary

There are certain database schemas for which PgIntrospectionPlugin causes an abrupt crash of NodeJS, within its introspect function, if either Node's --track-heap-objects flag is set, or if the memory profiler is recording while it is running.

Steps to reproduce

Since the root bug appears to be in NodeJS, I have filed the main bug report there: nodejs/node#41539

That thread contains clear instructions on how to reproduce the issue. (it links to this minimal repro case)

Expected results

See linked repro case above.

Actual results

An abrupt crash of NodeJS, without any error messages or the like. (see linked repro case)

Additional context

Node: v16.13.2 (the latest LTS release), as well as v14.17.1
OS: Bug occurs on both Windows 10 (my dev computer) and Linux (node-16-alpine, running in Kubernetes pod)

I can attach my database schema if needed (I have a dump of the original JSON that caused the crash), but this shouldn't be necessary since the repro case above is self-contained and demonstrates the same issue. (and saves me the time of ensuring the data is fine to post publicly)

Possible Solution

The ideal solution would be a fix in the NodeJS source code for the root problem. If that is delayed for some reason, there may be a way to avoid the bug on a higher-level in the meantime. (as the bug only seemed to occur when a particular structure was present; when I removed certain fields, for example, or changed the "wrapper" structure around the array, the bug went away)

[Venryx]

Closing the issue, since I'm assuming the NodeJS team will fix the root problem. (if not, it can be re-opened)

[Venryx]

I have found a workaround for the bug for now, involving a slight refactor of the deepClone function. See here for details: nodejs/node#41539 (comment)

If the NodeJS devs take too long to fix the root problem, it may be worth creating a pull-request including the slight fix/refactor, since the bug occurs "in the wild" for some schemas supplied to Postgraphile. (and the crash is very hard to debug for people if they don't already know the cause)

[benjie]

I can easily imagine wanting to refactor the workaround to be the non-workaround'd code to avoid the extra function declaration... Maybe we could refactor to use a loop instead - does that still resolve the issue?

const deepClone = value=>{
	if (Array.isArray(value)) {
		const l = value.length;
		const cloned = new Array(l);
		for (let i = 0; i < l; i++) {
			cloned[i] = deepClone(value[i]);
		}
		return cloned;
	}
	if (typeof value === "object" && value) {
		// Unrolled `reduce` to work around memory bug in Node.js
		const cloned = Object.create(null);
		const keys = Object.keys(value);
		for (let i = 0, l = keys.length; i < l; i++) {
			const k = keys[i];
			cloned[k] = deepClone(value[k]);
		}
        return cloned;
	}
	return value;
};

[Venryx]

I just tested it, and yes, that unrolled version also resolves the crash. :) (And yeah, it definitely seems like a better fix, since it avoids the redundant function declaration.)

On my end, I'm still waiting for the NodeJS dev team to see what they have to say on the crash. (ie. whether they plan to fix it, or if it's enough of an edge-case that it's deprioritized/forgotten-about)

[benjie]

I'm going to close this again, because yes I think it's Node.js' responsibility to fix this.