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

Attention: 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 11 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

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

nodejs-github-bot · 2025-03-04T07:34:36Z

Landed in 0aa3ddd

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 Fixes: #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]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Feb 23, 2025

joyeecheung force-pushed the fix-module-cache branch from 758440a to de697ab Compare February 23, 2025 23:33

joyeecheung mentioned this pull request Feb 23, 2025

Mocha ERR_REQUIRE_CYCLE_MODULE on latest CJS #57172

Closed

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2025

guybedford approved these changes Feb 25, 2025

View reviewed changes

aduh95 reviewed Feb 26, 2025

View reviewed changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

aduh95 reviewed Feb 26, 2025

View reviewed changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

aduh95 reviewed Feb 26, 2025

View reviewed changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2025

fixup! module: handle cached linked async jobs in require(esm)

be776fa

joyeecheung force-pushed the fix-module-cache branch from de697ab to be776fa Compare March 3, 2025 13:20

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2025

mcollina approved these changes Mar 3, 2025

View reviewed changes

joyeecheung 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 Mar 4, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 4, 2025

nodejs-github-bot merged commit 0aa3ddd into nodejs:main Mar 4, 2025
62 checks passed

This was referenced Mar 5, 2025

Session Proposal: module customization hooks openjs-foundation/summit#442

Open

v20.19.0 proposal #57349

Open

github-actions bot mentioned this pull request Mar 12, 2025

2025-03-13, Version 23.10.0 (Current) #57424

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joyeecheung commented Feb 23, 2025 •

edited

Loading

nodejs-github-bot commented Feb 23, 2025

joyeecheung commented Feb 23, 2025 •

edited

Loading

codecov bot commented Feb 24, 2025 •

edited

Loading

nodejs-github-bot commented Feb 24, 2025

guybedford left a comment

nodejs-github-bot commented Mar 3, 2025

nodejs-github-bot commented Mar 3, 2025

mcollina left a comment

nodejs-github-bot commented Mar 4, 2025

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

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

Conversation

joyeecheung commented Feb 23, 2025 • edited Loading

nodejs-github-bot commented Feb 23, 2025

joyeecheung commented Feb 23, 2025 • edited Loading

codecov bot commented Feb 24, 2025 • edited Loading

Codecov Report

nodejs-github-bot commented Feb 24, 2025

guybedford left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 3, 2025

nodejs-github-bot commented Mar 3, 2025

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 4, 2025

joyeecheung commented Feb 23, 2025 •

edited

Loading

joyeecheung commented Feb 23, 2025 •

edited

Loading

codecov bot commented Feb 24, 2025 •

edited

Loading