js-promise-integration icon indicating copy to clipboard operation
js-promise-integration copied to clipboard

Wrapped imports should expect Promises

Open fgmccabe opened this issue 3 years ago • 8 comments

In a parallel to Issue #11, a similar question arises for imports: should a suspending import require that it is given a Promise? (As opposed to testing for a Promise and only suspended if it is actually a Promise)

fgmccabe avatar Oct 12 '22 22:10 fgmccabe

According to this, an API is expecting a Promise should always resolve the value. This seems to imply that wrapped imports should always suspend.

fgmccabe avatar Oct 19 '22 18:10 fgmccabe

The current spec says that the suspender only needs to be valid if the import returns a promise and otherwise it can be null, not the active suspender, or have JS frames. I think this allows a nontrivial code simplification. Here is an example of a use case. This is all of course possible anyways but it requires significantly more complicated dynamically generated wasm modules. I think the code simplification allowed for this sort of use case is sufficient that it would be worth considering leaving this in.

For one thing, currently if I compile the following code:

#include <iostream>
#include <emscripten.h>

using namespace std;

EM_ASYNC_JS(int, async_func, (), {
  await new Promise(res => setTimeout(res, 1000));
  return 5;
});

int main () {
  try
  {
    int x = async_func();
    cout << "got:" << x << '\n';
    throw 20;
  }
  catch (int e)
  {
    cout << "An exception occurred. Exception Nr. " << e << '\n';
  }
  return 0;
}

with emcc -sASYNCIFY=2 -fexceptions, it fails because various C++ pre-main setup code is run with exception handlers that are suspending calls. They need a valid suspender, but while main is a promising function and when main is called it would work, the pre-main code hasn't been instrumented. This could be fixed in a few different ways, but they all add nontrivial complexity.

More specifically, the problem is that in existing code there are a bunch of JavaScript trampolines that do some setup, call a wasm function pointer, and then do some teardown. There are several uses for this:

  1. The Emscripten JS exception handling ABI uses it to do setjmp/longjmp / C++ exceptions / Rust panics with JS exceptions. It's desirable to eventually switch to wasm exceptions, but there are reasons to use the legacy system.
  2. calling a wasm function pointer that may take fewer arguments than expected -- it wants to just ignore the excess arguments. There's no way to do this with call_indirect but it is easy with a JS trampoline.
  3. libffi has an API to call a function with a signature that is provided at runtime. The reflection needed to do this is not possible in wasm

These trampolines are hazardous for JSPI because up until the JSPI was introduced they were implementation details but with the JSPI they prevent suspension from working as expected. You can handle all of these with a generator e.g.,

function * trampolineLogic() {
   // do setup 
   try {
      // it's expected that the generator host code will call the yielded fptr with 
      // the yielded args
      const result = yield [fptr, args]; 
      // do teardown
      return modified_result;
   } catch(e) {
      // exception handling
   }
}

then a handler:

function trampolineHandler(trampolineGenerator, ...args) {
   let gen = trampolineGenerator(...args);
   let [fptr, call_args] = gen.next().value;
   if (suspenderGlobal.value === null) {
      // handle synchronously
      try {
         const result = getWasmTableEntry(fptr)(...call_args);
         return gen.next(result).value;
      } catch(e) {
         return gen.throw(e).value;
      }
   } else {
      // handle asynchronously
      return async function() {
         try {
            const result = await promisingWrapper(getWasmTableEntry(fptr))(...call_args);
            return gen.next(result).value;
         } catch(e) {
            return gen.throw(e).value;
         }
      }
   }
}

and then we can wrap trampolineHandler.bind(null, trampolineLogic) in a suspending WebAssembly.Function and make another wrapper module that partially evaluates the suspending function with the suspenderGlobal and then the trampoline works both when there is a valid suspender and when the suspender is null. Without the option to call a function that does not return a promise with a null suspender, we would need more complicated dynamically generated wasm modules.

hoodmane avatar Jun 22 '23 17:06 hoodmane

Is there a reason the pre-main code cannot be executed within a suspender (with the understanding that the tooling would have to be updated to implement this change)?

RossTate avatar Jul 14 '23 14:07 RossTate

I asked the emscripten folks and they have no intention of making -sASYNCIFY=2 -fexceptions. Arguably if this isn't supposed to work it would be nice if emscripten warned me. But that first example is not a valid demonstration of anything because of this.

hoodmane avatar Jul 14 '23 15:07 hoodmane

We are getting to the point where we need to make a decision on this. At the moment, I see the arguments as pro:

  1. requiring a Promise from wrapped imports leads to a more uniform expectation; esp. given the assumption that one wraps imports that are 'supposed' to return a Promise
  2. It is observable whether or not a suspending function suspended or not. (This is really the same issue as above)

con:

  1. marginal performance cost
  2. situations like the one above: incomplete toolchain support
  3. current implementation in V8 does not automatically create Promises where none existed.

TBH, I believe that uniformity in design and expectations is pretty important. Head's up: I anticipate coming to a decision in the coming weeks.

fgmccabe avatar Oct 06 '23 20:10 fgmccabe

Whatever we do, I think we should make wrapped imports consistent with wrapped exports. So, if wrapped imports expect the original import to always return promises, then a wrapped export should always return promises, and vice versa.

RossTate avatar Nov 03 '23 16:11 RossTate

This is resolved in favor of resolving inputs of wrapped imports. I.e., use Promise.resolve on result of call to import; this coerces the value to be a Promise (or then-able). As a result, calling a wrapped import will result in the code being suspended.

fgmccabe avatar Nov 20 '23 20:11 fgmccabe

Am reopening this issue. The original motivation for performing the equivalent of Promise.resolve on the result of a call to an import was to follow W3C TAG guidelines about Promise-bearing APIs. As I understand it, the reason for the guideline (APIs that might return Promises should always return Promises) is one of consistency. In particular, any call to such an API becomes much more complicated if it has to deal with the two cases; which, esp in the case of JS, results in dramatically different code.

However, in the case of JSPI, in the case of calling an import (from WebAssembly), the expected behavior is both clear and straightforward: in the case that an import returns a Promise, suspend until it is resolved and continue with the resolved result; in the case that an import did not return a Promise, simply pass the result to the WebAssembly code. In both cases, JSPI is really specifiying the call site behavior of the API, not the result itself.

I therefore suggest that we revert to the original semantics of imports: suspend if the result of an import call is a Promise (and attach the continuation to the Promise), otherwise simply return the result of the call.

This does not affect the design wrt exporting wrapped functions from WebAssembly: they will continue to return Promises, in alignment with W3C TAG recommendations.

fgmccabe avatar Apr 09 '24 18:04 fgmccabe

We have decided to only suspend wrapped imports that return Promises.

fgmccabe avatar May 29 '24 19:05 fgmccabe

Belay that: following input from spec stakeholders and others, calls to wrapped imports (Suspending functions) will use Promise.resolve on the returned value. This permits Promise-like objects but also results in all such calls suspending.

fgmccabe avatar Dec 17 '24 19:12 fgmccabe