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

Top-level await incorrectly awaits promise continuation in REPL #43777

Closed
tniessen opened this issue Jul 11, 2022 · 5 comments · Fixed by #43827
Closed

Top-level await incorrectly awaits promise continuation in REPL #43777

tniessen opened this issue Jul 11, 2022 · 5 comments · Fixed by #43827
Labels
confirmed-bug Issues with confirmed bugs. promises Issues and PRs related to ECMAScript promises. repl Issues and PRs related to the REPL subsystem.

Comments

@tniessen
Copy link
Member

Version

18.5.0

Platform

Linux 5.13.0-Ubuntu x86_64 x86_64 x86_64 GNU/Linux

Subsystem

repl

What steps will reproduce the bug?

In REPL:

const foo = async () => 123;

foo();

await foo();

typeof (await Promise.resolve(foo))();

(await Promise.resolve(foo))();

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

> const foo = async () => 123;
undefined
> foo();
Promise {
  123,
  [Symbol(async_id_symbol)]: 33,
  [Symbol(trigger_async_id_symbol)]: 5
}
> await foo();
123
> typeof (await Promise.resolve(foo))();
'object'
> (await Promise.resolve(foo))();
Promise {
  123,
  [Symbol(async_id_symbol)]: 108,
  [Symbol(trigger_async_id_symbol)]: 5
}

Note that typeof prints 'object', which is correct and matches Promise { ... }.

What do you see instead?

> const foo = async () => 123;
undefined
> foo();
Promise {
  123,
  [Symbol(async_id_symbol)]: 33,
  [Symbol(trigger_async_id_symbol)]: 5
}
> await foo();
123
> typeof (await Promise.resolve(foo))();
'object'
> (await Promise.resolve(foo))();
123

Note that typeof prints 'object', which is correct, but the value of the expression is shown to be 123 in the REPL, which is a 'number'.

Additional information

Chrome and Edge seem to handle this as I'd expect. Firefox seems to handle this like Node.js (so I'm assuming it's a bug).

@tniessen tniessen added repl Issues and PRs related to the REPL subsystem. promises Issues and PRs related to ECMAScript promises. labels Jul 11, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jul 11, 2022

@guybedford FYI

@guybedford
Copy link
Contributor

@tniessen the repl top-level await doesn't use v8 directly and instead runs through an emulation layer. Short of someone to do the work to refactor that hack, this is a bug in the emulation layer originally implemented I believe by Gus.

The transform is here - https://github.com/nodejs/node/blob/main/lib/internal/repl/await.js, and the tests for the rewriting are here - https://github.com/nodejs/node/blob/main/test/parallel/test-repl-preprocess-top-level-await.js.

I would suggest adding the failing line to the transform tests to debug further what it is actually executing and why it is giving the wrong result. Working out the failing test would be a good start.

@tniessen tniessen added the confirmed-bug Issues with confirmed bugs. label Jul 13, 2022
@tniessen
Copy link
Member Author

Thank you for the info @guybedford!

I might be missing something but the transform seems to always wrap the code in an async function:

const wrapPrefix = '(async () => { ';
const wrapped = `${wrapPrefix}${src} })()`;

The REPL then seems to await the return value of that function:

node/lib/repl.js

Lines 571 to 575 in a055337

if (self.useGlobal) {
result = script.runInThisContext(scriptOptions);
} else {
result = script.runInContext(context, scriptOptions);
}

let promise = result;

node/lib/repl.js

Lines 617 to 621 in a055337

(async () => {
try {
const result = await promise;
finishExecution(null, result);
} catch (err) {

In this case, the value that the REPL should evaluate to is a Promise itself, and returning a Promise from the added async function causes the above snippet to incorrectly await the Promise.

I would suggest adding the failing line to the transform tests to debug further what it is actually executing and why it is giving the wrong result. Working out the failing test would be a good start.

I'm not sure what I'd expect the transform to produce. Maybe something like (async () => { return (await Promise.resolve(foo)); })().then((x) => x()) (which seems dangerously complicated).

@devsnek
Copy link
Member

devsnek commented Jul 13, 2022

I think the solution is to rewrite all return v to return { value: v } and then get that value property on the outside.

tniessen added a commit to tniessen/node that referenced this issue Jul 13, 2022
@tniessen
Copy link
Member Author

@devsnek That seems like an amazingly simple solution to me! See #43827.

nodejs-github-bot pushed a commit that referenced this issue Jul 15, 2022
Fixes: #43777

PR-URL: #43827
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
danielleadams pushed a commit that referenced this issue Jul 26, 2022
Fixes: #43777

PR-URL: #43827
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this issue Jul 31, 2022
Fixes: #43777

PR-URL: #43827
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this issue Jul 31, 2022
Fixes: #43777

PR-URL: #43827
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
Fixes: nodejs#43777

PR-URL: nodejs#43827
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
Fixes: nodejs/node#43777

PR-URL: nodejs/node#43827
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. promises Issues and PRs related to ECMAScript promises. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants