Emscripten: Undocumented Module.then causes infinite loop

[saschanaz]

https://github.com/kripken/emscripten/blob/93479ecbd390aec4f8a3765fe04bcb365d0b31b2/src/postamble.js#L118-L141

postamble.js adds Module.then when -s MODULARIZE=1 but using it causes indefinite loop on Promise.resolve() and ES2017 await statement.

By the spec the resolving strategy is greedy: if an object has then method then it will be called with two function arguments including resolver and rejector. If the resolver is called with an object with a then method it will be called again with two function arguments, and then again, again.

Module.then resolves with Module which still has then, so the await statement loops indefinitely.

PS: Fixed some wrong concepts.

[saschanaz]

This is anyway an undocumented feature, we should be able to rename this to whenInitialized (example), etc.

[curiousdannii]

I have production code using Module.then, but not the return value.

This is an odd situation, because if I understand correctly, await handles normal promises fine even though they always have a then property.

Remove .then when initialized. All following then calls will cause exception as they will be calling undefined.

This would remove the entire point of making Module a thenable, rather than just accepting a callback.

If it was changed to make Module an actual Promise object (or for then to return a true Promise), would that work with await? That would then make emscripten depend on Promises being present. One option could be for it to check if Promise is defined, and if not to return the current Module. So on old platforms you either provide a Promise polyfill, or you can't call Module.then.

It would probably be worth putting all the loading errors through the same system so that you could call Module.catch as well.

And it's not completely undocumented, it is mentioned here: http://kripken.github.io/emscripten-site/docs/porting/connecting_cpp_and_javascript/WebIDL-Binder.html#modular-output

It should really be mentioned elsewhere in the docs too.

[saschanaz]

It seems I had misunderstood the concept of thenable. Any thenable object will receive two function arguments as the Promise constructor callback does, which means that it won't be resolved until one of the arguments are called.

PS: So I modified the original post.

[saschanaz]

This is the minimal repro of the Promise thenable behavior:

var o = { then(resolve, reject) { console.log("called then()"); resolve(o) } };
Promise.resolve(o);
// logger logs indefinitely 

[curiousdannii]

Sorry, I'm not really sure what you're trying to do. But a proper thenable should call resolve/onFulfilled with the value, not the whole thenable object, and it should return the object (this).

Here's the simplest example I could find from noq:

    then: function noQ_Promise_then(p, n)
    {
        if(this.__val === undefined)
        {
            (this.__c || (this.__c = [])).push({p: p, n: n});
            if(n)
                this.fail(n);
        }
        else if(this.__val === true)
            p(this.__res);
        else if(n && (this.__val === false))
            n(this.__res);
        return(this);
    },

Where __val is the status and __res is the value.

[saschanaz]

Sorry, I'm not really sure what you're trying to do. But a proper thenable should call resolve/onFulfilled with the value, not the whole thenable object, but it should return the object.

This is not what I'm trying to do, it is what the current postamble.js do.

https://github.com/kripken/emscripten/blob/93479ecbd390aec4f8a3765fe04bcb365d0b31b2/src/postamble.js#L129

[curiousdannii]

Ah, true. I think emscripten is making a valid thenable (aside from ignoring the onRejected arg), but it's not a proper promise. I could be wrong. It probably should be a full proper promise though.

[saschanaz]

A valid thenable should not resolve by passing itself, as the greedy algorithm will cause infinite loop. (Forget about the return value story from my previous deleted comment, it was just wrong. Sorry 😭)

[saschanaz]

(Ignore my another deleted comment. Sorry)

Converting module to a real Promise still may not solve the problem, as the Promise resolver still uses greedy algorithm.

new Promise((resolve, reject) => {
    resolve(Promise.resolve(3))
}).then(console.log); // logs 3, not Promise

(As it will still have to resolve by passing itself to keep compatibility)

[saschanaz]

So I would:

1. Break the compatibility by resolving with no value.
2. Or, export a new thenable object which resolves with no value. (so that one can instead await module.forInit for example)

[curiousdannii]

Or should the MODULARIZE mode Module be effectively a factory? And you only get access to the true Module when your then function is called?

[saschanaz]

That would be neat 👍 but is there any use case that requires module access before initialized? Probably not?

[curiousdannii]

Um, possibly if you have no external files (either no memory or inline memory) then it might be available immediately?

It would probably help to outline the use cases for MODULARIZE, so then we can see whether they would be better served by being separated out a little.

1. Node/Browserify/UMD access (#5864) -> This should really be fixed for non MODULARIZE. Probably would be very easy too.
2. Wrapping the code so it doesn't leak variables and compresses better etc. pre/post js is still an option, as would be browserifying etc. I'd be inclined to sacrifice this if it makes it easier for other use cases. Or sacrifice the non-wrapped option.
3. Thenable interface (eventually a true Promise ideally). IMO this should move towards being the main/only way of hooking in to when the module is ready.

Can you think of any others?

[saschanaz]

My previous use case for MODULARIZE was to run a CLI command multiple times in a single worker. (as calling main() multiple times caused error)

[andreasscherman]

Was bitten by this exact behaviour -- thanks for the detailed flesh-out of the issue. If someone stumbles upon this, inspiration as a hack-around could look something like this:

Module.then(m => {
  delete m['then'];
  resolve(m);
});

[mischnic]

Promise.all([instance]) also causes an infinite loop.

On the referenced issue on binaryen, a proposed solution would be to make Module.ready a thenable instead of Module itself. (This might of course break some old code).

[kripken]

.ready might be the best option, then.

I think we can do it as a breaking change, if it's necessary to get a fully working API, and if we add a clear error if people use the old API - could we set Module.then to suggest using the new API?

[mischnic]

I don't think there is a way to fix the old then and getting out of the recursion hell.
This

Module.then = function(f){
    f(Module);

    // something like this as the error message?
    console.error("Please use Module.ready.then(...) instead.");

    delete Module.then;
    return Promise.resolve(Module);
}

might seem like a solution, but not for long:

Module.then(v => console.log(v));
Module.then(v => console.log(v)); // Uncaught TypeError: Instance.then is not a function

Alternatively resolving with no value (https://github.com/kripken/emscripten/issues/5820#issuecomment-348096395).

---

The new ready would be

Module['ready'] = new Promise(function(func) { 
  // We may already be ready to run code at this time. if 
  // so, just queue a call to the callback. 
  if (Module['calledRun']) { 
      func(Module); 
  } else { 
      // we are not ready to call then() yet. we must call it 
      // at the same time we would call onRuntimeInitialized. 
      var old = Module['onRuntimeInitialized']; 
      Module['onRuntimeInitialized'] = function() { 
          if (old) old(); 
          func(Module); 
      }; 
  } 
});

then?

[AnthumChris]

Stumbled upon this thread when searching for a Promise-based alternative to onRuntimeInitialized. I also needed repeated calls from a Web Worker's onmessage handler. I ended up writing a Module.ready Promise implementation by building with emcc --post-js 'src/module-post.js'. Code is below. Hope this is useful to anyone in a similar situation:

https://gist.github.com/AnthumChris/c1b5f5526b966011dac39fbb17dacafe

[david-polak]

For anybody stuck on this bug, this is my workaround. Thanks @AnthumChris for directions

--post-js

Module['ready'] = new Promise(function (resolve, reject) {
  delete Module['then']
  Module['onAbort'] = function (what) {
    reject(what)
  }
  addOnPostRun(function () {
    resolve(Module)
  })
})

Instantiate

new EXPORTED_NAME().ready.then(module => {})

[kdex]

Couldn't we have the function at EXPORT_NAME return an actual Promise instead (or make it an async function)? So for the default EXPORT_NAME, we could get something like:

function Module() {
  const staticContext = {
    ALLOC_DYNAMIC: 3,
    ALLOC_NONE: 4,
    ALLOC_NORMAL: 0,
    // …
  };
  return new Promise((resolve, reject) => {
     // fetch and compile wasm, add exports to `staticContext`
     // resolve with modified staticContext if successful, reject otherwise
  });
}

This would fix ES2017 await statements and save us from workarounds. In order to instantiate the module, we could then use

Module().then(({ export1, export2 }) => {
  /* use exports here */
});

from non-asynchronous contexts and

const { export1, export2 } = await Module();
/* use exports here */

from asynchronous contexts.

[surma]

Merging myself from #6563 into this issue: This is confusing because then() is undocumented and you don’t even need to use it to end up in the infinite loop:

import myModule from './myModule.js'; // ← This is the file generated by Emscripten

const module = new Promise(resolve => {
  myModule({
    onRuntimeInitialized() {
      resolve(myModule);
    }
  });
});

I think re-naming then to ready is a great solution although I think it’d be preferable to NOT make ready() resolve to the module, but just undefined.

[surma]

... a second solution would be to remove the then property before resolving here. That would basically make the module then-able just once and prevent the infinite loop. Not sure that’s a good practice tho.

I’m happy to whip up a PR if one of the maintainers can give me a hint which solution is preferred.

[mischnic]

a second solution would be to remove the then property before resolving here. [...] then-able just once [...] Not sure that’s a good practice tho.

Yes, I mentioned that here as well: https://github.com/kripken/emscripten/issues/5820#issuecomment-365689067

But the Module would still be a promise that doesn't work as one would expect (just as now).

[curiousdannii]

I think it would be best to follow @kdex's mock up and make (the external) Module return a true promise. Which I guess would mean making the internal Module object be that promise, and adding all the usual properties onto it instead of onto a plain object.

I think this might have too many difficulties, and now favour having a ready property.

[DJMcNab]

This is just throwing a (probably bad) idea into the mix:

would it be possible to have the global Module with a then function which, after instantiation completes starts to inherit [1] from a private ModuleReal, which the then resolves to. Alternatively, all the properties from ModuleReal could be copied to Module (using Object.assign()). Unfortunately, this is likely to have negative performance implications no matter which way is chosen, so should not really be considered.

[paulshapiro]

@kripken any suggestion on best practice? I've taken to @david-polak's .ready solution which replaces .then but am not sure how complete I'm being.

[kripken]

@paulshapiro I haven't had time for this myself. Would be good if someone could focus on designing a better API here.

[kdex]

@curiousdannii

I think this might have too many difficulties

Would you (or someone else) please elaborate? I'd be interested to learn if there are any advantages compared to just returning a standard Promise.

[curiousdannii]

@kdex Because you can pass in the Module object and it's expected to be the actual Module object, not just an options object, we'd have to somehow mutate what's passed in into a promise. Simpler to just export a true promise as a ready property on Module.

[rajsite]

The then approach is documented here: https://kripken.github.io/emscripten-site/docs/getting_started/FAQ.html#how-can-i-tell-when-the-page-is-fully-loaded-and-it-is-safe-to-call-compiled-functions

Another option is to use the MODULARIZE option, using -s MODULARIZE=1. That will put all of the generated JavaScript in a function, which you can call to create an instance. The instance has a promise-like .then() method, so if you build with say -s MODULARIZE=1 -s 'EXPORT_NAME="MyCode"' (see details in settings.js), then you can do something like this:

MyCode().then(function(Module) {
 // this is reached when everything is ready, and you can call methods on Module
});

I think I hit the same issue as Chrome was crashing the tab and Firefox was aborting the script. Went down this road when switching from wrapping the exported Module myself using pre / post js and switching to using the Modularize approach.

[jakearchibald]

It feels like the right solution is to remove .then() and replace it with .ready.

.then could be replaced with a getter that warns to the console & returns undefined.

Is there anything blocking that?

If compatibility is a concern:

Module['then'] = function(func) {
  // Here's the change:
  const resolvedModule = Object.create(Module);
  resolvedModule['then'] = undefined;

  // We may already be ready to run code at this time. if
  // so, just queue a call to the callback.
  if (Module['calledRun']) {
    func(resolvedModule);
  } else {
    // we are not ready to call then() yet. we must call it
    // at the same time we would call onRuntimeInitialized.
    var old = Module['onRuntimeInitialized'];
    Module['onRuntimeInitialized'] = function() {
      if (old) old();
      func(resolvedModule);
    };
  }
  return Module;
};

[surma]

I wanted to try and revive this issue.

After recently completing another wasm-based project, I feel like the summary by @jakearchibald is accurate. A .ready promise would circumvent most of the loop problems and the .then() fix seems reasonable to me as well.

[kripken]

The main blocker here has been to decide on the best API that fits in optimally with existing JS usage patterns. If there is consensus here about .ready, then that sounds good to me.

[brion]

I'm still supporting IE 11 in ogv.js, but I use a Promise polyfill for other code in the system so relying on native Promise for .ready's return value shouldn't break things for me. It might be a surprise for some libraries, though; do we use Promise or other ES6 features elsewhere at this point outside of wasm-specific code paths?

Also please increment minor version number for the API break of losing .then, if possible. :)

[surma]

AFAIU, .ready would be a new property and wouldn’t replace any of the old ones so existing libraries would have nothing to worry about.

[brion]

@surma ah I might have misread the latest version of the proposal; is Module.then being kept for backwards compatibility?

[surma]

That’s my understanding.

[brion]

Great. :) Then my only concern is whether it should just assume Promise is present, throw an exception if it's not, or what. IMHO just relying on it being present is probably fine, only old browsers like IE11 are issues and you can just toss in a polyfill (like I do for my other uses of Promise).

[curiousdannii]

We should deprecate .then if it isn't removed immediately. The current .then function is not a valid Promises spec thenable because it resolves to itself.

[jakearchibald]

@kripken .ready is used in the web platform already:

• https://drafts.csswg.org/css-font-loading/#font-face-set-ready
• https://w3c.github.io/ServiceWorker/#ref-for-dom-serviceworkercontainer-ready
• https://streams.spec.whatwg.org/#default-writer-class-definition
• https://drafts.csswg.org/web-animations-1/#the-current-ready-promise

[Taytay]

I have been working on porting sql.js to compile to wasm in this pull request. In doing so, I experimented with the MODULARIZATION=1 flags, came across some issues, then moved to a hand-rolled ready promise based on the recommendations here.

I was not satisfied with the results. With the current ready proposal above, modularizing a wasm module would return a partially initialized Module object that exposes numerous properties/methods, but expects you not to call any of them until you call ready() and wait for that to resolve.
That feels ripe for mistakes since it is quite easy for a user to either not know about this property or simply forget to call it. I prefer to have classes/objects that aren't able to be mis-used before being initialized. Furthermore, it was concerning to me that for some compiled modules you would need to call ready, and for others (asm.js) you could simply include it in the page and start calling methods on the Module object without calling ready.

I believe that a compiled wasm module (lowercase module) should instead export a promise-returning function rather than the Module object itself. This _promise_ would resolve with the fully-initialized Module, meaning that it would not be available to be used until the WASM is fully loaded.
You would use it as follows:

let loadWasmModule = require('myWasmModule');

const moduleConfig = {
     // This is where you would set emcc config properties like `preInit`, `print`, etc:
};

loadWasmModule(moduleConfig).then((fullyInitializedModule)=>{
     fullyInitializedModule.someMethod();
     ...
}).catch((error)=>{
     console.log(`The module failed to load/initialize: ${error}`);
});

As for using it in the browser, folks could follow one of two patterns:
1) If included in the browser, it would create a global version of the module _loading_ function, which is the closest analog to what Emscripten does today. If we were to do this, there would no longer be a concept of a global Module object that is both the input and output to Emscripten. The input Module config would be a parameter passed to the loading method, and the output would be a promise that resolves to the Module. I believe this is what the MODULARIZE method tries to do today.

2) People could use tools like Webpack to consume the module and export it or consume it using any variable name they see fit.

As long as we have the correct modularization post-amble code, all emcc modules would be able to be consumed by either pattern.

[Taytay]

@jakearchibald : You mentioned some other places where ready is a known pattern in the web world. I believe that those are slightly different cases.

Let's take these two examples:

• https://drafts.csswg.org/web-animations-1/#the-current-ready-promise
• https://streams.spec.whatwg.org/#default-writer-ready

In those cases, you have an object (an animation or writer) that is maintaining state that is changing over time. The ready promise does not tell you whether or not the object is valid/usable yet, but rather when a particular transient state is complete.

In the case of https://drafts.csswg.org/css-font-loading/#font-face-set-ready, and https://w3c.github.io/ServiceWorker/#ref-for-dom-serviceworkercontainer-ready, I believe that the other properties/methods of FontFaceSet and ServiceWorkerContainer are usable before ready resolves.

I confess that I am quite new to WebAssembly Modules (and totally unfamiliar with the APIs linked above until I clicked those links), so it's quite possible that my assumptions around whether or not a Module needs to be callable/usable before it's done loading are incorrect.

[jakearchibald]

If the object is totally unusable until 'ready', then I agree with you that it shouldn't be exposed until it's ready. This is similar to what we did with fetch():

const response = await fetch(url);

This can be a problem if you later want to introduce things that work before 'ready'. We hit this issue when we tried to make fetch abortable, and had to create a separate thing. I'm happy with the design we ended up with there, but if fetch() already returned an object representing the in-progress fetch I'd have just slapped an .abort() method on it.

[Taytay]

Excellent point @jakearchibald. It sounds like the question is whether or not this Module being returned is or needs to be usable in an uninitialized state.

I wasn't familiar with that abortable fetch method, and think that the solution you all came up with is a clever one. I can see how not having anything to return until the initialization is complete would paint you into an API corner when you want to add more functionality later!

(Ignoring the abortable case for a second, I _love_ the simplicity of the fetch API and how hard it is to misuse.)

Do you or anyone else here know whether or not we should preserve the ability to call/manipulate a Wasm module before it's finished loading? It sounds like that is pivot upon which we should design this async API.

[david-polak]

Do you or anyone else here know whether or not we should preserve the ability to call/manipulate a Wasm module before it's finished loading? It sounds like that is pivot upon which we should design this async API.

Since we have --pre-js and --post-js compile time options available, which basically implement this functionality, we can probably do away with pre-ready object mutations.

[surma]

@Taytay If we were to design the API from scratch, I’d agree. What we are discussing here, however, is an integration point of wasm modules in an async workflow without breaking existing code.

So we can’t really modify the code that -s MODULARIZE=1 generates. .ready (as a prop, not a method!) is the smallest, isolated change that gets us there, imo.

[Taytay]

Oh, I see @surma.
If backwards compatibility with existing MODULARIZE is a necessary goal, then my proposal is a non-starter.

In that case, I think my only related proposal would be to allow me to remove these lines of code in the MODULARIZE=0 case:

  if (typeof module !== 'undefined') {
    module['exports'] = Module;
  }

https://github.com/kripken/emscripten/blob/3e193554813643458a506e299c49cbf6c53fe4e9/src/shell.js#L149-L151

Those lines of code get in the way of my own manual modularization efforts in sql.js. When they run, the export of my module changes from my promise-returning function to the module itself. I am forced to undefined module as part of my pre-amble as an ugly workaround for ensuring that line of code doesn't run: https://github.com/kripken/sql.js/pull/255/files#diff-3e64d2627f31c61933d478d13578837eR68

This might also be too large of a breaking change, and I do have a workaround in place, so it's not absolutely necessary, but I do like the idea of saying, "If you want the existing modularization offered by emcc, use this. Otherwise, you can roll your own modularization by wrapping the code as follows. That line of code prevents the 'roll your own' option. "

[curiousdannii]

@Taytay That specific problem you're running in to really has nothing to do with this issue. Emscripten expects to be run in a CommonJS environment, and if you use it in another environment that still has a module of course it will get into trouble. I'd recommend you use browserify/webpack/rollup instead of cating files.

It would be reasonable to add an option to not do any exporting, but that should be a new issue.

[Taytay]

@curiousdannii : I am running in a CommonJS environment, but I still wanted control over the modularization code. I've created a new issue/feature request here, as you suggested: #7835

[s-h-a-d-o-w]

This might be a naive question but why don't you simply delete then from the module once it was executed?

I just modified the code like so:

Module['then'] = function(func) {
  delete Module.then;
  // ...

... and it works like a charm.

It may be hacky but then... couldn't the same be said for other current methods of communicating when the module is ready? (Through the global namespace)

[curiousdannii]

@s-h-a-d-o-w Because a .then method is meant to be able to be called multiple times, and we should assume that many Emscripten users are currently doing so.

[s-h-a-d-o-w]

Ah, of course! (I even persist promises in a memoization library I wrote but it's generally something that I very rarely use, so I didn't think of that.)

Well... how about documenting here how users can write a wrapper for it?

Like:

  const importWASM = () => {
    return new Promise((resolve) => {
      require('./webp_wasm.js')().then((module) => {
        // Since the module contains a .then(), it needs to be 
        // wrapped to prevent infinite calls to it.
        resolve({module});
      });
    });
  };

  const {module} = await importWASM();

[lourd]

Just wanted to +1 this and add another case. When using Emscripten with TypeScript, the compiler will not allow the factory function to be awaited on. It gives the error: Type is referenced directly or indirectly in the fulfillment callback of its own 'then' method.. This playground demonstrates the compiler outcome. This is actually quite helpful, as it avoids the infinite loop at runtime, but for this, loading a module becomes a confusing mess because even returning the module from an async function causes this compiler error.

I'm happy to take on the work to fix this. Instead of adding the ready property, what about just not passing the Module to the callback in then? Or is ready the approach people would prefer?

Edit: It's also strange that Module is returned from then. Could that be changed as well? I can't imagine there's many users out there relying on the then API given this footgun.

[kripken]

Thanks @lourd !

I really would like us to make progress here. I have not been able to do much myself because I am not knowledgeable enough about conventions in the JS ecosystem, and no one else has had the time to work on it it seems.

Your suggestion sounds reasonable. I think we can even make it error in a clear way for existing users by, in ASSERTIONS builds, proving an argument to then() that throws with a message if it is used (and the message would explain what to do and/or point to the PR for more details etc.). So we can avoid silent / weird breakage.

Alternatively, given the severity of this issue, another option is as discussed above to delete .then after the first use. That would also be a breaking change, and also I think it's a reasonable one, which we can mitigate with a nice error message in ASSERTIONS etc.

I don't have a strong preference between the two (again, I don't know enough about this) so I'd really like to hear from the people that discussed this at length previously, which of those two is best. But I do think we should move forward with one of the two, and soon, as this has been a big footgun for far too long.

(Sounds good about the other proposed change, to not return Module from then() - again, with some method to make sure users get a nice error in ASSERTIONS.)

[curiousdannii]

Alternatively, given the severity of this issue, another option is as discussed above to delete .then after the first use.

No, this is a bad option as it's meant to be able to be called more than once.

There's some good discussion at #9325 but I'm not sure if that PR should be merged as it's doing too many other things as well.

One more thought: it's not actually possible to add ready without removing then (from any builds that have ready), because if the object returned when ready resolves contains a then, the then will be called automatically.

Making it more restrictive is what I was aiming to do. Personally at least, for a very long time working with Emscripten I would consistently either forget to wait for initialization or forget how. Returning an opaque promise makes it obvious: wait for the promise and then you have a fully initialized Emscripten module.

I like these comments from @kainino0x and think there's a good argument for making the modularised function directly return a promise, because that way there's no accidentally using it before it's ready. For non-modularised builds a ready property could be made. In the first case it has to resolve with the module (otherwise you can't access it), and in the second it doesn't have to, but in both cases it couldn't have a then property. (Possibly this is exactly what your PR did, except that it requires you to change to MODULARIZE=2.)

Things would be simpler if the non-modularised option was deprecated. (And I think a terminology change would help - we have the confusing fact that "module" refers to what you pass in to it, as well as the object at the end of the process with all of the ASM functions and memory views, while "modularize" is really a factory function.)

[pwuertz]

Changing the module-API during lifetime, e.g. being thenable at first, not thenable later, feels wrong. Never seen that kind of behavior in any other API.

Removing the then argument may fix the await for module readiness. But if tried to return a module from another async function, then() will still get into your way.

Just remove then() in favor of ready.

[brion]

I agree changing the API at runtime is icky, and will break on static analysis tools. The most natural API for modern JS I feel is for the module factory function to return a Promise, hiding the actual Module object until it's resolved through the promise. This would be a breaking change, but so would removing the then method on its own without making the change to the factory func.

[kainino0x]

Another +1 to not removing then dynamically.

Removing then (statically) would be a nice easy fix, but I personally think we should go all the way to being completely opaque until the promise resolves.

I would greatly appreciate if someone else would pick up this task. I'm not working on it anymore; I have another way to achieve what I needed.

[lourd]

Thanks for the input everyone. It sounds like a good approach would be:

• For modularized builds, calling the module factory function should return a Promise. That Promise should resolve with the module instance. If initialization fails, it should be rejected with... what? What currently happens?
• For non-modularized builds, there should be a property named ready on the module that is a Promise. That Promise should be resolved once the Promise has been loaded.
• Removing the then method altogether.

Is that all correct? Should I also make any changes to onRuntimeInitialized? Is there anywhere I should put warnings on assertion builds?

Any codebase guidance folks have would be appreciated too. Will this change be contained to postamble.js and tests, or will this change affect more source files?

[brion]

In case of failure, the promise should be rejected with an Error instance; for instance if an exception gets thrown and caught during a sync operation, you'd funnel that into the reject function (unless you want to replace the specific error with a more generic error condition, but it's easier to diagnose problems with the original one I find)

IIRC what happens now on failure is that an exception gets thrown and then your 'then' callback just never gets called. This is ok for manual debugging but terrible for automatic fallback behavior where you might want to replace the emscripten module with a nice error message or alternate experience. :)

[pwuertz]

Edit: @brion Ah sorry, totally misunderstood the point you were making! ( Hi again by the way ;) )

I'll just keep this here as a possible example for loading modules as discussed earlier:

// Promise with success/error handlers
loadModule().then(onModuleLoaded, onModuleFail);

// Promise with fallback and fail handling
loadModule()
.catch(error => { /* try returning fallback module or re-throw */ });
.then(module => { /* use module */ })
.catch(error => { /* show error */ });

// Async style
try {
  const module = await loadModule();
  /* use module */
} catch (error) {
  /* show error */
}

[juj]

Whatever the fix, it would be great if someone can contribute minimal test cases for the use cases/loading patterns that are being used that showcase this failure. Reading through the long conversation thread, I don't think I was able to find a test case. (sorry if I missed one)

If there was a PR that showcases "I would like to do this, but it fails - if you fix this loading code to work, all would be good", I think that would help towards what is needed. (also, do people have different loading patterns that exhibit this problem from different angles?)

[brion]

@pwuertz I mean that not getting a rejection is terrible. When you get one, you can handle it and that's great! :)

@juj the failure cases I want to catch are mostly things like "oops we were asked to load module with SIMD instructions but no SIMD support in browser" or "the .wasm file is missing or inaccessible, so can't fully instantiate". The former is dependent on actual browser behavior so might not be suitable for test cases; the latter could be simulated in tests by deliberately corrupting or deleting the wasm file, perhaps.

Similar to @pwuertz's samples above, a basic instantiation in classic (non-async) code would look something like:

MyCoolModule({
  locateFile: blah,
  options: foo,
}).then(function(module) {
  module._do_something();
}).catch(function(err) {
  // This will catch any errors during instantiation or
  // at runtime during the 'then' handler.
  // We could also put this before the 'then' if we wanted
  // to treat instantiation errors separately from runtime errors.
  displayError("Error loading module: " + err);
});

You could be fancier with a fallback to loading another module, but the important thing is just getting the 'catch' callback on failure.

[kripken]

Thanks for the feedback everyone! Sounds like there is general consensus on the solution @lourd and @brion describe here? It sounds good to me. Perhaps a good next step is a PR from @lourd , including tests (which would also address @juj's point above)?

Regarding code, yes, I'd expect it to be just postamble.js + tests, unless I'm forgetting something. There is some modularize code emitted from emcc.py (see modularize()) but at a glance I don't think that handles .then() related things.

[lourd]

Just submitted a PR to start work on this. Would love any review or collaboration on it from folks subscribed here.

[lourd]

Running into some hairy issues with the changes needed to do this as related to pthreads and the MODULARIZE_INSTANCE option. See the PR for details, thanks in advance for your help.