vm: fix link with invalid graph

This fixes an annoying bug where the entry module would link before its dependencies were linked. This is fixed by forcing modules to wait for all dependency linker promises to resolve. Fixes: nodejs#37426 Signed-off-by: snek <[email protected]>
devsnek · Mar 4, 2021 · fee4bc4 · fee4bc4
1 parent 3b2863d
commit fee4bc4
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 18 deletions.
diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js
@@ -255,6 +255,7 @@ const kNoError = Symbol('kNoError');
 class SourceTextModule extends Module {
   #error = kNoError;
   #statusOverride;
+  #waitingLinkPromises;
 
   constructor(sourceText, options = {}) {
     validateString(sourceText, 'sourceText');
@@ -308,31 +309,37 @@ class SourceTextModule extends Module {
     this[kLink] = async (linker) => {
       this.#statusOverride = 'linking';
 
-      const promises = this[kWrap].link(async (identifier) => {
-        const module = await linker(identifier, this);
-        if (module[kWrap] === undefined) {
-          throw new ERR_VM_MODULE_NOT_MODULE();
-        }
-        if (module.context !== this.context) {
-          throw new ERR_VM_MODULE_DIFFERENT_CONTEXT();
-        }
-        if (module.status === 'errored') {
-          throw new ERR_VM_MODULE_LINKING_ERRORED();
-        }
-        if (module.status === 'unlinked') {
-          await module[kLink](linker);
-        }
-        return module[kWrap];
-      });
-
       try {
+        const promises = this[kWrap].link(async (identifier) => {
+          const module = await linker(identifier, this);
+          if (module[kWrap] === undefined) {
+            throw new ERR_VM_MODULE_NOT_MODULE();
+          }
+          if (module.context !== this.context) {
+            throw new ERR_VM_MODULE_DIFFERENT_CONTEXT();
+          }
+          if (module.status === 'errored') {
+            throw new ERR_VM_MODULE_LINKING_ERRORED();
+          }
+          if (module.status === 'unlinked') {
+            await module[kLink](linker);
+          }
+          if (module.status === 'linking') {
+            await module.#waitingLinkPromises;
+          }
+          return module[kWrap];
+        });
+
         if (promises !== undefined) {
-          await PromiseAll(promises);
+          assert(this.#waitingLinkPromises === undefined);
+          this.#waitingLinkPromises = PromiseAll(promises);
         }
+        await this.#waitingLinkPromises;
       } catch (e) {
         this.#error = e;
         throw e;
       } finally {
+        this.#waitingLinkPromises = undefined;
         this.#statusOverride = undefined;
       }
     };

diff --git a/test.js b/test.js
@@ -0,0 +1,24 @@
+'use strict';
+
+const { SourceTextModule, SyntheticModule } = require('vm');
+
+const tm = new SourceTextModule('import "a"');
+
+tm
+  .link(async () => {
+    const tm2 = new SourceTextModule('import "b"');
+    tm2.link(async () => {
+      const sm = new SyntheticModule([], () => {});
+      await sm.link(() => {});
+      await sm.evaluate();
+      return sm;
+    }).then(() => tm2.evaluate());
+    return tm2;
+  })
+  .then(() => tm.evaluate())
+  .then(() => {
+    console.log('dun');
+  })
+  .catch((e) => {
+    console.error(e);
+  });
diff --git a/test/parallel/test-vm-module-link-37426.js b/test/parallel/test-vm-module-link-37426.js
@@ -0,0 +1,23 @@
+// Flags: --experimental-vm-modules
+
+'use strict';
+
+const common = require('../common');
+const { SourceTextModule, SyntheticModule } = require('vm');
+
+const tm = new SourceTextModule('import "a"');
+
+tm
+  .link(async () => {
+    const tm2 = new SourceTextModule('import "b"');
+    tm2.link(async () => {
+      const sm = new SyntheticModule([], () => {});
+      await sm.link(() => {});
+      await sm.evaluate();
+      return sm;
+    }).then(() => tm2.evaluate());
+    return tm2;
+  })
+  .then(() => tm.evaluate())
+  .then(common.mustCall())
+  .catch(common.mustNotCall());