Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c6fc71
lib: make sure globals can be loaded after globalThis is freezed
joyeecheung Feb 24, 2023
8392842
fixup! lib: make sure globals can be loaded after globalThis is freezed
joyeecheung Feb 27, 2023
fffbcbd
fixup! lib: make sure globals can be loaded after globalThis is seale…
joyeecheung Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const {
ObjectGetOwnPropertyDescriptors,
ObjectGetPrototypeOf,
ObjectFreeze,
ObjectIsFrozen,
ObjectPrototypeHasOwnProperty,
ObjectSetPrototypeOf,
ObjectValues,
Expand Down Expand Up @@ -532,7 +533,10 @@ function defineLazyProperties(target, id, keys, enumerable = true) {
mod ??= require(id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@joyeecheung joyeecheung Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if (lazyLoadedValue === undefined) {
lazyLoadedValue = mod[key];
set(lazyLoadedValue);
if (!ObjectIsFrozen(this)) {
joyeecheung marked this conversation as resolved.
Show resolved Hide resolved
// Replace it with a data property.
set(lazyLoadedValue);
}
}
return lazyLoadedValue;
}
Expand Down
24 changes: 24 additions & 0 deletions test/parallel/test-frozen-global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
'use strict';

// This tests that the globals can still be loaded after globalThis is freezed.
joyeecheung marked this conversation as resolved.
Show resolved Hide resolved

require('../common');
const assert = require('assert');

Object.freeze(globalThis);
const keys = Reflect.ownKeys(globalThis).filter((k) => typeof k === 'string');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@joyeecheung joyeecheung Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// These failures come from undici. We can remove them once they are fixed.
const knownFailures = new Set(['FormData', 'Headers', 'Request', 'Response']);
const failures = new Map();
const success = new Map();
Comment on lines +13 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for (const key of keys) {
try {
success.set(key, globalThis[key]);
} catch (e) {
failures.set(key, e);
}
Comment on lines +16 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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); }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

console.log(failures);
joyeecheung marked this conversation as resolved.
Show resolved Hide resolved
assert.deepStrictEqual(new Set(failures.keys()), knownFailures);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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