GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: refactor to avoid mutation of global by a loader by targos · Pull Request #46220 · nodejs/node

Conversation

@targos

@targos targos commented Jan 15, 2023

Copy link
Copy Markdown
Member

This makes the test compatible with off-thread loaders.

/cc @GeoffreyBooth @aduh95

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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 Node.js core tests and test infrastructure. labels Jan 15, 2023
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 15, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 15, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 15, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos
targos commented Jan 15, 2023
View reviewed changes
Comment thread test/fixtures/es-module-loaders/hook-resolve-type.mjs Outdated
Comment on lines 17 to 22

@targos targos Jan 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Jan 15, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@targos targos Jan 16, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Jan 16, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@targos targos Jan 16, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Jan 18, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aduh95
aduh95 approved these changes Jan 15, 2023
View reviewed changes
GeoffreyBooth
GeoffreyBooth approved these changes Jan 15, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@GeoffreyBooth GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Jan 15, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 15, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth GeoffreyBooth added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 15, 2023
@GeoffreyBooth

GeoffreyBooth commented Jan 15, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jan 15, 2023
Move ESM loaders off-thread #44710
Merged
7 tasks done
@targos @GeoffreyBooth
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
targos force-pushed the fix-resolve-type branch from 1cc9f59 to 825261e Compare January 16, 2023 07:10
@targos

targos commented Jan 16, 2023

Copy link
Copy Markdown
Member Author

@GeoffreyBooth Yes, it passes on both branches.
GeoffreyBooth reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth GeoffreyBooth added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 16, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jan 17, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 17, 2023

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Jan 17, 2023
View reviewed changes
Comment thread test/fixtures/es-module-loaders/hook-resolve-type.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 removed author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Jan 18, 2023
@targos @aduh95
Update test/fixtures/es-module-loaders/hook-resolve-type.mjs
de3d1fd 
Co-authored-by: Antoine du Hamel <[email protected]>
@targos targos added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Jan 18, 2023
GeoffreyBooth
GeoffreyBooth approved these changes Jan 18, 2023
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Jan 18, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 18, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 18, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 19, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 19, 2023
@nodejs-github-bot
nodejs-github-bot merged commit cf8c699 into nodejs:main Jan 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

Landed in cf8c699
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@targos @GeoffreyBooth @RafaelGSS
test: refactor to avoid mutation of global by a loader
4ac5c71 
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]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
v19.5.0 proposal #46286
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@targos @GeoffreyBooth @juanarbol
test: refactor to avoid mutation of global by a loader
2e0f047 
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]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
V18.14.0 proposal #46396
Merged
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jan 29, 2023
esm: delete preload mock test #46402
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@targos @GeoffreyBooth @juanarbol
test: refactor to avoid mutation of global by a loader
a3056f4 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung left review comments
@aduh95 aduh95 aduh95 approved these changes
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. esm Issues and PRs related to the ECMAScript Modules implementation. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@targos @nodejs-github-bot @GeoffreyBooth @joyeecheung @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.