GeezerVison

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

GeezerVison is showing github.com — loader: use default loader as cascaded loader in the in loader worker by joyeecheung · Pull Request #47620 · nodejs/node

Conversation

joyeecheung commented Apr 19, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

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

Fixes: #47566

Sorry, something went wrong.

Uh oh!

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

bricss, benjamingr, and folke reacted with thumbs up emoji

JakobJingleheimer and jacobq reacted with heart emoji

All reactions

👍 3 reactions
❤️ 2 reactions


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

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/loaders
@nodejs/modules

All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot added lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

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 Resolved

Uh oh!

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

joyeecheung commented

Apr 19, 2023

View reviewed changes

Comment thread

lib/internal/modules/esm/loader.js Resolved

Uh oh!

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

aduh95 approved these changes

Apr 19, 2023

View reviewed changes

aduh95 left a comment

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

GeoffreyBooth commented Apr 19, 2023

Copy link

Copy Markdown

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.

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

Apr 19, 2023

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

Apr 19, 2023

JakobJingleheimer approved these changes

Apr 19, 2023

View reviewed changes

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

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

lib/internal/modules/esm/utils.js

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

JakobJingleheimer Apr 19, 2023

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

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

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

Sorry, something went wrong.

Uh oh!

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

All reactions

JakobJingleheimer Apr 19, 2023

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

aduh95 Apr 19, 2023 •

edited

Loading

Uh oh!

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

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

GeoffreyBooth Apr 20, 2023

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.

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.

Sorry, something went wrong.

Uh oh!

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

JakobJingleheimer reacted with thumbs up emoji

All reactions

👍 1 reaction

JakobJingleheimer Apr 20, 2023

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.

loaderWorkerModuleId?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

lib/internal/modules/esm/utils.js

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

JakobJingleheimer Apr 19, 2023

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.

nit: just make this a getter?

Sorry, something went wrong.

Uh oh!

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

Apr 19, 2023

GeoffreyBooth commented Apr 19, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 commented Apr 19, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

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.

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

nodejs-github-bot commented Apr 19, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels

Apr 19, 2023

aduh95 reviewed

Apr 19, 2023

View reviewed changes

Comment thread

lib/internal/main/worker_thread.js Resolved

Uh oh!

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

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

Copy link

Copy Markdown

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.

benjamingr reacted with heart emoji

All reactions

❤️ 1 reaction


          Apply suggestions from code review

3db9ee1

nodejs-github-bot commented Apr 20, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

benjamingr approved these changes

Apr 20, 2023

View reviewed changes

benjamingr left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

Uh oh!

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

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 done

targos pushed a commit that referenced this pull request

May 2, 2023


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

91e4a7c

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>

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 pushed a commit to targos/node that referenced this pull request

Nov 11, 2023


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

ee9cef2

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>

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 pushed a commit that referenced this pull request

Nov 23, 2023


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

b77a6b2

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>

targos mentioned this pull request

Nov 28, 2023

v18.19.0 release proposal #50953

Merged

sercher added a commit to sercher/graaljs that referenced this pull request

Apr 25, 2024


          loader: use default loader as cascaded loader in the in

cb8981f

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>

sercher added a commit to sercher/graaljs that referenced this pull request

Apr 25, 2024


          loader: use default loader as cascaded loader in the in

c07d36d

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>

inoway46 mentioned this pull request

Jul 31, 2026

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

Closed

Reviewers

GeoffreyBooth

GeoffreyBooth approved these changes

aduh95

aduh95 approved these changes

benjamingr

benjamingr approved these changes

JakobJingleheimer

JakobJingleheimer approved these changes

+2 more reviewers

merceyz

merceyz left review comments

targos

targos approved these changes

Reviewers whose approvals may not affect merge requirements

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.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

loaders

Issues and PRs related to ES module loaders.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using custom loaders results in a load loop / OOM

Uh oh!

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

9 participants

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.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

joyeecheung commented Apr 19, 2023 •

edited

Loading

Uh oh!

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

aduh95 Apr 19, 2023 •

edited

Loading

Uh oh!

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

GeoffreyBooth commented Apr 19, 2023 •

edited

Loading

Uh oh!

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

aduh95 commented Apr 19, 2023 •

edited

Loading

Uh oh!

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