module: handle cached linked async jobs in require(esm)#57187

joyeecheung · 2025-02-23T23:04:20Z

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. It's similar to how we handle the case when the two are both asynchronous.

When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). The evaluation, being synchronous, is guaranteed to complete by the time the other run() is about to start evaluation, the other task would just get the cached evaluation results instead of re-evaluating it, similar to what would happen if both are asynchronously imported.
When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require().

Refs: shufo/prettier-plugin-blade#311
Refs: https://github.com/fisker/prettier-plugin-blade-311
Refs: mochajs/mocha#5290
Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic
Fixes: #57172

nodejs-github-bot · 2025-02-23T23:04:25Z

Review requested:

@nodejs/loaders
@nodejs/vm

joyeecheung · 2025-02-23T23:17:48Z

I think it's still possible to have cached async jobs yielding JS-land handling while another require(esm) comes in and pick up the cache for the same module. In those cases, we can't really handle them as we can't turn a linking process that has started asynchronously to switch to a synchronous route. To get rid of these edge cases once and for all, we should do at least part of #55782 and make the linking fully synchronous. But I haven't seen those edge cases so far and the refactoring would be more complex to complete. Until then, this fixes the two cases that we can handle without refactoring and have appeared in the user land.

I don't think I can minimally reproduce the shape of the microtask queue of the two cases, as they depend on how linking and evaluation tasks from a complex graph happen to be interleaved. So I didn't add any tests, but I can confirm locally it fixes https://github.com/fisker/prettier-plugin-blade-311 and https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic

codecov · 2025-02-24T00:56:14Z

Codecov Report

❌ Patch coverage is 69.38776% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.23%. Comparing base (47a59bd) to head (be776fa).
⚠️ Report is 1150 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/loader.js	62.68%	24 Missing and 1 partial ⚠️
src/module_wrap.cc	84.61%	2 Missing and 2 partials ⚠️
lib/internal/modules/esm/module_job.js	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #57187 +/- ## ========================================== - Coverage 90.24% 90.23% -0.01%  ========================================== Files 630 630 Lines 184920 184980 +60 Branches 36186 36198 +12 ========================================== + Hits 166874 166920 +46 - Misses 11059 11088 +29 + Partials 6987 6972 -15

Files with missing lines	Coverage Δ
lib/internal/modules/esm/module_job.js	`98.27% <80.00%> (-0.22%)`	⬇️
src/module_wrap.cc	`72.45% <84.61%> (+0.22%)`	⬆️
lib/internal/modules/esm/loader.js	`95.04% <62.68%> (-1.64%)`	⬇️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nodejs-github-bot · 2025-02-24T08:53:48Z

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

guybedford

Would be great to see this backported.

lib/internal/modules/esm/loader.js

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require().

nodejs-github-bot · 2025-03-03T13:16:29Z

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

nodejs-github-bot · 2025-03-03T13:30:53Z

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

mcollina

lgtm

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: nodejs#57187Fixes: nodejs#57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in nodejs#57187. PR-URL: nodejs#58067Fixes: nodejs#58061 Reviewed-By: Jacob Smith <[email protected]>

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: nodejs#57187Fixes: nodejs#57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in nodejs#57187. PR-URL: nodejs#58067Fixes: nodejs#58061 Reviewed-By: Jacob Smith <[email protected]>

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: nodejs#57187Fixes: nodejs#57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in nodejs#57187. PR-URL: nodejs#58067Fixes: nodejs#58061 Reviewed-By: Jacob Smith <[email protected]>

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: nodejs#57187Fixes: nodejs#57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in nodejs#57187. PR-URL: nodejs#58067Fixes: nodejs#58061 Reviewed-By: Jacob Smith <[email protected]>

This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: #57187 Backport-PR-URL: #59504Fixes: #57172 Refs: shufo/prettier-plugin-blade#311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: mochajs/mocha#5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Refs: #52697

When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in #57187. PR-URL: #58067 Backport-PR-URL: #59504Fixes: #58061 Reviewed-By: Jacob Smith <[email protected]> Refs: #52697