module: ensure successful import returns the same result

nodejs · Jun 10, 2023 · cc1b1b8 · cc1b1b8
1 parent 7def7df
commit cc1b1b8
Show file tree

Hide file tree

Showing 5 changed files with 150 additions and 12 deletions.
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -4,9 +4,16 @@
 require('internal/modules/cjs/loader');
 
 const {
+  ArrayPrototypeJoin,
+  ArrayPrototypeMap,
+  ArrayPrototypeSort,
   FunctionPrototypeCall,
+  JSONStringify,
+  ObjectKeys,
   ObjectSetPrototypeOf,
   PromisePrototypeThen,
+  SafeMap,
+  PromiseResolve,
   SafeWeakMap,
 } = primordials;
 
@@ -75,6 +82,11 @@ class DefaultModuleLoader {
    */
   #defaultConditions = getDefaultConditions();
 
+  /**
+   * Import cache
+   */
+  #importCache = new SafeMap();
+
   /**
    * Map of already-loaded CJS modules to use
    */
@@ -145,8 +157,7 @@ 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>} importAssertions The import attributes.
    * @returns {ModuleJob} The (possibly pending) module job
    */
   getModuleJob(specifier, parentURL, importAssertions) {
@@ -227,6 +238,34 @@ class DefaultModuleLoader {
     return job;
   }
 
+  #serializeCache(specifier, parentURL, importAssertions) {
+    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(importAssertions)),
+        (key) => JSONStringify(key) + JSONStringify(importAssertions[key])),
+      ',');
+
+    return { specifierCache, serializedAttributes };
+  }
+
+  cacheStatic(specifier, parentURL, importAssertions, result) {
+    const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions);
+
+    specifierCache[serializedAttributes] = 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.
@@ -237,13 +276,43 @@ class DefaultModuleLoader {
    * @param {string} parentURL Path of the parent importing the module.
    * @param {Record<string, string>} importAssertions 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(specifier, parentURL, importAssertions) {
+    const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions);
+    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, importAssertions);
+        specifierCache[serializedAttributes] = result;
+        PromisePrototypeThen(result, undefined, removeCache);
+        return result;
+      };
+      return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback);
+    }
+    const result = this.#import(specifier, parentURL, importAssertions);
+    specifierCache[serializedAttributes] = result;
+    PromisePrototypeThen(result, undefined, removeCache);
+    return result;
+  }
+
+  async #import(specifier, parentURL, importAssertions) {
     const moduleJob = this.getModuleJob(specifier, parentURL, importAssertions);
     const { module } = await moduleJob.run();
+
+    const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions);
+    specifierCache[serializedAttributes] = moduleJob;
     return module.getNamespace();
   }
 

diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -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)

diff --git a/test/es-module/test-esm-dynamic-import-mutating-fs.js 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
@@ -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-initialization.mjs b/test/es-module/test-esm-initialization.mjs
@@ -8,22 +8,22 @@ import { describe, it } from 'node:test';
 describe('ESM: ensure initialization happens only once', { concurrency: true }, () => {
   it(async () => {
     const { code, stderr, stdout } = await spawnPromisified(execPath, [
+      '--experimental-import-meta-resolve',
       '--loader',
       fixtures.fileURL('es-module-loaders', 'loader-resolve-passthru.mjs'),
       '--no-warnings',
       fixtures.path('es-modules', 'runmain.mjs'),
     ]);
 
-    // Length minus 1 because the first match is the needle.
-    const resolveHookRunCount = (stdout.match(/resolve passthru/g)?.length ?? 0) - 1;
-
     assert.strictEqual(stderr, '');
     /**
-     * resolveHookRunCount = 2:
-     * 1. fixtures/…/runmain.mjs
+     * 'resolve passthru' appears 4 times in the output:
+     * 1. fixtures/…/runmain.mjs (entry point)
      * 2. node:module (imported by fixtures/…/runmain.mjs)
+     * 3. doesnt-matter.mjs (first import.meta.resolve call)
+     * 4. doesnt-matter.mjs (second import.meta.resolve call)
      */
-    assert.strictEqual(resolveHookRunCount, 2);
+    assert.strictEqual(stdout.match(/resolve passthru/g)?.length, 4);
     assert.strictEqual(code, 0);
   });
 });