Skip to content

Commit

Permalink
module: cache synchronous module jobs before linking
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
joyeecheung authored and targos committed May 11, 2024
1 parent 1f8eaec commit f74beb5
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 14 deletions.
39 changes: 25 additions & 14 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,22 +295,33 @@ class ModuleJobSync extends ModuleJobBase {
#loader = null;
constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) {
super(url, importAttributes, moduleWrap, isMain, inspectBrk, true);
assert(this.module instanceof ModuleWrap);
this.#loader = loader;
const moduleRequests = this.module.getModuleRequests();
// Specifiers should be aligned with the moduleRequests array in order.
const specifiers = Array(moduleRequests.length);
const modules = Array(moduleRequests.length);
const jobs = Array(moduleRequests.length);
for (let i = 0; i < moduleRequests.length; ++i) {
const { specifier, attributes } = moduleRequests[i];
const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
specifiers[i] = specifier;
modules[i] = job.module;
jobs[i] = job;

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

try {
const moduleRequests = this.module.getModuleRequests();
// Specifiers should be aligned with the moduleRequests array in order.
const specifiers = Array(moduleRequests.length);
const modules = Array(moduleRequests.length);
const jobs = Array(moduleRequests.length);
for (let i = 0; i < moduleRequests.length; ++i) {
const { specifier, attributes } = moduleRequests[i];
const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
specifiers[i] = specifier;
modules[i] = job.module;
jobs[i] = job;
}
this.module.link(specifiers, modules);
this.linked = jobs;
} finally {
// Restore it - if it succeeds, we'll reset in the caller; Otherwise it's
// not cached and if the error is caught, subsequent attempt would still fail.
loader.loadCache.delete(url, importAttributes.type);
}
this.module.link(specifiers, modules);
this.linked = jobs;
}

get modulePromise() {
Expand Down
6 changes: 6 additions & 0 deletions lib/internal/modules/esm/module_map.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ class LoadCache extends SafeMap {
validateString(type, 'type');
return super.get(url)?.[type] !== undefined;
}
delete(url, type = kImplicitAssertType) {
const cached = super.get(url);
if (cached) {
cached[type] = undefined;
}
}
}

module.exports = {
Expand Down
8 changes: 8 additions & 0 deletions test/es-module/test-require-module-cycle-cjs-esm-esm.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Flags: --experimental-require-module
'use strict';

// This tests that ESM <-> ESM cycle is allowed in a require()-d graph.
const common = require('../common');
const cycle = require('../fixtures/es-modules/cjs-esm-esm-cycle/c.cjs');

common.expectRequiredModule(cycle, { b: 5 });
1 change: 1 addition & 0 deletions test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { b } from './b.mjs';
2 changes: 2 additions & 0 deletions test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import './a.mjs'
export const b = 5;
1 change: 1 addition & 0 deletions test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = require('./a.mjs');

0 comments on commit f74beb5

Please sign in to comment.