Node: Fragile/buggy implementation of ES module loading

[iamstolis]

• Version: v8.9.4
• Platform: Linux stola-ThinkPad 3.16.0-38-generic #52~14.04.1-Ubuntu SMP Fri May 8 09:43:57 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: esm

The implementation of module loading depends on subtleties of the order of promise jobs (microtasks) in V8. In fact, I believe that some tests (like test-esm-namespace) pass because of issue 6007 in V8 only. I tried to run these tests on our alternative version of Node.js that embeds a different JavaScript engine and some of es-module tests failed with

Error: linking error, dependency promises must be resolved on instantiate
    at checkComplete (internal/loader/ModuleJob.js:75:27)
    at internal/loader/ModuleJob.js:58:11

The root of the problem seems to be in ModuleJob.js. ModuleWrap::ResolveCallback assumes/checks that promises that correspond to dependencies are resolved. Unfortunately, the callback (invoked through module.instantiate()) is triggered when moduleJob.linked promise is resolved. It is not triggered when all the promises created by ModuleWrap::Link are resolved. See the relevant part of ModuleJob.js:

    const linked = async () => {
      const dependencyJobs = [];
      ({ module: this.module,
         reflect: this.reflect } = await this.modulePromise);
      this.module.link(async (dependencySpecifier) => {
        const dependencyJobPromise =
            this.loader.getModuleJob(dependencySpecifier, url);
        dependencyJobs.push(dependencyJobPromise);
        const dependencyJob = await dependencyJobPromise;
        return (await dependencyJob.modulePromise).module;
      });
      return SafePromise.all(dependencyJobs);
    };
    this.linked = linked();

moduleJob.linked is resolved when all dependencyJobs are resolved. Unfortunately, the promises created by ModuleWrap::Link do not correspond to dependencyJobs. They correspond to the body of the async function passed to module.link. When the last dependencyJob (aka dependencyJobPromise) is resolved then SafePromise.all(dependencyJob) (aka moduleJob.linked) is resolved (and checkComplete/module.instantiate/ResolveCallback is triggered). This can happen before the whole body of the async function is finished (and the promise checked by ModuleWrap::ResolveCallback is resolved).

I think that the code should be rewritten such that there is a proper promise-based dependency (through Promise.prototype.then) between module.install and the promises used by ModuleWrap::ResolveCallback.

[targos]

/cc @nodejs/esm (I just created the team)

[bmeck]

@iamstolis await calls promise.then so not sure that would change anything. Is there a public copy of the engine having problems?

[iamstolis]

Is there a public copy of the engine having problems?

We do produce public builds but we do not have a public build of the version based on the lastest stable Node.js (v8.9.4) yet - we've just upgraded to v8.9.4 and found this issue.

Anyway, this does not seem to be a problem of our engine. It seems to be a problem of the implementation of ES module loading in Node.js hidden by a bug in V8 (as I tried to explain above). It may as well occur with V8 as soon as issue 6007 is fixed and you updgrade to a version of V8 with the fix.

Unfortunately, the specification of promises is complicated and it is hard to argue exactly about order of unchained promise jobs even for rather small examples (despite the fact that the order is defined by the specification). That's why the title of this issue starts with "Fragile/buggy" - even if I am wrong and your loading works even with issue 6007 fixed it still holds that the code is written such that it is hard to argue that it is correct (and I believe that it is not correct as it is described above). That's why I suggest to rewrite it such that there is a clear then relation between the promises created by ModuleWrap::Link and the code that invokes module.instantiate().

[devsnek]

@iamstolis i'm working on a pretty simple fix for this atm (got the idea from my work on vm.Module) i'll let you know how it turns out. (waiting for the long build now 😄)

@bmeck if we moved the resolved cache to the js it might make linking a little less hectic, any thoughts on that?

[devsnek]

@iamstolis this patch should make linked wait for all the promises to actually resolve, i think moving forward there's probably a nicer way to fix this though, i'll keep looking at it.

diff --git a/lib/internal/loader/ModuleJob.js b/lib/internal/loader/ModuleJob.js
index 8aa1b4b051..2cc83c3273 100644
--- a/lib/internal/loader/ModuleJob.js
+++ b/lib/internal/loader/ModuleJob.js
@@ -31,13 +31,19 @@ class ModuleJob {
       ({ module: this.module,
          reflect: this.reflect } = await this.modulePromise);
       assert(this.module instanceof ModuleWrap);
-      this.module.link(async (dependencySpecifier) => {
-        const dependencyJobPromise =
+      const promises = [];
+      this.module.link((dependencySpecifier) => {
+        const p = (async () => {
+          const dependencyJobPromise =
             this.loader.getModuleJob(dependencySpecifier, url);
-        dependencyJobs.push(dependencyJobPromise);
-        const dependencyJob = await dependencyJobPromise;
-        return (await dependencyJob.modulePromise).module;
+          dependencyJobs.push(dependencyJobPromise);
+          const dependencyJob = await dependencyJobPromise;
+          return (await dependencyJob.modulePromise).module;
+        })();
+        promises.push(p);
+        return p;
       });
+      await Promise.all(promises);
       if (enableDebug) {
         // Make sure all dependencies are entered into the list synchronously.
         Object.freeze(dependencyJobs);

[iamstolis]

@devsnek, thanks for the quick suggestion of a possible patch. It looks correct to me (i.e. it introduces a proper dependency between the problematic promises). I've applied the patch to our fork and I can confirm that all es-module tests are passing now (test-esm-namespace, test-esm-require-cache, test-esm-snapshot were failing before).

[jdalton]

@iamstolis Nice find! This will likely help other non-V8 engines as well.

[devsnek]

just to keep people up to date, the current plan i have is to remove ModuleWrap::Link after vm.Module lands, and the new implementation i have will take care of this bug as a side-effect

[jdalton]

Cool!

I think this PR could land as is without waiting for vm.Module, which is a much larger chunk.

Update:

https://github.com/nodejs/node/pull/18394 landed.

[TimothyGu]

Fixed in #18394 and #18509.