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

Normative: handle broken promises in AsyncGenerator.prototype.return #2683

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Mar 5, 2022

Fixes #2412 by trapping the exception and using it to reject the promise returned by the call to AsyncGenerator.prototype.return.

Background 1: broken promises

You can make a promise which throws when you try to await or Promise.resolve it:

let brokenPromise = Promise.resolve(42);
Object.defineProperty(brokenPromise, 'constructor', {
  get: function () {
    throw new Error('broken promise');
  }
});

(see PromiseResolve step 1.a.)

If you await such a value, or pass it to Promise.resolve, it will synchronously throw an exception.

Background 2: AsyncGenerator.prototype.return

Generators have a .next() method which asks for the next value. Async generators have the same thing, except it returns a promise. Because the generator might be blocked on an await when you call .next(), they have an internal queue of requests to service once they're unblocked.

Generators also have a .return(x) method, used for cleanup, which injects a return completion at the current yield and resumes execution of the generator. If called before the generator has started, or after it's finished, the call to .return returns { done: true, value: [the value passed to .return] }.

Async generators also have a .return, with some differences:

  • It returns a promise
  • As with .next, it's queued (in fact it shares the queue with .next)
  • Once a call gets to the head of the queue, if the generator has completed, it will await the passed value before returning it in the result object (matching what happens if you do a normal return x; from an async generator).

The problem

Right now the spec doesn't account for the possibility of a broken promise passed to .return. It has an assertion that this doesn't happen, and that assertion can be violated:

let brokenPromise = Promise.resolve(42);
Object.defineProperty(brokenPromise, 'constructor', {
  get: function () {
    throw new Error('broken promise');
  }
});

{
  let gen = (async function* () { })();
  gen.return(brokenPromise);
}

// or
{
  let unblock;
  let blocking = new Promise(res => { unblock = res; });

  let gen = (async function* (){ await blocking; })();
  gen.next();

  // generator is now blocked on the blocking promise
  // further calls to `.next` or `.return` will be queued until it settles

  gen.return(brokenPromise);

  unblock();
}

Both of these calls violate an assertion in the spec: the first in AsyncGenerator.prototype.return step 8.a, the second in AsyncGeneratorDrainQueue step 5.c.ii.

Current behavior

XS never triggers the broken promise, and ChakraCore doesn't implement async generators.

For the first example, calling .return with a broken promise while the generator is not blocked: In SpiderMonkey, V8, and GraalJS, it synchronously throws. In JavaScriptCore, it returns a promise which never settles, and the exception disappears into the void.

For the second example, calling .return with a broken promise while the generator is blocked: all engines return a promise which never settles. In V8 and JavaScriptCore, the exception disappears into the void. In SpiderMonkey and GraalJS, the exception is thrown outside of any handlers (in SpiderMonkey this is observable in the console or with onerror).

Proposed behavior

The behavior this PR implements is to catch the exception and use it to reject the promise returned by AsyncGenerator.prototype.return. This doesn't match any implementation but seems like the only reasonable behavior, holding everything else constant.

@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Mar 9, 2022
@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Mar 28, 2022
@mhofman
Copy link
Member

mhofman commented Mar 28, 2022

I wish we didn't have this constructor lookup gross-ness

@ljharb ljharb added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Mar 28, 2022
@syg
Copy link
Contributor

syg commented Apr 5, 2022

I wish we didn't have this constructor lookup gross-ness

Das Leben ist kein Wunschkonzert :((

@bathos
Copy link
Contributor

bathos commented Apr 5, 2022

I wish we didn't have this constructor lookup gross-ness

I’ve wondered if there’s any reason not to change step 3 of SpeciesConstructor to “If Type(C) is not Object, return defaultConstructor”. This wouldn’t solve for the general problem, but it would get rid of the subset of cases like the following, I think. Currently this is all it takes to kill every async function / await expression in the current realm, no accessor needed:

Promise.prototype.constructor = null;
(async () => "Das Leben ist kein Wunschkonzert")();
// > Uncaught (in promise) TypeError: The .constructor property is not an object

@bakkot
Copy link
Contributor Author

bakkot commented Apr 5, 2022

@bathos If we're worried about code which is actively trying to break things that's not really an improvement (since you can still add a throwy accessor), and I don't think there's a reason to believe non-malicious code is going to be doing anything like that. So there's no clear-to-me benefit to such a change.

@bathos
Copy link
Contributor

bathos commented Apr 5, 2022

@bakkot Wasn’t thinking malicious so much as “why is this even here”: I figured a path by which ES code can “break syntax” (with no user value) would be worth eliminating even if the broader capability can’t be. But you’d have a better sense of the trade-offs than me.

kangwoosukeq pushed a commit to prosyslab/v8 that referenced this pull request Apr 28, 2022
As ecma262 normative change tc39/ecma262#2683,
exception thrown on PromiseResolve the broken promises need to be caught
and use it to reject the promise returned by
`AsyncGenerator.prototype.return`.

AsyncGeneratorReturn didn't handle the exception thrown by Await. This
CL add an exception handler around it and pass through the caught
exception to the returned promise and resume the generator by
AsyncGeneratorAwaitResume if the generator is not closed, otherwise
reject the promise by AsyncGeneratorReject and drain the queue.

Bug: v8:12770
Change-Id: Ic3cac4ce36a6d8ecfeb5d7d762a37a2e0524831c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3581158
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Chengzhong Wu <legendecas@gmail.com>
Cr-Commit-Position: refs/heads/main@{#80066}
@jugglinmike
Copy link
Contributor

@ljharb
Copy link
Member

ljharb commented Apr 29, 2022

@jugglinmike awesome! typically test PRs are merged prior to the spec PR being marked as "has tests".

@jugglinmike
Copy link
Contributor

Alrighty, I've merged the tests.

@ljharb ljharb added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels May 12, 2022
@ljharb ljharb requested review from syg, michaelficarra and a team May 12, 2022 18:46
@bakkot
Copy link
Contributor Author

bakkot commented Aug 3, 2022

https://github.com/tc39/proposal-faster-promise-adoption might actually make it impossible to have "broken promises" in this sense, so I want to hold off on landing this for a little while to see if we can fix this more comprehensively.

@ljharb
Copy link
Member

ljharb commented Aug 7, 2022

@bakkot should implementations be notified, since there's been (failing in latest v8 at least) tests for this in test262 for almost 3 months?

@bakkot
Copy link
Contributor Author

bakkot commented Jan 4, 2023

@ljharb On consideration we should just merge this. I'll rebase.

webkit-commit-queue pushed a commit to shvaikalesh/WebKit that referenced this pull request Dec 19, 2023
…omises

https://bugs.webkit.org/show_bug.cgi?id=266502
<rdar://problem/119734587>

Reviewed by Justin Michaud.

Before this change, abrupt completions of PromiseResolve [1] that arised during "constructor" lookup
were not handled properly in async functions and generators, resulting in exception propagation up
the call stack rather than rejecting a promise. That affected `await`, `yield`, and `return` called
with a broken promise (i.e. with throwing "constructor" getter).

Most likely, this is a regression from implementing async / await tick reduction proposal [2].

This patch guards "constructor" lookup with exception handling, ensuring that all call sites supply
onRejected() callback that is semantically equivalent to throwing an exception at that point, as per
spec. Invoking onRejected() synchronously, without extra microtask, is also required to match the
standard, V8, and SpiderMonkey.

Also, this change implements a proposal [3] to fix AsyncGenerator.prototype.return() called on a
broken promise, aligning JSC with V8.

[1]: https://tc39.es/ecma262/#sec-promise-resolve (step 1.a)
[2]: tc39/ecma262#1250
[3]: tc39/ecma262#2683

* JSTests/stress/async-function-broken-promise.js: Added.
* JSTests/test262/expectations.yaml: Mark 4 tests as passing.
* Source/JavaScriptCore/builtins/PromiseOperations.js:
(linkTimeConstant.resolveWithoutPromiseForAsyncAwait):

Canonical link: https://commits.webkit.org/272291@main
@bakkot bakkot added the editor call to be discussed in the next editor call label Apr 14, 2024
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Apr 24, 2024
@bakkot bakkot force-pushed the fix-async-generator-assertion branch from 9e725c9 to f32ef41 Compare May 2, 2024 16:35
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jun 20, 2024
@ljharb ljharb force-pushed the fix-async-generator-assertion branch from f32ef41 to 32e8809 Compare June 26, 2024 22:24
@ljharb ljharb merged commit 32e8809 into main Jun 26, 2024
8 checks passed
@ljharb ljharb deleted the fix-async-generator-assertion branch June 26, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect ! assertion in AsyncGeneratorAwaitReturn
7 participants