Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use IsPromise #56

Open
bakkot opened this issue Nov 14, 2024 · 6 comments
Open

Don't use IsPromise #56

bakkot opened this issue Nov 14, 2024 · 6 comments

Comments

@bakkot
Copy link

bakkot commented Nov 14, 2024

IsPromise is an ECMAScript-internal thing. Web specs don't use it, and ECMA-262 only uses it inside of Promise.resolve and Promise.prototype.then. It's not intended for use by host APIs like this.

The more conventional thing would be to either do an actual await unconditionally (which would be my preference), or to check that the result is an object and only await it in that case (if you're really concerned about not having a microtask tick for null, or something).

I can help update the spec text to do the conventional thing if you'd like.

@fgmccabe
Copy link
Collaborator

The issue is that (a) we are only looking for 'branded' Promises and (b) we definitely do NOT want to await if a non-Promise is returned.
Given that, I am certainly interested in a better wording.

@bakkot
Copy link
Author

bakkot commented Nov 14, 2024

we are only looking for 'branded' Promises

Why? That is not something literally any other JS or web API does, as far as I'm aware.

we definitely do NOT want to await if a non-Promise is returned.

Say more? Why not? What are the scenarios you're concerned about here? My understanding is that this bit of logic only applies when calling specially-marked JS functions, which means that it only applies to functions which return Promises at least some of the time, and in JS it is extremely unusual to have a function which sometimes returns a Promise and sometimes does not.

@bakkot
Copy link
Author

bakkot commented Nov 14, 2024

we definitely do NOT want to await if a non-Promise is returned.

If you're concerned about ensuring that applications can avoid the microtask tick when calling a function whose result happens to be immediately ready, they can achieve that with something like

function possiblyAsync() {
  if (isReady) {
    return { async: false, value: foo };
  } else {
    return { async: true, value: new Promise(resolve => /*...*/) };
  }
}

function isAsync(box) {
  return val.async;
}

function unwrap(box) {
  return box.value;
}

let imports = {
  possiblyAsync,
  isAsync,
  unwrapAsync: new WebAssembly.Suspending(unwrap),
  unwrapSync: unwrap,
};

and then

(local.set $wrapped (call $possiblyAsync))
(if (call $isAsync (local.get $wrapped))
  (then (local.set $unwrapped (call $unwrapAsync (local.get $wrapped))))
  (else (local.set $unwrapped (call $unwrapSync (local.get $wrapped))))
)

The { async: true, value: promise } | { async: false, value: notPromise } return type is the pattern used by Atomics.waitAsync.

@fgmccabe
Copy link
Collaborator

The approach described in the spec has been adopted after a fair amount of discussion within the Wasm CG.
Your approach would specifically not be reliable in some cases; e.g., Atomics.awaitAsync.

Note that we are not specifically covered by this. Or, more accurately, we are partly covered by it: when a module function is exported via WebAssembly.promising, that function will always return a Promise. In the case of suspending, JSPI acts as a consumer of the Promise-returning API call; as such we have the same latitude that JS callers do.

Furthermore, the Promise (if it is actually there) is not seen by any user code: it is 'consumed' by the JSPI and transmuted into a suspension of the WebAssembly code. I.e., as far as user code is concerned, from a normal data flow perspective, the caller from user code does not see whether or not a Promise was returned.

(There is a back-door route by which a wasm client can potentially tell if they had been suspended but the caller of the suspending function is not directly aware of having been suspended. The principle is the same as for a stopped machine: a stopped machine can only infer indirectly -- after the fact -- that it had been stopped.)

@bakkot
Copy link
Author

bakkot commented Nov 14, 2024

The approach described in the spec has been adopted after a fair amount of discussion within the Wasm CG.

Is that discussion available anywhere? If not, can you summarize it? Since this contradicts specific guidance from the TAG (as well as being different from every other web spec) I think it's important to capture the reasoning here. I can also come to a Wasm CG meeting if that would be helpful.

Your approach would specifically not be reliable in some cases; e.g., Atomics.awaitAsync.

I don't know what that means. I gave an example of how to consume an API like Atomics.awaitAsync; just replace possiblyAsync with Atomics.awaitAsync in my sample and it works as-is. Can you explain what you mean by "not reliable"?

Note that we are not specifically covered by this. Or, more accurately, we are partly covered by it: when a module function is exported via WebAssembly.promising, that function will always return a Promise. In the case of suspending, JSPI acts as a consumer of the Promise-returning API call

Consuming the result of a developer-supplied Promise-returning function is covered in this document and it says the same thing I did: "you should also allow it to return a thenable or non-promise value, or even throw an exception, and treat all these cases as if they had returned an analogous promise". What do you mean by "we are not covered"?

as such we have the same latitude that JS callers do.

JS callers do not have access to IsPromise, which is internal to 262. You're doing something that is beyond what JS callers can do. The most a JS caller could do would be to switch on "is it an object" or something like that.

Furthermore, the Promise (if it is actually there) is not seen by any user code

Right, but it is produced by user code, yes? So it matters whether only branded promises are accepted.

@fgmccabe
Copy link
Collaborator

fgmccabe commented Dec 4, 2024

Following on from the discussion in the stacks sub-group, and some off-line discussions with other stakeholders:

  1. There are three choices for specifying the behavior of Suspending:

    1. Use a 'branded check' as currently specified
    2. Implement a test that mimics the test as specified in EcmaScript for Promise.resolve
    3. Use Promise.resolve on the result of the called JS function.
  2. The first of these options was deemed as being unsuitable (in the meeting, and also outside)

  3. The second is not suitable for V8 for a variety of reasons:

    1. The V8 engine would have to apply Promise.resolve (or the moral equivalent) to the Promise-like object. This is because we cannot permit the continuation that we currently attach to Promises to be invoked from user code.
    2. The user observability of the test is somewhat vague (is the getter for .then called once or twice?)
    3. The resulting increase of complexity of the specification, the implementation and the ecosystem is unfortunate.
  4. The third option has the potential to 'leave some performance on the table'. However, as far as we are aware, this does not effect the primary use cases that caused us to design the current specification.

This counts as a call to those who feel that option 3 does not meet their needs: i.e., that the performance implications of using Promise.resolve are not acceptable. Otherwise, we are leaning to option 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants