Remove Job from Promise Resolve Functions

[jridgewell]

This aligns the behavior of handling thenables with the Promises A+ spec, which says when the resolution value is a thenable, you're supposed to synchronously call thenable.then(onRes, onRej) (Step 2.3.3.3).

Given the example code:

new Promise(resolve => {
  resolve(thenable)
});

The current behavior requires 2 ticks for the outer promise to fully resolve. One tick is created by the Promise Resolving Functions (and is removed in this PR), and one tick is created by the wrapping of the onRes/onRej callbacks passed to Promise.p.then. This made it noticeably slower to resolve with a thenable than to invoke the thenable's .then directly: thenable.then(onRes, onRej) only requires a single tick (for the wrapped onRes/onRej).

With this change, we could revert #1250 without slowing down Await. Partially, I didn't see the 2nd promise allocation. This PR makes the pre-#1250 run in 2 ticks instead of 3. We still want 1 tick.

Fixes #2770.

[ljharb]

What are the observable changes as a result of this PR, besides "number of ticks greater than zero"?

[ljharb]

Also, it's worth investigating how this would impact websites deployed with es6-shim, core-js, and other Promise shims - if browsers start having different behavior here, it might cause the shims to kick in when they otherwise wouldn't.

[mhofman]

Also very curious about the web compat aspect of this change. I don't doubt some code out there now relies on then jobs being in a new tick.

[jridgewell]

What are the observable changes as a result of this PR, besides "number of ticks greater than zero"?

Case 1: Observable Change

new Promise(res => {
  let sync = false;

  resolve({
    then(onRes, onRej) {
      sync = true;
      onRes();
    }
  });

  console.assert(sync === true);
})

Before, sync === false. Now, it'll be true. There are no other observable changes. If there's were an abrupt completion, the outer promise would behave the same.

---

Case 2: Observable Change

const parent = Promise.resolve();

parent.then(() => {
  let sync = false;
  parent.then(() => {
    console.assert(sync === true);
  });

  return {
    then(onRes) {
      sync = true;
      onRes();
    }
  };
});

Same deal, now sync === true. There are no observable changes to the outer promise.

There are no other orderings of parent.then() which would be observable. Because you have to reutrn thenable, it's not possible to observe that its then will be sync called after you, well, returned the thenable. If you added more parent.then(() => sync) calls in any other location, you could only observe sync === false. It is only parent.then()s called in that exact spot before we return the thenable that you can observe the then is sync called. And they can't even tell that its sync, they can only tell that it's called before the promise reaction their function.

[jridgewell]

Also, it's worth investigating how this would impact websites deployed with es6-shim, core-js, and other Promise shims…

Both es6-shim and core-js behave with the current semantics. I do not know if they actually test for this behavior, but it'd be ill advised to overwrite the native Promise impl if they did, given that Promise is used all over, and native APIs/async functions will never return the userland impl.

I imagine most other Promises impls are using A+ semantics, because it actually has a portable test suite and isn't written in spec language.

[mhofman]

For what it's worth, this change would partially break my "hack" to detect promise adoption/redirect through node's async_hooks. I heavily relied on the fact that the then would be called on a clean stack to differentiate from other async hooks triggers. Definitely not a blocker as I've been meaning to request a better hook for this in v8 (and have thoughts on a less privileged hook).

[raulsebastianmihaila]

Not only is the proposed change a breaking change but it is also counter-intuitive. then in my mind means 'in a future tick' so the function shouldn't be called synchronously.

[nicolo-ribaudo]

@raulsebastianmihaila As far as I understand, with this change the function called to then is still called asynchronously. The difference is only that .then itself is called synchronously (i.e. it behave like thenable.then(fn) instead of Promise.resolve().then(() => thenable.then(fn))).

[raulsebastianmihaila]

@raulsebastianmihaila As far as I understand, with this change the function called to then is still called asynchronously. The difference is only that .then itself is called synchronously (i.e. it behave like thenable.then(fn) instead of Promise.resolve().then(() => thenable.then(fn))).

Even so it's a breaking change and counter-intuitive.

[jridgewell]

then in my mind means 'in a future tick' so the function shouldn't be called synchronously

That's not the contract of thenable. then provides a way for an outer promise to receive the inner's value, and it's actually not enforced that the thenable be async at all (it could sync call onRes):

const outer = Promise.resolve({
  then(onRes) {
    // onRes is sync called.
    onRes(1);
  }
});

Regardless of the behavior of the inner thenable or how we receive its value, we don't change the behavior of the outer promise. Chaining outer.then(next), the next callback is guaranteed to be called 1 tick after outer becomes resolved. So the users of native Promise won't affected by this change, it'll only make it faster for a native promise to consume a thenable (which could also be a native promise):

const parent = Promise.resolve();
const outer = parent.then(() => {
  // Fetch returns a native promise
  return fetch('...');
});

Here, it used to take 2 ticks in order for outer to consume the fetch. Now, it'll only take a single tick.

[raulsebastianmihaila]

That's not the contract of thenable.

I'm not necessarily arguing what the contract of 'thenable' is, as I'm not certain what that means. If it is what's specified at the moment (as I have learned it) then calling then asynchronously is part of the contract, not sure why you say it's not. I don't think the Promises A+ spec should weigh more than the current spec.

What I'm saying is that the current spec matches the intuition that whatever the steps encapsulated in the then function are, they are executed in the future as in my intuition the word 'then' suggests. I went to the library, then I went to the supermarket. Obviously I went to the supermarket later. Where we disagree is that you think that only the callback passed to the then method of a promise is relevant in terms of asynchronicity, while I think that the then method of a thenable itself is relevant too, when resolving the thenable. For instance you can ensure something is executed in the next tick by doing:

Promise.resolve({ then() { console.log('logged in the future'); } });

You can do the same like this:

Promise.resolve().then(() => { console.log('logged in the future'); });

but resolving 'nothing' is less intuitive. Another way of thinking about a thenable (which is illustrated in the snippet above) is that it encapsulates some steps not only syntactically but also from a time perspective. This means that the thenable itself represents a kind of encapsulation, not only a callback passed to a then method of a promise.

Also having both the then method of a thenable (when resolving the thenable), as well as the callback passed to the then method of a promise be treated similarly means there's more consistency with regard to how userland code that interacts with promises is treated and therefore it also represents less cognitive load.

[jridgewell]

Another way of thinking about a thenable (which is illustrated in the snippet above) is that it encapsulates some steps not only syntactically but also from a time perspective. This means that the thenable itself represents a kind of encapsulation, not only a callback passed to a then method of a promise.

You're unfortunately comparing snippets with two very different behaviors, and one is doing the work now and the other is deferring a tick. This is demonstrated by .then being sync retrieved in the current spec:

// current spec text
// t = 0
Promise.resolve({
  get then() {
    // t = 0
    console.log('logged immediately');
  },
});

A more equivalent snippet is to defer work till later in both branches:

// proposed spec
// t = 0
Promise.resolve().then(() => {
  // t = 1
  console.log('logged in the future');
});

// t = 0
Promise.resolve().then(() => {
  // t = 1
  return {
    then() {
      // t = 1
      console.log('logged in the future');
    }
  };
});

But frankly, I doubt many devs actually write/depend a thenable that cares which tick it's invoked in. As long as the core behavior of native promises is maintained (callbacks passed to then are fired in the next tick), then a lot of code is still going to behave correctly:

// proposed spec
// t = 0
Promise.resolve().then(() => {
  // t = 1
  return {
    then(onRes) {
      // t = 1, used to be 2
      onRes();
    }
  };
}).then(() => {
  // t = 2, used to be 3
});

And some promise-heavy projects will be considerably faster. That's a big deal.

[bakkot]

while I think that the then method of a thenable itself is relevant too

The only time this would matter is if you had a thenable whose behavior was synchronous, rather than scheduling work to be performed in the future as the built-in Promise.prototype.then does. I think you should not do that, and then the problem does not arise.

Note that you can call the then method synchronously, so there's already not a guarantee that then is invoked asynchronously.

[raulsebastianmihaila]

You're unfortunately comparing snippets with two very different behaviors

I think you're mischaracterizing my example. I don't understand why you introduced a getter as my examples didn't include one. Also a piece of your example is incorrect. You wrote:

// t = 0
Promise.resolve().then(() => {
  // t = 1
  return {
    then() {
      // t = 1
      console.log('logged in the future');
    }
  };
});

If understand you're intention, this code is incorrect because console.log('logged in the future'); is actually called in t2, not t1. This can be proven with this example:

console.log('t0');
Promise.resolve().then(() => {
  console.log('t1');
  Promise.resolve().then(() => { console.log('t2'); });
  return {
    then() {
      console.log('t3');
    }
  };
});

In any case it seems to me you've made the discussion more complicated for reasons I don't understand.

[raulsebastianmihaila]

Note that you can call the then method synchronously, so there's already not a guarantee that then is invoked asynchronously.

The mechanics of promises can't call the then method synchronously and I specifically mentioned that you can have consistent expectations when interacting with promises as this is what is relevant in this topic.

[bakkot]

Yes. My point is that as a producer of thenables you should not design on the assumption that then is only called asychronously, because this is not true; rather, your then should always schedule work to be done asynchronously. And if producers of thenables adhere to this principle, then consumers of thenables will not need to know whether await invokes then itself synchronously or asychronously.

[raulsebastianmihaila]

Yes, and my point is that fortunately you don't have to care about that because the promises behavior is consistent with regard to how it handles userland code.

[bakkot]

fortunately you don't have to care about that

My claim is that as a consumer you don't have to care about it either way. This PR would not change that.

[raulsebastianmihaila]

My claim is that as a consumer you don't have to care about it either way.

This part is not very clear to me because it does make changes relevant to producing, which makes it a breaking change that we should care about.

[bakkot]

It does not make changes relevant to producing. Producers already have to be aware that then may be invoked synchronously or asynchronously. This PR does not change that.

[raulsebastianmihaila]

Producers already have to be aware that then may be invoked synchronously or asynchronously.

Unfortunately I can not agree with that. The reason being that producers can rely on how promises work and this change will break such code. Take the small example I gave previously for executing code in a future tick. Even if there is universal agreement with your view on how thenables should be written in general, there are certainly exceptions where such snippets are used in practice as workarounds in codebases that are not well taken care of. I have seen it in real code (way before queueMicrotask was invented, given promises became standard 7 years ago). It's not that one would disagree with you on what the best practices should be for creating thenables. It's just that sometimes the creation of thenables is not the main focus, but instead they are created as part of a workaround or something like that which relies on the standard behavior of promises.

[bakkot]

It is simply a fact that then may be invoked synchronously or asynchronously. Yes, it's possible to write code which would break if if then were invoked synchronously but where the actual consumers don't actually do so. And it may well be that there is enough such code in existing code bases which would break with this change that we can't get away with changing it. But it may not be - that is an empirical question, not a question about mental models. We have successfully made similar changes before.

But there is separately a question of whether this change significantly complicates the mental model for authors of new code, which was what you were discussing above, and I claim it does not.

[raulsebastianmihaila]

This change does complicate the mental model because it introduces a new rule to follow. But I think this is less relevant nowadays than the other things I was discussing (like this being a breaking change, which is precisely what I started with).

[bakkot]

It does not introduce a new rule to follow.

[jridgewell]

This discussion is getting off track, please take it to matrix.

[bakkot]

Dug up where this change was introduced: domenic/promises-unwrapping#105

The concern was specifically that Promise.resolve(foo) could break invariants in the surrounding code. That's already out the window because of the synchronous Get(x, "constructor") in PromiseResolve and Get(resolution, "then") in Promise Resolve Functions, but I guess the idea was that this wouldn't matter in practice?

If we don't want to outright revert the above PR, we could instead add a fast-path in Promise Resolve Functions which says that if thenAction is the built-in Promise.prototype.then then it's invoked immediately rather than on the next tick (with custom exception handling). That avoids the problem in the linked issue and also keeps the old behavior for custom thenables which @raulsebastianmihaila is worried about. And it doesn't invoke any user code synchronously which was not already invoked synchronously, because the real Promise.prototype.then never invokes any user code synchronously - it would just happen after a single tick instead of after two. (EDIT: except for getters on the promise and the species constructor, I guess; subclassing strikes again.)

The cost is that this is a place userland thenables take an extra tick vs real promises, but since that's already true for await, I think that's fine.

[ljharb]

That seems like a pretty reasonable/safe/best-of-both-worlds compromise.

[jridgewell]

For what it's worth, this change would partially break my "hack" to detect promise adoption/redirect through node's async_hooks.
— #2772 (comment)

I think is already broken for async/await since #1250. Doing await nativePromiseWithMonkeyedThen does not call the monkey-patched then, it uses the internal slots to access data directly.

---

The concern was specifically that Promise.resolve(foo) could break invariants in the surrounding code. That's already out the window because of the synchronous Get(x, "constructor") in PromiseResolve and Get(resolution, "then") in Promise Resolve Functions, but I guess the idea was that this wouldn't matter in practice?
— #2772 (comment)

I found a few more comments about untrusted code in the repo:

1. Promise.resolve(thenable) executes thenable.then synchronously domenic/promises-unwrapping#105 (comment) (the linked issue)
2. Overriden thens? domenic/promises-unwrapping#42 (comment)
3. [[PromiseConstructor]] vs .constructor and instanceof domenic/promises-unwrapping#65 (comment)

In particular, it seems to boil down to:

const cast = Promise.resolve(untrustedThenable);
cast.then(untrustedFulfilled, untrustedRejected);

The desire is that at no point during this tick should untrusted code be run. But as #2772 (comment) points out, that's been broken for a while because the untrustedThenable's .constructor and .then are both accessed during this tick. It seems to me that invoking the then wouldn't be that bad.

Note that during the time these comments were made, the new Promise(executor) wasn't supposed to invoke the executor during this tick either, but that was changed too. The security guarantees softened to accommodate a more consistent API. We could decided that they should soften again, given they're already soft and it'll improve speed in both promise and async/await codebases.

Also, the untrustedFulfilled and untrustedRejected will still wait till the next tick to be invoked with the proposed changes. Casting an untrusted (and possibly badly behaving) thenable into a native promise will guarantee that cast.then calls its callbacks after a tick, so we don't have any zalgo issues.

---

PS. Promise.resolve has never been a good security guarantee. The following has been exploitable since ES2015:

const p = Promise.resolve();
p.then = () => {
  alert('Gotcha!');
};

const cast = Promise.resolve(p);
cast.then(); // Gotcha!

[mhofman]

I think is already broken for async/await since #1250. Doing await nativePromiseWithMonkeyedThen does not call the monkey-patched then, it uses the internal slots to access data directly.

Thankfully #1250 does not impact my use case since I only care about promises which are adopted by a user reachable promise, like as returned by an async function.

That seems like a pretty reasonable/safe/best-of-both-worlds compromise.

Agreed. That compromise also wouldn't break my fringe scenario, but more importantly, the more or less legitimate assumptions that custom then execute on a new tick / clean stack.

The desire is that at no point during this tick should untrusted code be run. But as #2772 (comment) points out, that's been broken for a while because the untrustedThenable's .constructor and .then are both accessed during this tick. It seems to me that invoking the then wouldn't be that bad.

Correct, it's already broken, but only in the case of evil promises using get traps. I believe the argument here is that it's a lot easier to write code that assumes the then is called in a new tick. It also would be a much harder to observe change if we did SameValue(thenValue, %Promise.prototype.then%), while reaping most of the benefits.

PS. Promise.resolve has never been a good security guarantee. The following has been exploitable since ES2015:

Right, and that's why I would really like if we could expose a clean IsPromise to userland as that would make it possible to be defensive against evil get constructor and get then shenanigans.

[bakkot]

Another alternative fast-path would be to use the same test as Promise.resolve and await: before reading resolution.then, first test IsPromise(resolution) && resolution.constructor === Promise, and if that passes then invoke the built-in PerformPromiseThen machinery on resolution directly, bypassing any check of .then.

This would have the advantage of skipping two observable lookups (of then and Symbol.species) in the common case of a native Promise. The constructor lookup is already present for native Promises because Promise.prototype.then calls SpeciesConstructor which does a lookup of constructor, so that lookup just gets moved around. It does incur an additional lookup (of constructor) for Promise subclasses which override then.

[jridgewell]

before reading resolution.then, first test IsPromise(resolution) && resolution.constructor === Promise, and if that passes then invoke the built-in PerformPromiseThen machinery on resolution directly, bypassing any check of .then.

Wouldn't this immediately call PerformPromiseThen with the now-verified Promise?

8. If Type(resolution) is not Object, then
  a. Perform FulfillPromise(promise, resolution).
  b. Return undefined.
9. If IsPromise(x) is true, then
  a. Let xConstructor be ? Get(x, "constructor").
  b. If SameValue(xConstructor, %Promise%) is true, then
    i. Let resolvingFunctions be CreateResolvingFunctions(promise).
    ii. Perform PerformPromiseThen(x, resolvingFunctions.[[Resolve]], resolvingFunctions.[[Reject]]).
    iii. Return undefined.
10. Let then be Completion(Get(resolution, "then")).
11. If then is an abrupt completion, then
  a. Perform RejectPromise(promise, then.[[Value]]).
  b. Return undefined.
12. Let thenAction be then.[[Value]].
12. If IsCallable(thenAction) is false, then
  a. Perform FulfillPromise(promise, resolution).
  b. Return undefined.
13. Let thenJobCallback be HostMakeJobCallback(thenAction).
14. Let job be NewPromiseResolveThenableJob(promise, resolution, thenJobCallback).
15. Perform HostEnqueuePromiseJob(job.[[Job]], job.[[Realm]]).
16. Return undefined.

[bakkot]

Wouldn't this immediately call PerformPromiseThen with the now-verified Promise?

That was the idea, yes. (Same as my earlier suggestion, just skipping some intermediate steps.)

PerformPromiseThen doesn't invoke any user code synchronously, so that seems fine.

[zloirock]

core-js implementation use the actual semantic. The feature detection does not contain this behavior.

However, sometimes I see issues about the order of promises resolution in similar cases, so I'm pretty sure that it will break something in the Web.

[mhofman]

@zloirock could you clarify "the"/"this"?

Does core-js follow the A+ rules (synchronous call) or the currently specified ES Promises (delayed call).

If taking the hybrid approach of only delaying non native promises and fast pathing native promise with no custom then, the only impact should be on programs that are particularly sensitive to the number of ticks.

[zloirock]

could you clarify "the"/"this"?

I mean core-js feature detection of Promise.

Does core-js follow the A+ rules (synchronous call) or the currently specified ES Promises (delayed call).

Sure, delayed call, like in the actual spec.

[jridgewell]

For interested parties, I've created https://github.com/tc39/proposal-faster-promise-adoption to track the proposal.