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.

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

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

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

targos
Copy link
Member

@targos targos commented Jan 15, 2023

This makes the test compatible with off-thread loaders.

/cc @GeoffreyBooth @aduh95

@nodejs-github-bot 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 targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2023
@nodejs-github-bot
Copy link
Collaborator

Comment on lines 17 to 22
// 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.

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

@joyeecheung joyeecheung Jan 18, 2023

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

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

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

@GeoffreyBooth GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Jan 15, 2023
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth 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
Copy link
Member

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

@GeoffreyBooth GeoffreyBooth mentioned this pull request Jan 15, 2023
7 tasks
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@targos
Copy link
Member Author

targos commented Jan 16, 2023

@GeoffreyBooth Yes, it passes on both branches.

@GeoffreyBooth GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 16, 2023
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 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
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos 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
@aduh95 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit cf8c699 into nodejs:main Jan 19, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in cf8c699

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: #46220
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: #46220
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: #46220
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants