From 311d1c356ccf8dc649e1ba0d4614929989b1ab58 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 26 Jul 2023 22:34:20 +0200 Subject: [PATCH] module: ensure successful import returns the same result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/46662 Reviewed-By: Michaël Zasso Reviewed-By: Geoffrey Booth --- benchmark/esm/esm-loader-import.js | 45 ++++++++++ lib/internal/modules/esm/loader.js | 37 +++++--- lib/internal/modules/esm/module_job.js | 6 +- lib/internal/modules/esm/module_map.js | 89 +++++++++++++++++-- .../test-esm-dynamic-import-mutating-fs.js | 25 ++++++ .../test-esm-dynamic-import-mutating-fs.mjs | 42 +++++++++ test/es-module/test-esm-loader-modulemap.js | 38 +++++--- 7 files changed, 255 insertions(+), 27 deletions(-) create mode 100644 benchmark/esm/esm-loader-import.js create mode 100644 test/es-module/test-esm-dynamic-import-mutating-fs.js create mode 100644 test/es-module/test-esm-dynamic-import-mutating-fs.mjs diff --git a/benchmark/esm/esm-loader-import.js b/benchmark/esm/esm-loader-import.js new file mode 100644 index 00000000000000..9967cd95275469 --- /dev/null +++ b/benchmark/esm/esm-loader-import.js @@ -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); +} diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d1cc9da3c74baf..93ab847bf979d0 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -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() { @@ -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 @@ -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 { @@ -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) { @@ -277,7 +287,7 @@ class ModuleLoader { inspectBrk, ); - this.moduleMap.set(url, importAssertions.type, job); + this.loadCache.set(url, importAssertions.type, job); return job; } @@ -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; } /** diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index cb1654812f9d82..35185c64a7d08f 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -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'); @@ -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 { diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index ac6d95445ae757..12a1a526178a7e 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -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} 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} + */ + 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'); @@ -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); @@ -40,4 +115,8 @@ class ModuleMap extends SafeMap { return super.get(url)?.[type] !== undefined; } } -module.exports = ModuleMap; + +module.exports = { + LoadCache, + ResolveCache, +}; diff --git a/test/es-module/test-esm-dynamic-import-mutating-fs.js b/test/es-module/test-esm-dynamic-import-mutating-fs.js new file mode 100644 index 00000000000000..09cbffe487959e --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-mutating-fs.js @@ -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()); diff --git a/test/es-module/test-esm-dynamic-import-mutating-fs.mjs b/test/es-module/test-esm-dynamic-import-mutating-fs.mjs new file mode 100644 index 00000000000000..7eb79337065765 --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-mutating-fs.mjs @@ -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: '', + }); diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js index 093a4f6b3ef162..860775df0a2ce8 100644 --- a/test/es-module/test-esm-loader-modulemap.js +++ b/test/es-module/test-esm-loader-modulemap.js @@ -5,7 +5,7 @@ require('../common'); const { strictEqual, throws } = require('assert'); const { createModuleLoader } = require('internal/modules/esm/loader'); -const ModuleMap = require('internal/modules/esm/module_map'); +const { LoadCache, ResolveCache } = require('internal/modules/esm/module_map'); const ModuleJob = require('internal/modules/esm/module_job'); const createDynamicModule = require( 'internal/modules/esm/create_dynamic_module'); @@ -24,11 +24,11 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, () => new Promise(() => {})); -// ModuleMap.set and ModuleMap.get store and retrieve module jobs for a -// specified url/type tuple; ModuleMap.has correctly reports whether such jobs +// LoadCache.set and LoadCache.get store and retrieve module jobs for a +// specified url/type tuple; LoadCache.has correctly reports whether such jobs // are stored in the map. { - const moduleMap = new ModuleMap(); + const moduleMap = new LoadCache(); moduleMap.set(jsModuleDataUrl, undefined, jsModuleJob); moduleMap.set(jsonModuleDataUrl, 'json', jsonModuleJob); @@ -50,10 +50,10 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, strictEqual(moduleMap.has(jsonModuleDataUrl, 'unknown'), false); } -// ModuleMap.get, ModuleMap.has and ModuleMap.set should only accept string +// LoadCache.get, LoadCache.has and LoadCache.set should only accept string // values as url argument. { - const moduleMap = new ModuleMap(); + const moduleMap = new LoadCache(); const errorObj = { code: 'ERR_INVALID_ARG_TYPE', @@ -68,10 +68,10 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); } -// ModuleMap.get, ModuleMap.has and ModuleMap.set should only accept string +// LoadCache.get, LoadCache.has and LoadCache.set should only accept string // values (or the kAssertType symbol) as type argument. { - const moduleMap = new ModuleMap(); + const moduleMap = new LoadCache(); const errorObj = { code: 'ERR_INVALID_ARG_TYPE', @@ -86,9 +86,9 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); } -// ModuleMap.set should only accept ModuleJob values as job argument. +// LoadCache.set should only accept ModuleJob values as job argument. { - const moduleMap = new ModuleMap(); + const moduleMap = new LoadCache(); [{}, [], true, 1].forEach((value) => { throws(() => moduleMap.set('', undefined, value), { @@ -98,3 +98,21 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); }); } + +{ + const resolveMap = new ResolveCache(); + + strictEqual(resolveMap.serializeKey('./file', { __proto__: null }), './file::'); + strictEqual(resolveMap.serializeKey('./file', { __proto__: null, type: 'json' }), './file::"type""json"'); + strictEqual(resolveMap.serializeKey('./file::"type""json"', { __proto__: null }), './file::"type""json"::'); + strictEqual(resolveMap.serializeKey('./file', { __proto__: null, c: 'd', a: 'b' }), './file::"a""b","c""d"'); + strictEqual(resolveMap.serializeKey('./s', { __proto__: null, c: 'd', a: 'b', b: 'c' }), './s::"a""b","b""c","c""d"'); + + resolveMap.set('key1', 'parent1', 1); + resolveMap.set('key2', 'parent1', 2); + resolveMap.set('key2', 'parent2', 3); + + strictEqual(resolveMap.get('key1', 'parent1'), 1); + strictEqual(resolveMap.get('key2', 'parent1'), 2); + strictEqual(resolveMap.get('key2', 'parent2'), 3); +}