Importing modules which failed evaluation previously

[domenic]

@GeorgNeis pointed out an interesting case. The setup:

// a.mjs
import "./b.mjs";
launchTheMissiles();

// b.mjs
throw new Error("boo");

Then consider these two different scenarios using these modules:

// main1.js
import("./a.mjs");

// main2.js
import("./b.mjs");
import("./a.mjs");

// assume that the first import() finishes before the second

As currently specced, main1.js will not launch any missiles. But, main2.js will launch the missiles. (This is a mostly host-agnostic statement, but does require a host to obey some basic assumptions about HostResolveImportedModule, so that the same module record is used for b.mjs in both cases.)

This is because the module record corresponding to b.mjs will be evaluated twice: once by the import(), and then second by the import declaration. The second time, ModuleEvaluation() will be a no-op per the ES spec, so the missiles will launch.

This is somewhat surprising, especially because if we replaced the contents of b.mjs with a syntax error, we would get the error at instantiation time, which is kind of per spec required to be "cached" (or at least prevent future evaluation):

Assert: ModuleDeclarationInstantiation has already been invoked on module and successfully completed.

(inside ModuleEvaluation)

Another way it is surprising was stated by @ajklein:

just at a high level: seems strange that we guarantee that your dependencies evaluate before you (modulo samth's point about cycles) but not that your dependencies successfully evaluated

---

Is this a problem? I am leaning toward yes.

If we agree it's a problem, hosts could fix it by maintaining their own cache of module evaluation results. But as shown above, it's a fairly host-agnostic issue, so I was thinking maybe it should be solved in ES.

[ajklein]

Looping in some folks who might have context or opinions: @allenwb @dherman @caridy @bmeck

[benjamn]

If you call import("./b.mjs") more than once, won't you get a (new) rejected promise with the same Error object each time?

If that's correct, then what's the analogy for synchronous static import declarations? Perhaps they should re-throw any errors thrown by the original ModuleEvaluation()?

Tangentially, re-throwing feels like the right behavior in a future where we allow synchronous nested import declarations, e.g.

try {
  import "./b.mjs";
} catch (error) {
  if (isReallySerious(error)) {
    throw error;
  }
}
launchTheMissiles();

[bmeck]

My implementation would never finish linking if the Module resolved produced an error. It has a ModuleJob structure that keeps a hold of any error it produced. Right now the HostResolveImportedModule checks the job when linking, sees the error, then fails. Same as stated earlier. I am ok with it being in spec since I like the early error on incomplete/errored graphs. For now, we are able to work around it.

I am not sure what should happen though on finding an error currently I rethrow, but I think generating a new error everytime makes sense.

@benjamn

If you call import("./b.mjs") more than once, won't you get a (new) rejected promise with the same Error object each time?

https://tc39.github.io/proposal-dynamic-import/#sec-hostresolveimportedmodule

Doesn't specify the cache/idempotency behavior of errors from eval, so thats why this is coming up.

If that's correct, then what's the analogy for synchronous import declarations?

Do you mean just import declarations? They are untimed but I am using async coordination for them.

It is somewhat equivalent to errors during ModuleDeclarationInstantiation.

Perhaps they should re-throw any errors thrown by the original ModuleEvaluation()?

That is my current behavior, but people can muck with the error between imports which has made me start to lean towards throwing new Errors every time.

Tangentially, re-throwing feels like the right behavior in a future where we allow synchronous nested import declarations, e.g.

I'm still not sure on those synchronous blocking import declarations, but it is up for discussion.

[benjamn]

I'm not sure what "throwing new Errors every time" would mean. If we're not throwing the original error object, as produced by new Error("boo"), what else is there to throw? I can't imagine we would restart module evaluation and let the offending module throw a different error.

Do you mean just "import declarations"? They are untimed but I am using async coordination for them.

@bmeck Thanks, I've updated my terminology to "static import declarations" to distinguish them from dynamic import(...)s.

[caridy]

If you call import("./b.mjs") more than once, won't you get a (new) rejected promise with the same Error object each time?
https://tc39.github.io/proposal-dynamic-import/#sec-hostresolveimportedmodule

Doesn't specify the cache/idempotency behavior of errors from eval, so thats why this is coming up.

We are currently looking at this, and we might propose changing the spec to be specific about caching since it is needed for realms. Assuming that we will cache the resolution of hostresolveimportedmodule into an internal slot in the module record, will that help in any way with the problem described here? I haven't fully understand the problem, but I plan to look at it later this week.

[bmeck]

@caridy this is a result of ModuleEvaluation not changing the Module record to be in an Error state of some kind when a throw occurs while running. The cache itself is a bit of a red herring, but the re-use of any Module record that has already thrown.

[allenwb]

There are many ways to write buggy code and many ways to screw up module initialization that doesn't involve throwing an exception during module initialization. A similar failure would occur if someone coded:

//a.mjs
import {okToFire} from "./b.mjs";
okToFire();
launchTheMissiles();

//b.mjs
var initialized=true;  //no TDZ, set to undefined during modulation instantiation
export function okToFire() {
  if (initialized) {
    initialized=false;
    throw newError("boo");
  }
}

Clearly using module initialization success/failure (however it is defined) for control flow is a bad code smell. But trying to define the language in a manner that makes them impossible is a whack-a-mole endeavor. That's
why the spec. was written the way it was.

[guybedford]

The way I've tackled this problem in loader implementations has been to remove errored load records and all their parents from the loader registry immediately and synchronously as the error happens. Removing synchronously is important to avoid possible race conditions as well in the error handling.

So the behaviour of the example would then be that main2.js fails the first load, then reloads b.mjs for a.mjs from the cache (if using the cache, otherwise refetching), causing the error to fire again, and as a separate second execution.

Caching the error is certainly an alternative, but the default thing users want is the ability to "deal with" errors, and not have them causing leftover state which blocks loads from working for the remaining duration of the page (which is especially tricky without a registry API to remove this state).

[domenic]

I've written up a large, large variety of error cases as part of ironing this out for HTML. This question remains a possible inconsistency, although I'm still not sure about how bad it is:

• If you import a module that previously failed to fetch, you will get an error
• If you import a module that previously failed to parse, you will get an error
• If you import a module that when previously parsed contained a syntactically-bad module specifier, you will get an error
• If you import a module that previously failed to instantiate, you will get an error
• If you import a module that previously failed to evaluate, doing so will be a no-op

This seems a little surprising to me still. Again, we could fix it on the HTML side, by caching evaluation errors just like we cache parsing and instantiation errors. But I'm just not sure what the right approach is.

[GeorgNeis]

I agree that it is a little surprising. While I somewhat sympathize with Allen's point above, I don't see it as an argument for the current behavior.

[rossberg]

Agreed that this is bad.

@allenwb, the issue isn't that you can screw up initialization, but that it may still non-deterministically "succeed". It shouldn't be possible to execute a module when execution of its (direct or indirect) dependencies has failed.

[domenic]

I've updated whatwg/html#2595 so that in the near future the HTML spec should treat evaluation like all the other failures. I've added an item to the May agenda to have a quick discussion on if we want to mandate this for all hosts, but that's not as urgent as it sounds like browsers and Node.js at least will cache the error.

[allenwb]

@rossberg-chromium I see. Yes, I agree. If ModuleDeclarationInstantiation or ModuleEvaluation of some module fails the first time either is invoked then they should also fail for any subsequent invocations for that module. This probably should be stated in the Table 38 requirements for those abstract Module Record methods.

One way to deal in the spec. with failed module evaluation is to make the [[Evaluated]] field of Module Records into a tri-state value (uninitialized/initialized/failedInitialization) and to deal with setting the appropriate state in Source Text ModuleEvaluation.

It isn't clear that it is necessary (or desirable) to propagate any actual exception value thrown out of a module body. Prior to supporting dynamic import, any such error would have propagate to being a top level unhanded exception, so the actual exception value wouldn't have been available to the JS code (at least as specified).

But dynamic import changes that. I'm concern that using the same exception value on multiple dynamic import invocations could be exploited as a communications channel. For that reason, I think a fresh exception should be propagated each time an attempt is made to initialized an already failedInitialization module record. I suggest that exception should be a SyntaxError exception because it is indicating that the overall structure of the program (as represented by the imported modules) can not be proper constructed.

[domenic]

I don't think we should be re-generating new exceptions, losing the original context. In HTML at least it's important for telemetry purposes to get the same exception so that people can de-dupe to a single root cause. So we plan to catch any failures and store them for propagation to the top level and/or to dynamic import().

[allenwb]

An user thrown exception in a module body can carry an arbitrarily complex user provided payload. Seems risky to hang on to such things and pass them back out of dynamic import() multiple times.

[bmeck]

I would argue this falls under similar Idempotency goals as HostResolveImportedModule . Re-using the same value can be both seen as a pro and a con, but I think consistency across existing operations can be seen as valuable.

[littledan]

@allenwb How is that riskier than the way that modules typically export things like functions and classes, which, unless frozen, could be used to hang off an arbitrary payload (not to mention mutable export bindings)? You can freeze the functions you export the same way as you can freeze the exceptions you throw.

[domenic]

As we explore trying to spec HTML integration it's becoming increasingly clear that there's a problem at this interface. E.g. in whatwg/html#1545 long ago @jonco3 and I arrived at the conclusion that we needed to do a bottom-up instantiation so that we could properly figure out what caused an error and remember that state, so that future calls to ModuleDeclarationInstantion() don't succeed silently and thus break the idempotence requirements. As you can see this bug is very similar to the one in the OP.

But this has caused us some pain now for cicular modular graphs, since we are not doing the simple top-down instantiation: whatwg/html#2629

Looking through this problem, @GeorgNeis and I tentatively think the right solution might be for ES to handle this error propagation which HTML is currently doing manually---both for instantiation and for evaluation. Alternately, we could add host-configurable hooks after each instantiation/evaluation that allows the host to sync state appropriately; this might be more flexible and even necessary.

Our current plan is to prototype these approaches, both in spec-land in in V8 + Chrome code, and see whether they solve all of our outstanding problems. If so I will provide some pull requests on the ES-side modifications that would help us out. (Either just one PR with host-configurable hooks, if those prove necessary, or the choice between a PR with host-configurable hooks and one with baked-in error propagation.)

Will report back!!