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

test: refactor to avoid mutation of global by a loader #46220

Merged
merged 2 commits into from Jan 19, 2023
Merged

test: refactor to avoid mutation of global by a loader #46220

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
825261e
test: refactor to avoid mutation of global by a loader
targos Jan 16, 2023
de3d1fd
Update test/fixtures/es-module-loaders/hook-resolve-type.mjs
targos Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 17 additions & 15 deletions test/es-module/test-esm-loader-resolve-type.mjs
View file
Open in desktop
Expand Up @@ -6,6 +6,9 @@ import * as fs from 'fs';

allowGlobals(global.getModuleTypeStats);

const { importedESM: importedESMBefore,
importedCJS: importedCJSBefore } = await global.getModuleTypeStats();

const basePath =
new URL('./node_modules/', import.meta.url);

Expand All @@ -17,25 +20,24 @@ const createDir = (path) => {
};

const moduleName = 'module-counter-by-type';

const moduleDir = rel(`${moduleName}`);
createDir(basePath);
createDir(moduleDir);
fs.cpSync(
fixtures.path('es-modules', moduleName),
moduleDir,
{ recursive: true }
);

const { importedESM: importedESMBefore,
importedCJS: importedCJSBefore } = global.getModuleTypeStats();

await import(`${moduleName}`).finally(() => {
try {
createDir(basePath);
createDir(moduleDir);
fs.cpSync(
fixtures.path('es-modules', moduleName),
moduleDir,
{ recursive: true }
);


await import(`${moduleName}`);
} finally {
fs.rmSync(basePath, { recursive: true, force: true });
});
}

const { importedESM: importedESMAfter,
importedCJS: importedCJSAfter } = global.getModuleTypeStats();
importedCJS: importedCJSAfter } = await global.getModuleTypeStats();

// Dynamic import above should increment ESM counter but not CJS counter
assert.strictEqual(importedESMBefore + 1, importedESMAfter);
Expand Down
25 changes: 24 additions & 1 deletion test/fixtures/es-module-loaders/hook-resolve-type.mjs
View file
Open in desktop
@@ -1,6 +1,29 @@
let importedESM = 0;
let importedCJS = 0;
global.getModuleTypeStats = () => { return {importedESM, importedCJS} };

export function globalPreload({ port }) {
port.on('message', (int32) => {
port.postMessage({ importedESM, importedCJS });
Atomics.store(int32, 0, 1);
Atomics.notify(int32, 0);
});
port.unref();
return `
const { receiveMessageOnPort } = getBuiltin('worker_threads');
global.getModuleTypeStats = async function getModuleTypeStats() {
const sab = new SharedArrayBuffer(4);
const int32 = new Int32Array(sab);
port.postMessage(int32);
// Artificial timeout to keep the event loop alive.
// https://bugs.chromium.org/p/v8/issues/detail?id=13238
// TODO(targos) Remove when V8 issue is resolved.
const timeout = setTimeout(() => {}, 1_000);
await Atomics.waitAsync(int32, 0, 0).value;
clearTimeout(timeout);
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @joyeecheung this is related to https://bugs.chromium.org/p/v8/issues/detail?id=13238 / #44409

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW the current implementation of #44710 relies on Atomics.waitAsync not keeping the event loop alive.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aduh95 What would be your expectation? Should Atomics.waitAsync keep the event loop alive?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally there would be a way to control it, like you can opt-out for setTimeout. My expectation was that it would not keep the event loop alive, but I could see how that can also be annoying for some (most?) use cases. Does it also not keep the event loop alive if you add a fourth timeout parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

The fourth timeout parameter doesn't change the behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I think so far the vibes I got is that "this is host-defined" i.e. Node.js gets to decide if the timeout parameter keeps the event loop alive (neither the ES or the Web specs have this "shut down the loop on completion" concept, they just assume the agent runs forever until it gets told to shut down, so we are on our own here), but also if we want to implement that, V8 needs to provide a way in the platform API for canceling a delayed task (so that when it wakes up before the timeout, we get notified that there is no need to keep the event loop alive for it anymore), which is probably too much complexity, so to be lazy we can also just keep the things the way they are (not let the time out keep the event loop alive, which some may argue is also their expected behavior, like how an unresolved promise doesn't keep the thread alive).

targos marked this conversation as resolved.
Show resolved Hide resolved
return receiveMessageOnPort(port).message;
};
`;
}

export async function load(url, context, next) {
return next(url);
Expand Down