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

async/await/Promise scheduling order wrong #31552

Closed
trevordixon opened this issue May 23, 2019 · 8 comments · Fixed by #32462 or microsoft/tslib#70
Closed

async/await/Promise scheduling order wrong #31552

trevordixon opened this issue May 23, 2019 · 8 comments · Fixed by #32462 or microsoft/tslib#70
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@trevordixon
Copy link

trevordixon commented May 23, 2019

TypeScript Version: 3.3, 3.5.0-dev.20190523

Search Terms: async await Promise order scheduling microtask

Code

async function a(msg) {
  await Promise.resolve();
  console.log(msg);
}

function b(msg) {
  Promise.resolve().then(() => {
    console.log(msg);
  });
}

a('1');
b('2');

Expected behavior: Log 1 then 2. Chrome and Node.js get this right. Babel's transpiled JS also gets it right.

Actual behavior: Logs 2 then 1.

Playground Link: https://www.typescriptlang.org/play/#src=async%20function%20a(msg)%20%7B%0D%0A%20%20await%20Promise.resolve()%3B%0D%0A%20%20console.log(msg)%3B%0D%0A%7D%0D%0A%0D%0Afunction%20b(msg)%20%7B%0D%0A%20%20Promise.resolve().then(()%20%3D%3E%20%7B%0D%0A%20%20%20%20console.log(msg)%3B%0D%0A%20%20%7D)%3B%0D%0A%7D%0D%0A%0D%0Aa('1')%3B%0D%0Ab('2')%3B%0D%0A

Babel Link: https://babeljs.io/repl/#?babili=false&browsers=&build=&builtIns=false&spec=false&loose=false&code_lz=IYZwngdgxgBAZgV2gFwJYHsI2ACgLYgDmAlDAN4BQM2A7sKsjAAoBO6eqIApgHQtch0AGwBuXHMQDcVGFEyChvIekL4iUigF8KFRCgxYARmpLkZrdp178FYiT2QALLhBwSYAXgB8Z6tTkQCkoqJhrUmhraFLgA5ACMMRrGMQBMidJAA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=true&fileSize=false&timeTravel=false&sourceType=script&lineWrap=true&presets=es2015%2Ces2017&prettier=false&targets=&version=7.4.5&externalPlugins=

Related Issues: Didn't find any.

@fatcerberus
Copy link

Does the spec require Promise.resolve()s to settle in FIFO order? I agree the deviation in behavior from V8 (and Babel) is surprising; just wondering if ES makes any guarantees here.

@trevordixon
Copy link
Author

trevordixon commented May 23, 2019

If my understanding about the event loop and microtask queue scheduling is correct, then yes, Typescript's behavior is wrong.

My meager understanding is based on what I learned reading https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/. That article doesn't address async/await functions specifically, but I'm pretty sure await Promise.resolve(); ... and Promise.resolve().then(...) should both queue microtasks that are executed FIFO at the end of the event loop.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 23, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.6.0 milestone May 23, 2019
@rbuckton
Copy link
Member

The reason is that we don't depend on Promise.resolve because we allow ES5 down-level async functions to specify their Promise implementation of choice, which didn't rely on resolve existing on a custom Promise implementation. It seems the extra delay is because these two expressions are not equivalent:

const p = Promise.resolve();
Promise.resolve(p).then(onfulfilled); // 1
new Promise(resolve => resolve(p)).then(onfulfilled); // 2

Both (1) and (2) perform state adoption of p, but (2) takes an extra turn. I will investigate modifying our __awaiter implementation to do the same thing as Promise.resolve without depending on resolve so that we can continue to support downlevel scenarios.

@rbuckton
Copy link
Member

It seems this is because PromiseResolve in the ECMAScript spec "cheats" for native promises:

...
2. If IsPromise(x) is true then,
  a. Let xConstructor be ? Get(x, "constructor").
  b. If SameValue(xConstructor, C) is true, return x.
...

Where IsPromise checks for the presence of a [[PromiseState]] internal slot.

@fatcerberus
Copy link

I actually remember reading something about this somewhere, to the effect of:

  1. Promise.resolve() returns an "already resolved" promise, vs.
  2. new Promise(y => y()) returns a promise which "resolves immediately"

...and these two cases, while they sound identical (in both cases you get the callback on the next tick), are actually subtly different. The JS promise semantics are really, really weird.

@rbuckton
Copy link
Member

Proposed change:

var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
    return new (P || (P = Promise))(function (resolve, reject) {
        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
        function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
        step((generator = generator.apply(thisArg, _arguments || [])).next());
    });
    function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
};

@rbuckton
Copy link
Member

I believe Promise.resolve(CustomPromiseA.resolve()) would also take two turns because CustomPromiseA is not a native promise, even if it is a Promise/A+ -compatible promise implementation.

@fatcerberus
Copy link

Yeah, Promise.resolve(nativePromise) is idempotent. Promise.resolve(customPromise) isn't - it has to be chained via .then. That much I knew. What I am still confused by to this day is the apparently subte difference between Promise.resolve(x) and new Promise(resolve => resolve(x)). But that's not relevant to this issue, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
5 participants