lib: make sure globals can be loaded after globalThis is sealed

[joyeecheung]

Since we made some of the globals lazily loaded to reduce startup overhead, and tried to update then as data properties once loaded, the lazily-loaded globals can no longer be loaded once globalThis is freezed. Fix this by not attempting to overwrite the property on loading these globals (so the property descriptors won't be spec-compliant in this case, but at least it's a good compromise between breakages and startup overhead).

Note that certain globals that come from undici are still broken because undici attempts to define a global dispatcher symbol on globalThis. This is reflected in the test.

Refs: #46788

[bnoordhuis]

Can I suggest moving this line inside the if statement below? If the getter can't be replaced, it should do as little work as possible in the common path.

[joyeecheung]

I suppose by the if statement you meant if (lazyLoadedValue === undefined)? In that case it made sense to me, otherwise (if (!ObjectIsFrozen(this))) that's probably not the desired behavior (we can still return the result in the getter, just can't fix up the descriptor when it's frozen).

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50075/

[aduh95]

Consider updating the commit message. I left a few suggestions which would I think simplify the tests.

[aduh95]

Suggested change

	try {
	success.set(key, globalThis[key]);
	} catch (e) {
	failures.set(key, e);
	}
	const access = () => globalThis[key];
	if (knownFailures.has(key)) {
	assert.throws(access, { name: 'TypeError' });
	failures.delete(key);
	} else {
	access();
	}

[joyeecheung]

Why is it deleting the failure (which is undeclared if we apply the suggestion above)?

[aduh95]

Sorry I meant

Suggested change

	try {
	success.set(key, globalThis[key]);
	} catch (e) {
	failures.set(key, e);
	}
	const access = () => globalThis[key];
	if (knownFailures.has(key)) {
	assert.throws(access, { name: 'TypeError' });
	knownFailures.delete(key);
	} else {
	access();
	}

[joyeecheung]

I think this version would lead to less debuggable messages - for example if the error is caused by redefining something else on a global in the implementation of another (e.g. the undici symbol for the known failures listed here), the error message would not show which key is causing the trouble, and only shows the symbol which is an implementation detail of that key. So fixing the known failures takes 4 trial-and-errors. Whereas having a failures map for them all and logging the map only takes 1 trail-and-error.

[aduh95]

What do you think of the following then?

Suggested change

	try {
	success.set(key, globalThis[key]);
	} catch (e) {
	failures.set(key, e);
	}
	const access = () => globalThis[key];
	if (knownFailures.has(key)) {
	assert.throws(access, { name: 'TypeError' });
	knownFailures.delete(key);
	} else {
	try { access(); } catch { unexpectedFailures.add(key); }
	}

[joyeecheung]

(And also I personally prefer to avoid one-line helpers like access() here in the test because they make the test failures harder to read. IMO tests are usually easier to understand if we can just be verbose and write the code being tested directly even if it involves some repetition - as long as it's not too much repetition)

[aduh95]

At least you can agree that the success map is not useful, nor for the test itself, and likely not for debugging either?

[joyeecheung]

What do you think of the following then?

I think this is more complicated than the original version, which just adds results on success and error on failure to two maps instead of trying to add or delete anything? At least it'd take me a while to understand what the second version is doing, whereas the original one doesn't take much brain power to understand.

[joyeecheung]

At least you can agree that the success map is not useful, nor for the test itself, and likely not for debugging either?

That avoids having to add a comment to skip the no-unused-expressions eslint check. I figure if we have to throw extra code to avoid it, might as well just put them into a map, in case one wants to find out what can actually be loaded (for example, to find out fetch can be loaded, but other fetch-related classes can't).

[aduh95]

Suggested change

	const keys = Reflect.ownKeys(globalThis).filter((k) => typeof k === 'string');
	const keys = Reflect.ownKeys(globalThis);

[joyeecheung]

Hmm, I don't think having only the keys is an improvement for debugging the failures? At least it helped my figuring out what's going on when I could just console.log() the failures. Sometimes I think there is a tendency in our tests that made the tests too simple and DRY so it makes the tests harder to debug, but I'd personally prefer debuggability over simplicity...

[aduh95]

Did you post your comment on the right thread? This suggestion is removing a filter (so we run the test on all the keys), it's not adding one.

[joyeecheung]

Oh, right, the comment should be posted to the for-loop below. But I don't think we need to run the test on all keys though, otherwise we run into the undici dispatch symbol, and I don't see that it's Node.js core that should test the symbol works on a frozen globalThis?

[aduh95]

Restricting to string keys is a bit weird, I think it would be better to be able to access all global keys when the global object is sealed, and add to one that would fail to the expected failures set. If that's harder than it's worth, we should add a comment explaining that.

[joyeecheung]

Personally I'd only care about string keys because it would be weird that contextual lookup on the global stops working once globalThis is sealed. Symbols not so much because one can't do contextual lookup on the global with symbols, and also these symbols aren't necessarily visible to/fixable by Node.js core (like the undici one). But I can add a comment.

[aduh95]

Suggested change

	const failures = new Map();
	const success = new Map();

[aduh95]

Suggested change

	assert.deepStrictEqual(new Set(failures.keys()), knownFailures);
	assert.deepStrictEqual(knownFailures, new Set());