Skip to content

Commit

Permalink
module: ensure successful import returns the same result
Browse files Browse the repository at this point in the history
PR-URL: nodejs#46662
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
  • Loading branch information
aduh95 authored and rluvaton committed Jul 28, 2023
1 parent dbed52a commit 311d1c3
Show file tree
Hide file tree
Showing 7 changed files with 255 additions and 27 deletions.
45 changes: 45 additions & 0 deletions benchmark/esm/esm-loader-import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Tests the impact on eager operations required for policies affecting
// general startup, does not test lazy operations
'use strict';
const fs = require('node:fs');
const path = require('node:path');
const common = require('../common.js');

const tmpdir = require('../../test/common/tmpdir.js');
const { pathToFileURL } = require('node:url');

const benchmarkDirectory = pathToFileURL(path.resolve(tmpdir.path, 'benchmark-import'));

const configs = {
n: [1e3],
specifier: [
'data:text/javascript,{i}',
'./relative-existing.js',
'./relative-nonexistent.js',
'node:prefixed-nonexistent',
'node:os',
],
};

const options = {
flags: ['--expose-internals'],
};

const bench = common.createBenchmark(main, configs, options);

async function main(conf) {
tmpdir.refresh();

fs.mkdirSync(benchmarkDirectory, { recursive: true });
fs.writeFileSync(new URL('./relative-existing.js', benchmarkDirectory), '\n');

bench.start();

for (let i = 0; i < conf.n; i++) {
try {
await import(new URL(conf.specifier.replace('{i}', i), benchmarkDirectory));
} catch { /* empty */ }
}

bench.end(conf.n);
}
37 changes: 27 additions & 10 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,14 @@ const {
} = require('internal/modules/esm/utils');
let defaultResolve, defaultLoad, importMetaInitializer;

function newModuleMap() {
const ModuleMap = require('internal/modules/esm/module_map');
return new ModuleMap();
function newResolveCache() {
const { ResolveCache } = require('internal/modules/esm/module_map');
return new ResolveCache();
}

function newLoadCache() {
const { LoadCache } = require('internal/modules/esm/module_map');
return new LoadCache();
}

function getTranslators() {
Expand Down Expand Up @@ -71,10 +76,15 @@ class ModuleLoader {
*/
evalIndex = 0;

/**
* Registry of resolved specifiers
*/
#resolveCache = newResolveCache();

/**
* Registry of loaded modules, akin to `require.cache`
*/
moduleMap = newModuleMap();
loadCache = newLoadCache();

/**
* Methods which translate input code or other information into ES modules
Expand Down Expand Up @@ -183,7 +193,7 @@ class ModuleLoader {
const ModuleJob = require('internal/modules/esm/module_job');
const job = new ModuleJob(
this, url, undefined, evalInstance, false, false);
this.moduleMap.set(url, undefined, job);
this.loadCache.set(url, undefined, job);
const { module } = await job.run();

return {
Expand Down Expand Up @@ -213,11 +223,11 @@ class ModuleLoader {
getJobFromResolveResult(resolveResult, parentURL, importAssertions) {
const { url, format } = resolveResult;
const resolvedImportAssertions = resolveResult.importAssertions ?? importAssertions;
let job = this.moduleMap.get(url, resolvedImportAssertions.type);
let job = this.loadCache.get(url, resolvedImportAssertions.type);

// CommonJS will set functions for lazy job evaluation.
if (typeof job === 'function') {
this.moduleMap.set(url, undefined, job = job());
this.loadCache.set(url, undefined, job = job());
}

if (job === undefined) {
Expand Down Expand Up @@ -277,7 +287,7 @@ class ModuleLoader {
inspectBrk,
);

this.moduleMap.set(url, importAssertions.type, job);
this.loadCache.set(url, importAssertions.type, job);

return job;
}
Expand Down Expand Up @@ -315,13 +325,20 @@ class ModuleLoader {
* @param {string} [parentURL] The URL path of the module's parent.
* @param {ImportAssertions} importAssertions Assertions from the import
* statement or expression.
* @returns {Promise<{ format: string, url: URL['href'] }>}
* @returns {{ format: string, url: URL['href'] }}
*/
resolve(originalSpecifier, parentURL, importAssertions) {
if (this.#customizations) {
return this.#customizations.resolve(originalSpecifier, parentURL, importAssertions);
}
return this.defaultResolve(originalSpecifier, parentURL, importAssertions);
const requestKey = this.#resolveCache.serializeKey(originalSpecifier, importAssertions);
const cachedResult = this.#resolveCache.get(requestKey, parentURL);
if (cachedResult != null) {
return cachedResult;
}
const result = this.defaultResolve(originalSpecifier, parentURL, importAssertions);
this.#resolveCache.set(requestKey, parentURL, result);
return result;
}

/**
Expand Down
6 changes: 4 additions & 2 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const {

const { ModuleWrap } = internalBinding('module_wrap');

const { decorateErrorStack } = require('internal/util');
const { decorateErrorStack, kEmptyObject } = require('internal/util');
const {
getSourceMapsEnabled,
} = require('internal/source_map/source_map_cache');
Expand Down Expand Up @@ -140,7 +140,9 @@ class ModuleJob {
/module '(.*)' does not provide an export named '(.+)'/,
e.message);
const { url: childFileURL } = await this.loader.resolve(
childSpecifier, parentFileUrl,
childSpecifier,
parentFileUrl,
kEmptyObject,
);
let format;
try {
Expand Down
89 changes: 84 additions & 5 deletions lib/internal/modules/esm/module_map.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,92 @@
'use strict';

const { kImplicitAssertType } = require('internal/modules/esm/assert');
const {
ArrayPrototypeJoin,
ArrayPrototypeMap,
ArrayPrototypeSort,
JSONStringify,
ObjectKeys,
SafeMap,
} = primordials;
const { kImplicitAssertType } = require('internal/modules/esm/assert');
let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
debug = fn;
});
const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes;
const { validateString } = require('internal/validators');

// Tracks the state of the loader-level module cache
class ModuleMap extends SafeMap {
/**
* Cache the results of the `resolve` step of the module resolution and loading process.
* Future resolutions of the same input (specifier, parent URL and import assertions)
* must return the same result if the first attempt was successful, per
* https://tc39.es/ecma262/#sec-HostLoadImportedModule.
* This cache is *not* used when custom loaders are registered.
*/
class ResolveCache extends SafeMap {
constructor(i) { super(i); } // eslint-disable-line no-useless-constructor

/**
* Generates the internal serialized cache key and returns it along the actual cache object.
*
* It is exposed to allow more efficient read and overwrite a cache entry.
* @param {string} specifier
* @param {Record<string,string>} importAssertions
* @returns {string}
*/
serializeKey(specifier, importAssertions) {
// To serialize the ModuleRequest (specifier + list of import assertions),
// we need to sort the assertions by key, then stringifying,
// so that different import statements with the same assertions are always treated
// as identical.
const keys = ObjectKeys(importAssertions);

if (keys.length === 0) {
return specifier + '::';
}

return specifier + '::' + ArrayPrototypeJoin(
ArrayPrototypeMap(
ArrayPrototypeSort(keys),
(key) => JSONStringify(key) + JSONStringify(importAssertions[key])),
',');
}

#getModuleCachedImports(parentURL) {
let internalCache = super.get(parentURL);
if (internalCache == null) {
super.set(parentURL, internalCache = { __proto__: null });
}
return internalCache;
}

/**
* @param {string} serializedKey
* @param {string} parentURL
* @returns {import('./loader').ModuleExports | Promise<import('./loader').ModuleExports>}
*/
get(serializedKey, parentURL) {
return this.#getModuleCachedImports(parentURL)[serializedKey];
}

/**
* @param {string} serializedKey
* @param {string} parentURL
* @param {{ format: string, url: URL['href'] }} result
*/
set(serializedKey, parentURL, result) {
this.#getModuleCachedImports(parentURL)[serializedKey] = result;
return this;
}

has(serializedKey, parentURL) {
return serializedKey in this.#getModuleCachedImports(parentURL);
}
}

/**
* Cache the results of the `load` step of the module resolution and loading process.
*/
class LoadCache extends SafeMap {
constructor(i) { super(i); } // eslint-disable-line no-useless-constructor
get(url, type = kImplicitAssertType) {
validateString(url, 'url');
Expand All @@ -29,7 +104,7 @@ class ModuleMap extends SafeMap {
}
debug(`Storing ${url} (${
type === kImplicitAssertType ? 'implicit type' : type
}) in ModuleMap`);
}) in ModuleLoadMap`);
const cachedJobsForUrl = super.get(url) ?? { __proto__: null };
cachedJobsForUrl[type] = job;
return super.set(url, cachedJobsForUrl);
Expand All @@ -40,4 +115,8 @@ class ModuleMap extends SafeMap {
return super.get(url)?.[type] !== undefined;
}
}
module.exports = ModuleMap;

module.exports = {
LoadCache,
ResolveCache,
};
25 changes: 25 additions & 0 deletions test/es-module/test-esm-dynamic-import-mutating-fs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';
const common = require('../common');
const tmpdir = require('../common/tmpdir');

const assert = require('node:assert');
const fs = require('node:fs/promises');
const { pathToFileURL } = require('node:url');

tmpdir.refresh();
const tmpDir = pathToFileURL(tmpdir.path);

const target = new URL(`./${Math.random()}.mjs`, tmpDir);

(async () => {

await assert.rejects(import(target), { code: 'ERR_MODULE_NOT_FOUND' });

await fs.writeFile(target, 'export default "actual target"\n');

const moduleRecord = await import(target);

await fs.rm(target);

assert.strictEqual(await import(target), moduleRecord);
})().then(common.mustCall());
42 changes: 42 additions & 0 deletions test/es-module/test-esm-dynamic-import-mutating-fs.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { spawnPromisified } from '../common/index.mjs';
import tmpdir from '../common/tmpdir.js';

import assert from 'node:assert';
import fs from 'node:fs/promises';
import { execPath } from 'node:process';
import { pathToFileURL } from 'node:url';

tmpdir.refresh();
const tmpDir = pathToFileURL(tmpdir.path);

const target = new URL(`./${Math.random()}.mjs`, tmpDir);

await assert.rejects(import(target), { code: 'ERR_MODULE_NOT_FOUND' });

await fs.writeFile(target, 'export default "actual target"\n');

const moduleRecord = await import(target);

await fs.rm(target);

assert.strictEqual(await import(target), moduleRecord);

// Add the file back, it should be deleted by the subprocess.
await fs.writeFile(target, 'export default "actual target"\n');

assert.deepStrictEqual(
await spawnPromisified(execPath, [
'--input-type=module',
'--eval',
[`import * as d from${JSON.stringify(target)};`,
'import{rm}from"node:fs/promises";',
`await rm(new URL(${JSON.stringify(target)}));`,
'import{strictEqual}from"node:assert";',
`strictEqual(JSON.stringify(await import(${JSON.stringify(target)})),JSON.stringify(d));`].join(''),
]),
{
code: 0,
signal: null,
stderr: '',
stdout: '',
});
Loading

0 comments on commit 311d1c3

Please sign in to comment.