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

loader: use default loader as cascaded loader in the in loader worker #47620

Merged
merged 7 commits into from
Apr 21, 2023

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 19, 2023

Use the default loader as the cascaded loader in the loader worker. Otherwise we spawn loader workers in the loader workers indefinitely.

Fixes: #47566

Use the default loader as the cascaded loader in the loader worker.
Otherwise we spawn loader workers in the loader workers indefinitely.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 19, 2023
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I agree it's a better fix than my PR, I would prefer if we used my test, but other than that LGTM. Thanks!

@GeoffreyBooth
Copy link
Member

I agree it’s a better fix than my PR, I would prefer if we used my test, but other than that LGTM. Thanks!

I’d like to add a few more tests, from the other PR and the simpler cases we discussed that could also trigger the issue, but then this LGTM.

@GeoffreyBooth GeoffreyBooth added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Apr 19, 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 Apr 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2023
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

This seems a nicely simple fix—thanks for putting it together!

Before landing I would like there to be a few more code comments and, if possible, leverage an existing, persistent fixture (I believe the repro of the issue this addresses does not require dynamically created files, so let's not over-complicate things).

@@ -165,4 +173,6 @@ module.exports = {
initializeHooks,
getDefaultConditions,
getConditionsSet,
loaderWorkerId: 'internal/modules/esm/worker',
Copy link
Member

Choose a reason for hiding this comment

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

nit: id seems misleading to me, like it's a PID or similar, rather than a path.

Suggested change
loaderWorkerId: 'internal/modules/esm/worker',
loaderWorkerSpecifier: 'internal/modules/esm/worker',

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely not a path (because the internal module loader doesn't use paths), I think calling it an ID is more correct.

Copy link
Member

Choose a reason for hiding this comment

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

O.o it is literally the path to the file (but with CJS's whacked exclusion of the file extension) and is even consumed as such:

this.#worker = new InternalWorker(loaderWorkerId, {

Copy link
Contributor

@aduh95 aduh95 Apr 19, 2023

Choose a reason for hiding this comment

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

It's not the path to a file, because internal modules are not in the FS, it's more correct to think of them as an offset in the node binary. InternalWorker constructor doesn't take a path, it only accepts internal module IDs – you can try to replace loaderWorkerId with a path at the line you linked to to convince yourself if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it’s whatever we call the string that’s the input to internalBinding(). The signature of that function is internalBinding(module), so I guess we could use loaderWorkerModule. Not sure if that’s any better than loaderWorkerId. It’s not really a specifier because we don’t use this with import.

Copy link
Member

Choose a reason for hiding this comment

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

loaderWorkerModuleId?

@@ -165,4 +173,6 @@ module.exports = {
initializeHooks,
getDefaultConditions,
getConditionsSet,
loaderWorkerId: 'internal/modules/esm/worker',
isLoaderWorker,
Copy link
Member

Choose a reason for hiding this comment

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

nit: just make this a getter?

Copy link
Member

Choose a reason for hiding this comment

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

I would assume a function call would be slower than just mapping directly to a primitive?

lib/internal/modules/esm/loader.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 19, 2023
@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Apr 19, 2023

This isn’t author ready, there are many open questions and requests for more tests. If I have time I’ll add the tests, but this shouldn’t land as is. I think we agree with the general approach and have only nits for the changes themselves, but I at least have objections to the tests.

@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2023

I suggest we land ASAP (as is) and address the concerns regarding tests and such in a follow up PR, the current flakiness of the CI si very bad for everyone, and there are no blocking comments.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 19, 2023
@GeoffreyBooth
Copy link
Member

I suggest we land ASAP (as is) and address the concerns regarding tests and such in a follow up PR, the current flakiness of the CI si very bad for everyone, and there are no blocking comments.

I’ve added the tests and implemented the code suggestions that had clear resolutions. I left alone possibly renaming loaderWorkerId or changing it to a getter, as we don’t seem to have consensus on those; and those are nits that I think can easily be taken care of in a follow up. I’m okay with landing this now.

@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

tests lgtm

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 21, 2023
@nodejs-github-bot nodejs-github-bot merged commit f14d2e5 into nodejs:main Apr 21, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in f14d2e5

targos pushed a commit that referenced this pull request May 2, 2023
Use the default loader as the cascaded loader in the loader worker.
Otherwise we spawn loader workers in the loader workers indefinitely.

PR-URL: #47620
Fixes: #47566
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request May 2, 2023
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jun 27, 2023
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
Use the default loader as the cascaded loader in the loader worker.
Otherwise we spawn loader workers in the loader workers indefinitely.

PR-URL: nodejs#47620
Fixes: nodejs#47566
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
Use the default loader as the cascaded loader in the loader worker.
Otherwise we spawn loader workers in the loader workers indefinitely.

PR-URL: #47620
Backport-PR-URL: #50669
Fixes: #47566
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Use the default loader as the cascaded loader in the loader worker.
Otherwise we spawn loader workers in the loader workers indefinitely.

PR-URL: nodejs/node#47620
Backport-PR-URL: nodejs/node#50669
Fixes: nodejs/node#47566
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Use the default loader as the cascaded loader in the loader worker.
Otherwise we spawn loader workers in the loader workers indefinitely.

PR-URL: nodejs/node#47620
Backport-PR-URL: nodejs/node#50669
Fixes: nodejs/node#47566
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Michaël Zasso <targos@protonmail.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. backported-to-v18.x PRs backported to the v18.x-staging branch. 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. lib / src Issues and PRs related to general changes in the lib or src directory. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using custom loaders results in a load loop / OOM
9 participants