module: ensure successful import returns the same result

PR-URL: nodejs#46662 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
Ceres6 · Aug 14, 2023 · f167a65 · f167a65
1 parent 0ccb1d0
commit f167a65
Show file tree

Hide file tree

Showing 7 changed files with 255 additions and 27 deletions.
diff --git a/benchmark/esm/esm-loader-import.js 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
@@ -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
@@ -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
@@ -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');
@@ -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
@@ -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: '',
+  });