module: cache synchronous module jobs before linking #52868

joyeecheung · 2024-05-07T00:17:39Z

test: add common.expectRequiredModule()

To minimize changes if/when we change the layout of the
result returned by require(esm).

module: cache synchronous module jobs before linking

So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

Fixes: #52864

To minimize changes if/when we change the layout of the result returned by require(esm).

So that if there are circular dependencies in the synchronous module graph, they could be resolved using the cached jobs. In case linking fails and the error gets caught, reset the cache right after linking. If it succeeds, the caller will cache it again. Otherwise the error bubbles up to users, and since we unset the cache for the unlinkable module the next attempt would still fail.

nodejs-github-bot · 2024-05-07T00:17:44Z

Review requested:

@nodejs/loaders

nodejs-github-bot · 2024-05-07T00:20:27Z

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

lib/internal/modules/esm/module_job.js

test/common/index.js

test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs

nodejs-github-bot · 2024-05-07T20:15:47Z

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

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

nodejs-github-bot · 2024-05-07T21:33:49Z

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

legendecas · 2024-05-08T12:53:24Z

lib/internal/modules/esm/module_job.js

+    assert(this.module instanceof ModuleWrap);
+    // Store itself into the cache first before linking in case there are circular
+    // references in the linking.
+    loader.loadCache.set(url, importAttributes.type, this);


Setting in cache at

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

Lines 405 to 407 in fe4e569

job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk);

this.loadCache.set(url, importAttributes.type, job);

would be redundant but I think it could be a follow up work.

That was what I did initially but it would cache modules that failed to link and if that’s caught, subsequent attempts would not fail as expected - which is why I added comments explaining why it’s reset in the finally block (doing it inside the linking phase prevents a rethrow in the caller which looks less nice)

Hmmm, maybe we could transform the try-finally into a try-catch-rethrow and only delete in catch? In this way we don't need to set cache again in the caller.

The rethrow was exactly what I am trying to prevent - setting the cache again is fine as the overhead is trivial but having to rethrow in all callers of this makes the code harder to read IMO. It also makes all the source lines of the errors less readable, which shows in the stderr when they are not caught.

nodejs-github-bot · 2024-05-09T00:25:16Z

Landed in 1291d3d...2863c54

To minimize changes if/when we change the layout of the result returned by require(esm). PR-URL: #52868 Fixes: #52864 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

So that if there are circular dependencies in the synchronous module graph, they could be resolved using the cached jobs. In case linking fails and the error gets caught, reset the cache right after linking. If it succeeds, the caller will cache it again. Otherwise the error bubbles up to users, and since we unset the cache for the unlinkable module the next attempt would still fail. PR-URL: #52868 Fixes: #52864 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

To minimize changes if/when we change the layout of the result returned by require(esm). PR-URL: #52868 Fixes: #52864 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

So that if there are circular dependencies in the synchronous module graph, they could be resolved using the cached jobs. In case linking fails and the error gets caught, reset the cache right after linking. If it succeeds, the caller will cache it again. Otherwise the error bubbles up to users, and since we unset the cache for the unlinkable module the next attempt would still fail. PR-URL: #52868 Fixes: #52864 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

joyeecheung added 2 commits May 7, 2024 08:11

test: add common.expectRequiredModule()

6ea1c51

To minimize changes if/when we change the layout of the result returned by require(esm).

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels May 7, 2024

joyeecheung mentioned this pull request May 7, 2024

Experimental require of circular ESM graph stack overflow #52864

Closed

anonrig reviewed May 7, 2024

View reviewed changes

lib/internal/modules/esm/module_job.js Show resolved Hide resolved

test/common/index.js Show resolved Hide resolved

test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs Outdated Show resolved Hide resolved

MoLow approved these changes May 7, 2024

View reviewed changes

H4ad approved these changes May 7, 2024

View reviewed changes

aduh95 approved these changes May 7, 2024

View reviewed changes

fixup! module: cache synchronous module jobs before linking

c034992

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels May 8, 2024

legendecas approved these changes May 8, 2024

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2024

nodejs-github-bot closed this May 9, 2024

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: cache synchronous module jobs before linking #52868

module: cache synchronous module jobs before linking #52868

joyeecheung commented May 7, 2024

nodejs-github-bot commented May 7, 2024

nodejs-github-bot commented May 7, 2024

nodejs-github-bot commented May 7, 2024

nodejs-github-bot commented May 7, 2024

legendecas May 8, 2024

joyeecheung May 8, 2024 •

edited

legendecas May 8, 2024 •

edited

joyeecheung May 10, 2024 •

edited

nodejs-github-bot commented May 9, 2024

	job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk);

	this.loadCache.set(url, importAttributes.type, job);

module: cache synchronous module jobs before linking #52868

module: cache synchronous module jobs before linking #52868

Conversation

joyeecheung commented May 7, 2024

test: add common.expectRequiredModule()

module: cache synchronous module jobs before linking

nodejs-github-bot commented May 7, 2024

nodejs-github-bot commented May 7, 2024

nodejs-github-bot commented May 7, 2024

nodejs-github-bot commented May 7, 2024

legendecas May 8, 2024

Choose a reason for hiding this comment

joyeecheung May 8, 2024 • edited

Choose a reason for hiding this comment

legendecas May 8, 2024 • edited

Choose a reason for hiding this comment

joyeecheung May 10, 2024 • edited

Choose a reason for hiding this comment

nodejs-github-bot commented May 9, 2024

joyeecheung May 8, 2024 •

edited

legendecas May 8, 2024 •

edited

joyeecheung May 10, 2024 •

edited