loader: use default loader as cascaded loader in the in loader worker by joyeecheung · Pull Request #47620 · nodejs/node · GitHub

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.

Review requested:

@nodejs/loaders
@nodejs/modules

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

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

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',

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

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:

node/lib/internal/modules/esm/hooks.js

Line 498 in 3707d00

this.#worker = new InternalWorker(loaderWorkerId, {

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.

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.

loaderWorkerModuleId?

nit: just make this a getter?

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

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

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.

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.

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

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.

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

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

tests lgtm

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

Landed in f14d2e5

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>

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>

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>

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>

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

3707d00

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

joyeecheung mentioned this pull request Apr 19, 2023

esm: do not lazy instantiate loaders to avoid infinite loop #47599

Closed

joyeecheung commented Apr 19, 2023

View reviewed changes

Comment thread test/es-module/test-loader-worker.js Outdated

joyeecheung commented Apr 19, 2023

View reviewed changes

Comment thread lib/internal/modules/esm/loader.js

aduh95 approved these changes Apr 19, 2023

View reviewed changes

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

benjamingr approved these changes Apr 19, 2023

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 19, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2023

JakobJingleheimer approved these changes Apr 19, 2023

View reviewed changes

GeoffreyBooth removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 19, 2023

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

aduh95 reviewed Apr 19, 2023

View reviewed changes

Comment thread lib/internal/main/worker_thread.js Outdated

GeoffreyBooth added 3 commits April 19, 2023 17:28

Add tests, reimplement new test in current loaders tests style

4c0fbfd

Add comment

bb939a5

Clarity

954e02e

GeoffreyBooth added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 20, 2023

aduh95 reviewed Apr 20, 2023

View reviewed changes

Comment thread test/es-module/test-loaders-workers-spawned.mjs Outdated

aduh95 reviewed Apr 20, 2023

View reviewed changes

Comment thread test/es-module/test-loaders-workers-spawned.mjs Outdated

aduh95 reviewed Apr 20, 2023

View reviewed changes

Comment thread test/es-module/test-loaders-workers-spawned.mjs Outdated

Apply suggestions from code review

3db9ee1

benjamingr approved these changes Apr 20, 2023

View reviewed changes

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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 21, 2023

nodejs-github-bot merged commit f14d2e5 into nodejs:main Apr 21, 2023

This was referenced Apr 24, 2023

CI Reliability 2023-04-24 nodejs/reliability#545

Open

CI Reliability 2023-04-25 nodejs/reliability#546

Open

CI Reliability 2023-04-26 nodejs/reliability#547

Open

gtm-nayan mentioned this pull request Apr 26, 2023

[BUG] Node 20: Test timeouts do not exit gracefully microsoft/playwright#22641

Closed

1 task

targos mentioned this pull request May 2, 2023

v20.1.0 release proposal #47820

Merged

This was referenced May 4, 2023

Release plan - v20.x Maintenance LTS nodejs/Release#855

Closed

Using custom loaders results in a load loop / OOM #47566

Closed

danielleadams added the backport-requested-v18.x label Jun 27, 2023

targos mentioned this pull request Nov 11, 2023

[v18.x] Backport most ESM and customization hook changes #50669

Closed

targos added backported-to-v18.x and removed backport-requested-v18.x labels Nov 23, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

inoway46 mentioned this pull request Jul 31, 2026

Static and dynamic imports inside loader code should have same behaviour #43244

Closed

                 initializeHooks,
                 getDefaultConditions,
                 getConditionsSet,
+                loaderWorkerId: 'internal/modules/esm/worker',

                 getDefaultConditions,
                 getConditionsSet,
+                loaderWorkerId: 'internal/modules/esm/worker',
+                isLoaderWorker,

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

loader: use default loader as cascaded loader in the in loader worker by joyeecheung · Pull Request #47620 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

loader: use default loader as cascaded loader in the in loader worker#47620nodejs-github-bot merged 7 commits intonodejs:mainnodejs/node:mainfrom joyeecheung:fix-loader-workerjoyeecheung/node:fix-loader-workerCopy head branch name to clipboard

Conversation

joyeecheung commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 19, 2023

Uh oh!

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Apr 19, 2023

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Apr 19, 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 Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 19, 2023

Uh oh!

GeoffreyBooth commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 19, 2023

Uh oh!

Uh oh!

GeoffreyBooth commented Apr 20, 2023

Uh oh!

nodejs-github-bot commented Apr 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 20, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 20, 2023

Uh oh!

nodejs-github-bot commented Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

loader: use default loader as cascaded loader in the in loader worker#47620
nodejs-github-bot merged 7 commits into
nodejs:mainfrom
joyeecheung:fix-loader-worker

joyeecheung commented Apr 19, 2023 •
edited
Loading

aduh95 Apr 19, 2023 •
edited
Loading

GeoffreyBooth commented Apr 19, 2023 •
edited
Loading

aduh95 commented Apr 19, 2023 •
edited
Loading