undici leaking Symbol to global space in 18.2 (but not 18.1)

[Trott]

Version

18.2.0

Platform

macOS 20.6.0

Subsystem

http

What steps will reproduce the bug?

No response

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

I'll try to come up with a narrower test case later (unless someone finds the issue before then), but running tests for https://github.com/Trott/solr-proxy in 18.2.0 results in a report that Symbol(undici.globalDispatcher.1) is leaked. Running the same test with Node.js 18.1.0 does not report that.

Refs: https://github.com/Trott/solr-proxy/runs/6525176564?check_suite_focus=true

Additional information

No response

[climba03003]

I think it is true that the symbol is added to global because it allow the support of shared global dispatcher between node and undici.

It is added #43059

https://github.com/nodejs/undici/blob/d023b2d808169b7269307d2546125b780e7be5ad/lib/global.js#L5-L27

[targos]

Aren't we supposed to detect leaked globals in the Node.js CI?

[mscdex]

Aren't we supposed to detect leaked globals in the Node.js CI?

We probably only test for actual globals and not global symbols. Does V8 even provide an API to get a list of global/shared symbols (created via Symbol.for())?

[Trott]

Aren't we supposed to detect leaked globals in the Node.js CI?

The check is in test/common/index.js. I'm guessing that it would miss a symbol, though.

node/test/common/index.js

Lines 266 to 368 in 26846a0

	let knownGlobals = [
	atob,
	btoa,
	clearImmediate,
	clearInterval,
	clearTimeout,
	global,
	setImmediate,
	setInterval,
	setTimeout,
	queueMicrotask,
	];
	// TODO(@jasnell): This check can be temporary. AbortController is
	// not currently supported in either Node.js 12 or 10, making it
	// difficult to run tests comparatively on those versions. Once
	// all supported versions have AbortController as a global, this
	// check can be removed and AbortController can be added to the
	// knownGlobals list above.
	if (global.AbortController)
	knownGlobals.push(global.AbortController);
	if (global.gc) {
	knownGlobals.push(global.gc);
	}
	if (global.performance) {
	knownGlobals.push(global.performance);
	}
	if (global.PerformanceMark) {
	knownGlobals.push(global.PerformanceMark);
	}
	if (global.PerformanceMeasure) {
	knownGlobals.push(global.PerformanceMeasure);
	}
	// TODO(@ethan-arrowood): Similar to previous checks, this can be temporary
	// until v16.x is EOL. Once all supported versions have structuredClone we
	// can add this to the list above instead.
	if (global.structuredClone) {
	knownGlobals.push(global.structuredClone);
	}
	if (global.fetch) {
	knownGlobals.push(fetch);
	}
	if (hasCrypto && global.crypto) {
	knownGlobals.push(global.crypto);
	knownGlobals.push(global.Crypto);
	knownGlobals.push(global.CryptoKey);
	knownGlobals.push(global.SubtleCrypto);
	}
	if (global.ReadableStream) {
	knownGlobals.push(
	global.ReadableStream,
	global.ReadableStreamDefaultReader,
	global.ReadableStreamBYOBReader,
	global.ReadableStreamBYOBRequest,
	global.ReadableByteStreamController,
	global.ReadableStreamDefaultController,
	global.TransformStream,
	global.TransformStreamDefaultController,
	global.WritableStream,
	global.WritableStreamDefaultWriter,
	global.WritableStreamDefaultController,
	global.ByteLengthQueuingStrategy,
	global.CountQueuingStrategy,
	global.TextEncoderStream,
	global.TextDecoderStream,
	global.CompressionStream,
	global.DecompressionStream,
	);
	}
	function allowGlobals(...allowlist) {
	knownGlobals = knownGlobals.concat(allowlist);
	}
	if (process.env.NODE_TEST_KNOWN_GLOBALS !== '0') {
	if (process.env.NODE_TEST_KNOWN_GLOBALS) {
	const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
	allowGlobals(...knownFromEnv);
	}
	function leakedGlobals() {
	const leaked = [];
	for (const val in global) {
	if (!knownGlobals.includes(global[val])) {
	leaked.push(val);
	}
	}
	return leaked;
	}
	process.on('exit', function() {
	const leaked = leakedGlobals();
	if (leaked.length > 0) {
	assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
	}
	});
	}

[aduh95]

Aren't we supposed to detect leaked globals in the Node.js CI?

We currently only test for enumerable globals, for some reason. I've #42056 open that should address this, but it's still blocked on nodejs/build#2879.

[bnoordhuis]

As the undici.globalDispatcher.1 symbol is used by people now (we mention it as a workaround in a couple of places in the bug tracker) I don't think we can - or want to - remove it again, at least not until #43187 is done, and even then a grace period is needed.

I'll go ahead and close this but please reopen if needed.