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 (Including an emscripten module in browserify build process #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);
});