module: ensure successful import returns the same result

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<string, string>} importAssertions Validations for the
-   *                                                  module import.
+   * @param {Record<string, string>} 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,23 +185,23 @@ 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<string, string>} importAssertions Validations for the
+   * @param {Record<string, string>} 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
    * @param {string} [format] The format hint possibly returned by the
    *                          `resolve` hook
    * @returns {Promise<ModuleJob>} The (possibly pending) module job
    */
-  #createModuleJob(url, importAssertions, parentURL, format) {
+  #createModuleJob(url, importAttributes, parentURL, format) {
     const moduleProvider = async (url, isMain) => {
       const {
         format: finalFormat,
         responseURL,
         source,
       } = await this.load(url, {
         format,
-        importAssertions,
+        importAttributes,
       });
 
       const translator = getTranslators().get(finalFormat);
@@ -218,69 +226,95 @@ 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.
    *
    * 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<string, string>} importAssertions Validations for the
+   * @param {Record<string, string>} importAttributes Validations for the
    *                                                  module import.
-   * @returns {Promise<ExportedHooks | KeyedExports[]>}
+   * @returns {Promise<ExportedHooks>}
    *  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() {

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() {

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

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