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

targos · 2023-01-15T12:07:37Z

This makes the test compatible with off-thread loaders.

nodejs-github-bot · 2023-01-15T12:30:27Z

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

targos · 2023-01-15T12:43:41Z

test/fixtures/es-module-loaders/hook-resolve-type.mjs

+ // 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);


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

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

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

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?

The fourth timeout parameter doesn't change the behavior.

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

GeoffreyBooth

LGTM. FYI in nodejs/loaders#124 (reply in thread) I’m discussing redesigning globalPreload, possibly by creating a new API to handle the communications port stuff and getting rid of the “sloppy mode script” aspect of globalPreload now that we have --import. I was thinking that that could come after we land off-thread, though, and possibly even after making loaders stable (I would keep globalPreload as experimental but mark resolve and load as stable).

nodejs-github-bot · 2023-01-15T21:13:00Z

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

GeoffreyBooth · 2023-01-15T21:19:10Z

@targos I assume this new version of this test passes both on main and in the off-thread branch?

This makes the test compatible with off-thread loaders. Co-Authored-By: Geoffrey Booth <[email protected]>

targos · 2023-01-16T09:58:46Z

@GeoffreyBooth Yes, it passes on both branches.

nodejs-github-bot · 2023-01-17T12:13:12Z

Commit Queue failed

- Loading data for nodejs/node/pull/46220 ✔ Done loading data for nodejs/node/pull/46220 ----------------------------------- PR info ------------------------------------ Title test: refactor to avoid mutation of global by a loader (#46220) Author Michaël Zasso (@targos) Branch targos:fix-resolve-type -> nodejs:main Labels test, esm, author ready Commits 1 - test: refactor to avoid mutation of global by a loader Committers 1 - Michaël Zasso PR-URL: https://github.com/nodejs/node/pull/46220 Reviewed-By: Antoine du Hamel Reviewed-By: Geoffrey Booth ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46220 Reviewed-By: Antoine du Hamel Reviewed-By: Geoffrey Booth -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 15 Jan 2023 12:07:37 GMT ✔ Approvals: 2 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46220#pullrequestreview-1249340907 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/46220#pullrequestreview-1249358333 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-01-15T21:13:00Z: https://ci.nodejs.org/job/node-test-pull-request/48993/ ⚠ Commits were pushed after the last Full PR CI run: ⚠ - test: refactor to avoid mutation of global by a loader - Querying data for job/node-test-pull-request/48993/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3939092857

test/fixtures/es-module-loaders/hook-resolve-type.mjs

Co-authored-by: Antoine du Hamel <[email protected]>

nodejs-github-bot · 2023-01-18T21:35:05Z

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

nodejs-github-bot · 2023-01-19T00:14:09Z

Landed in cf8c699

This makes the test compatible with off-thread loaders. Co-Authored-By: Geoffrey Booth <[email protected]> PR-URL: #46220 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 15, 2023

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2023

targos commented Jan 15, 2023
View reviewed changes

aduh95 approved these changes Jan 15, 2023
View reviewed changes

GeoffreyBooth approved these changes Jan 15, 2023
View reviewed changes

GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Jan 15, 2023

GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 15, 2023

GeoffreyBooth mentioned this pull request Jan 15, 2023
Move ESM loaders off-thread #44710
Merged
7 tasks

test: refactor to avoid mutation of global by a loader
825261e
This makes the test compatible with off-thread loaders. Co-Authored-By: Geoffrey Booth <[email protected]>

targos force-pushed the fix-resolve-type branch from 1cc9f59 to 825261eCompare January 16, 2023 07:10

GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 16, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 17, 2023

aduh95 reviewed Jan 17, 2023
View reviewed changes

test/fixtures/es-module-loaders/hook-resolve-type.mjs Outdated Show resolvedHide resolved

aduh95 removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 18, 2023

Update test/fixtures/es-module-loaders/hook-resolve-type.mjs
de3d1fd
Co-authored-by: Antoine du Hamel <[email protected]>

targos added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 18, 2023

GeoffreyBooth approved these changes Jan 18, 2023
View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 18, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023

nodejs-github-bot merged commit cf8c699 into nodejs:mainJan 19, 2023

RafaelGSS mentioned this pull request Jan 20, 2023
v19.5.0 proposal #46286
Merged

juanarbol mentioned this pull request Jan 28, 2023
V18.14.0 proposal #46396
Merged

GeoffreyBooth mentioned this pull request Jan 29, 2023
esm: delete preload mock test #46402
Merged

Uh oh!

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

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

Uh oh!

Conversation

targos commented Jan 15, 2023

Uh oh!

nodejs-github-bot commented Jan 15, 2023

Uh oh!

targosJan 15, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

targosJan 16, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

targosJan 16, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungJan 18, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 15, 2023

Uh oh!

GeoffreyBooth commented Jan 15, 2023

Uh oh!

targos commented Jan 16, 2023

Uh oh!

nodejs-github-bot commented Jan 17, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 18, 2023

Uh oh!

nodejs-github-bot commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joyeecheungJan 18, 2023•
edited
Loading