From 9bf820c4668419fd9c4d7eea6c898c72662a2c60 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 14 Apr 2023 18:09:49 +0200 Subject: [PATCH] module: ensure successful import returns the same result --- lib/internal/modules/esm/loader.js | 166 +++++++++++------- lib/internal/modules/esm/module_job.js | 17 +- lib/internal/modules/esm/utils.js | 12 +- .../test-esm-dynamic-import-mutating-fs.js | 25 +++ .../test-esm-dynamic-import-mutating-fs.mjs | 42 +++++ 5 files changed, 181 insertions(+), 81 deletions(-) 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/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index a42319dba892c82..e9923ca2ababbd0 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -4,12 +4,16 @@ require('internal/modules/cjs/loader'); const { - Array, - ArrayIsArray, + ArrayPrototypeJoin, + ArrayPrototypeMap, + ArrayPrototypeSort, FunctionPrototypeCall, + JSONStringify, + ObjectKeys, ObjectSetPrototypeOf, PromisePrototypeThen, - SafePromiseAllReturnArrayLike, + SafeMap, + PromiseResolve, SafeWeakMap, } = primordials; @@ -78,6 +82,11 @@ class DefaultModuleLoader { */ #defaultConditions = getDefaultConditions(); + /** + * Import cache + */ + #importCache = new SafeMap(); + /** * Map of already-loaded CJS modules to use */ @@ -119,8 +128,8 @@ class DefaultModuleLoader { const { setCallbackForWrap } = require('internal/modules/esm/utils'); const module = new ModuleWrap(url, undefined, source, 0, 0); setCallbackForWrap(module, { - importModuleDynamically: (specifier, { url }, importAssertions) => { - return this.import(specifier, url, importAssertions); + importModuleDynamically: (specifier, { url }, importAttributes) => { + return this.import(specifier, url, importAttributes); }, }); @@ -147,18 +156,17 @@ class DefaultModuleLoader { * @param {string | undefined} parentURL The URL of the module importing this * one, unless this is the Node.js entry * point. - * @param {Record} importAssertions Validations for the - * module import. + * @param {Record} importAttributes The import attributes. * @returns {ModuleJob} The (possibly pending) module job */ - getModuleJob(specifier, parentURL, importAssertions) { - const resolveResult = this.resolve(specifier, parentURL, importAssertions); - return this.getJobFromResolveResult(resolveResult, parentURL, importAssertions); + getModuleJob(specifier, parentURL, importAttributes) { + const resolveResult = this.resolve(specifier, parentURL, importAttributes); + return this.getJobFromResolveResult(resolveResult, parentURL, importAttributes); } - getJobFromResolveResult(resolveResult, parentURL, importAssertions) { + getJobFromResolveResult(resolveResult, parentURL, importAttributes) { const { url, format } = resolveResult; - const resolvedImportAssertions = resolveResult.importAssertions ?? importAssertions; + const resolvedImportAssertions = resolveResult.importAttributes ?? importAttributes; let job = this.moduleMap.get(url, resolvedImportAssertions.type); @@ -177,7 +185,7 @@ class DefaultModuleLoader { /** * Create and cache an object representing a loaded module. * @param {string} url The absolute URL that was resolved for this module - * @param {Record} importAssertions Validations for the + * @param {Record} importAttributes Validations for the * module import. * @param {string} [parentURL] The absolute URL of the module importing this * one, unless this is the Node.js entry point @@ -185,7 +193,7 @@ class DefaultModuleLoader { * `resolve` hook * @returns {Promise} The (possibly pending) module job */ - #createModuleJob(url, importAssertions, parentURL, format) { + #createModuleJob(url, importAttributes, parentURL, format) { const moduleProvider = async (url, isMain) => { const { format: finalFormat, @@ -193,7 +201,7 @@ class DefaultModuleLoader { source, } = await this.load(url, { format, - importAssertions, + importAttributes, }); const translator = getTranslators().get(finalFormat); @@ -218,17 +226,72 @@ class DefaultModuleLoader { const job = new ModuleJob( this, url, - importAssertions, + importAttributes, moduleProvider, parentURL === undefined, inspectBrk, ); - this.moduleMap.set(url, importAssertions.type, job); + this.moduleMap.set(url, importAttributes.type, job); return job; } + #serializeCache(specifier, parentURL, importAttributes) { + let cache = this.#importCache.get(parentURL); + let specifierCache; + if (cache == null) { + this.#importCache.set(parentURL, cache = new SafeMap()); + } else { + specifierCache = cache.get(specifier); + } + + if (specifierCache == null) { + cache.set(specifier, specifierCache = { __proto__: null }); + } + + const serializedAttributes = ArrayPrototypeJoin( + ArrayPrototypeMap( + ArrayPrototypeSort(ObjectKeys(importAttributes)), + (key) => JSONStringify(key) + JSONStringify(importAttributes[key])), + ','); + + return { specifierCache, serializedAttributes }; + } + + cacheStatic(specifier, parentURL, importAttributes, result) { + const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAttributes); + + specifierCache[serializedAttributes] = result; + } + + async import(specifier, parentURL, importAttributes) { + const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAttributes); + const removeCache = () => { + delete specifierCache[serializedAttributes]; + }; + if (specifierCache[serializedAttributes] != null) { + if (PromiseResolve(specifierCache[serializedAttributes]) !== specifierCache[serializedAttributes]) { + const { module } = await specifierCache[serializedAttributes].run(); + return module.getNamespace(); + } + const fallback = () => { + if (specifierCache[serializedAttributes] != null) { + return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + } + const result = this.#import(specifier, parentURL, importAttributes); + specifierCache[serializedAttributes] = result; + PromisePrototypeThen(result, undefined, removeCache); + return result; + }; + return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + } + const result = this.#import(specifier, parentURL, importAttributes); + specifierCache[serializedAttributes] = result; + PromisePrototypeThen(result, undefined, removeCache); + return result; + } + /** * This method is usually called indirectly as part of the loading processes. * Internally, it is used directly to add loaders. Use directly with caution. @@ -236,51 +299,22 @@ class DefaultModuleLoader { * This method must NOT be renamed: it functions as a dynamic import on a * loader module. * - * @param {string | string[]} specifiers Path(s) to the module. + * @param {string} specifier The first parameter of an `import()` expression. * @param {string} parentURL Path of the parent importing the module. - * @param {Record} importAssertions Validations for the + * @param {Record} importAttributes Validations for the * module import. - * @returns {Promise} + * @returns {Promise} * A collection of module export(s) or a list of collections of module * export(s). */ - async import(specifiers, parentURL, importAssertions) { - // For loaders, `import` is passed multiple things to process, it returns a - // list pairing the url and exports collected. This is especially useful for - // error messaging, to identity from where an export came. But, in most - // cases, only a single url is being "imported" (ex `import()`), so there is - // only 1 possible url from which the exports were collected and it is - // already known to the caller. Nesting that in a list would only ever - // create redundant work for the caller, so it is later popped off the - // internal list. - const wasArr = ArrayIsArray(specifiers); - if (!wasArr) { specifiers = [specifiers]; } - - const count = specifiers.length; - const jobs = new Array(count); - - for (let i = 0; i < count; i++) { - jobs[i] = PromisePrototypeThen( - this - .getModuleJob(specifiers[i], parentURL, importAssertions) - .run(), - ({ module }) => module.getNamespace(), - ); - } - - const namespaces = await SafePromiseAllReturnArrayLike(jobs); + async #import(specifier, parentURL, importAttributes) { - if (!wasArr) { return namespaces[0]; } // We can skip the pairing below - - for (let i = 0; i < count; i++) { - namespaces[i] = { - __proto__: null, - url: specifiers[i], - exports: namespaces[i], - }; - } + const moduleJob = this.getModuleJob(specifier, parentURL, importAttributes); + const { module } = await moduleJob.run(); - return namespaces; + const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAttributes); + specifierCache[serializedAttributes] = moduleJob; + return module.getNamespace(); } /** @@ -289,17 +323,17 @@ class DefaultModuleLoader { * @param {string} originalSpecifier The specified URL path of the module to * be resolved. * @param {string} [parentURL] The URL path of the module's parent. - * @param {ImportAssertions} importAssertions Assertions from the import + * @param {ImportAssertions} importAttributes Assertions from the import * statement or expression. * @returns {{ format: string, url: URL['href'] }} */ - resolve(originalSpecifier, parentURL, importAssertions) { + resolve(originalSpecifier, parentURL, importAttributes) { defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve; const context = { __proto__: null, conditions: this.#defaultConditions, - importAssertions, + importAttributes, parentURL, }; @@ -357,21 +391,21 @@ class CustomizedModuleLoader extends DefaultModuleLoader { * @param {string} originalSpecifier The specified URL path of the module to * be resolved. * @param {string} [parentURL] The URL path of the module's parent. - * @param {ImportAssertions} importAssertions Assertions from the import + * @param {ImportAssertions} importAttributes Assertions from the import * statement or expression. * @returns {{ format: string, url: URL['href'] }} */ - resolve(originalSpecifier, parentURL, importAssertions) { - return hooksProxy.makeSyncRequest('resolve', originalSpecifier, parentURL, importAssertions); + resolve(originalSpecifier, parentURL, importAttributes) { + return hooksProxy.makeSyncRequest('resolve', originalSpecifier, parentURL, importAttributes); } - async #getModuleJob(specifier, parentURL, importAssertions) { - const resolveResult = await hooksProxy.makeAsyncRequest('resolve', specifier, parentURL, importAssertions); + async #getModuleJob(specifier, parentURL, importAttributes) { + const resolveResult = await hooksProxy.makeAsyncRequest('resolve', specifier, parentURL, importAttributes); - return this.getJobFromResolveResult(resolveResult, parentURL, importAssertions); + return this.getJobFromResolveResult(resolveResult, parentURL, importAttributes); } - getModuleJob(specifier, parentURL, importAssertions) { - const jobPromise = this.#getModuleJob(specifier, parentURL, importAssertions); + getModuleJob(specifier, parentURL, importAttributes) { + const jobPromise = this.#getModuleJob(specifier, parentURL, importAttributes); return { run() { diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 2cf2813a6dcf7f1..35f768936b8181f 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -50,10 +50,10 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) => class ModuleJob { // `loader` is the Loader instance used for loading dependencies. // `moduleProvider` is a function - constructor(loader, url, importAssertions = { __proto__: null }, + constructor(loader, url, importAttributes = { __proto__: null }, moduleProvider, isMain, inspectBrk) { this.loader = loader; - this.importAssertions = importAssertions; + this.importAttributes = importAttributes; this.isMain = isMain; this.inspectBrk = inspectBrk; @@ -72,10 +72,12 @@ class ModuleJob { // so that circular dependencies can't cause a deadlock by two of // these `link` callbacks depending on each other. const dependencyJobs = []; - const promises = this.module.link((specifier, assertions) => { - const job = this.loader.getModuleJob(specifier, url, assertions); + const promises = this.module.link(async (specifier, attributes) => { + const job = this.loader.getModuleJob(specifier, url, attributes); ArrayPrototypePush(dependencyJobs, job); - return job.modulePromise; + const result = await job.modulePromise; + this.loader.cacheStatic(specifier, url, attributes, job); + return result; }); if (promises !== undefined) @@ -95,10 +97,7 @@ class ModuleJob { } instantiate() { - if (this.instantiated === undefined) { - this.instantiated = this._instantiate(); - } - return this.instantiated; + return this.instantiated ??= this._instantiate(); } async _instantiate() { diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index 1c01dd13b3ab211..7a3da3d87114c24 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -117,12 +117,12 @@ async function initializeHooks() { const { DefaultModuleLoader } = require('internal/modules/esm/loader'); class ModuleLoader extends DefaultModuleLoader { loaderType = 'internal'; - async #getModuleJob(specifier, parentURL, importAssertions) { - const resolveResult = await hooks.resolve(specifier, parentURL, importAssertions); - return this.getJobFromResolveResult(resolveResult, parentURL, importAssertions); + async #getModuleJob(specifier, parentURL, importAttributes) { + const resolveResult = await hooks.resolve(specifier, parentURL, importAttributes); + return this.getJobFromResolveResult(resolveResult, parentURL, importAttributes); } - getModuleJob(specifier, parentURL, importAssertions) { - const jobPromise = this.#getModuleJob(specifier, parentURL, importAssertions); + getModuleJob(specifier, parentURL, importAttributes) { + const jobPromise = this.#getModuleJob(specifier, parentURL, importAttributes); return { run() { return PromisePrototypeThen(jobPromise, (job) => job.run()); @@ -146,7 +146,7 @@ async function initializeHooks() { // Importation must be handled by internal loader to avoid polluting user-land const keyedExportsSublist = await privateModuleLoader.import( - [customLoaderPath], // Import can handle multiple paths, but custom loaders must be sequential + customLoaderPath, parentURL, kEmptyObject, ); 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 000000000000000..b4c1726c0fa3b24 --- /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('./target.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 000000000000000..ca8589e1b9ab937 --- /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('./target.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: '', + });