1410820 - top-level await evaluation should be handled as regular evaluation

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce:
1. Open the console
2. Evaluate 
```js
foo = async () => 42;
await foo();
```


Expected results:
I get `42` printed in the console

Actual results:
I get the following syntax error: 
`SyntaxError: await is only valid in async functions and async generators`

---

Note that this example is simple, but we could be dealing with real asynchronous behaviour , e.g.

```js
sleep = async (t) => new Promise(res => setTimeout({res(t)}, t));
await sleep(2000);
```

In this case, what should be printed ?
I think it's mandatory to give user immediate feedback (so maybe, display the promise right away), and then, when the promise resolves, its resolved value. The tricky part is how you make sense of that result, how do you connect it to the code the user evaluated.
Think of a Promise that resolves after 30s, in an application where you have many logs, it can be confusing to print a number out of the blue, without the context.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

This is working in Chrome, and they seem to print the result of the Promise when it resolves (and never the Promise).
We can have the same thing as a first step, but I'd rather find something more meaningful

Comment 3

[Alexandre Poirot [:ochameau]]

Chrome behavior is meaningful.

`(async () => {})()` returns a Promise. Calling a async function returns a promise.
So in your example: `sleep(2000)` should display a promise. That's what we already do and is correct.

`await (async () => {return "foo"})()` returns the async function result, not a Promise. Here it returns "foo".
So in your example: `await sleep(200)` should display 2000.

I think what is disturbing here is that we would have asynchronous console evaluation.
You can see that chrome allows mixing up evaluations.

Execute all these steps in individual evaluations quickly:
* sleep = async (t) => new Promise(res => setTimeout({res(t)}, t))
* await sleep(2000);
* 1

You will get this as console output:
> sleep = async (t) => new Promise(res => setTimeout(()=>{res(t)}, t))
< async (t) => new Promise(res => setTimeout(()=>{res(t)}, t))
> await sleep(2000)
> 1
< 1
< 2000

`await sleep(2000)` and `1` end up being mixed up...

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Execute all these steps in individual evaluations quickly:
> * sleep = async (t) => new Promise(res => setTimeout({res(t)}, t))
> * await sleep(2000);
> * 1
> 
> You will get this as console output:
> > sleep = async (t) => new Promise(res => setTimeout(()=>{res(t)}, t))
> < async (t) => new Promise(res => setTimeout(()=>{res(t)}, t))
> > await sleep(2000)
> > 1
> < 1
> < 2000
> 
> `await sleep(2000)` and `1` end up being mixed up...

Yes, that's the part that I don't like.
I think we could print the expression again if there were new logs between user input and the promise resolution.
The following sequence: 
- await sleep(5000)
- await sleep(1000)
- 1 + 2

would print

❯ await sleep(5000)
----------------------
❯ await sleep(1000)
----------------------
❯ 1 + 2
----------------------
← 3
----------------------
❯ await sleep(1000)
----------------------
← 1000
----------------------
❯ await sleep(5000)
----------------------
← 5000


Or, we could display the await expression right away, and when the result comes in, show it before the result, and remove the initial evaluation message.
That would give us, first:

❯ await sleep(5000)
----------------------
❯ await sleep(1000)
----------------------
❯ 1 + 2
----------------------
← 3
----------------------

1s later:

❯ await sleep(5000)
----------------------
❯ 1 + 2
----------------------
← 3
----------------------
❯ await sleep(1000)
----------------------
← 1000

4s later:

❯ 1 + 2
----------------------
← 3
----------------------
❯ await sleep(1000)
----------------------
← 1000
----------------------
❯ await sleep(5000)
----------------------
← 5000


The downside to this, would be that timestamps would be shuffled:

11:50:02  ❯ 1 + 2
----------------------
11:50:02  ← 3
----------------------
11:50:01  ❯ await sleep(1000)
----------------------
11:50:02  ← 1000
----------------------
11:50:00  ❯ await sleep(5000)
----------------------
11:50:05  ← 5000

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Some people also suggest having a button in the result message that would scroll you to the evaluation message.
Maybe a tooltip on the result icon would be enough ? Not really discoverable though.

Comment 6

[Julien Wajsberg [:julienw]]

I like your second proposal, but I think we need an information like "awaiting promise resolution...", possibly with "promise" being a rep to the underlying promise. When the promise is resolved or rejected, we'd add a new line right after the information with the result -- or an exception in the case of the rejection.

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> I like your second proposal, but I think we need an information like
> "awaiting promise resolution...", possibly with "promise" being a rep to the
> underlying promise. 

`await sleep(1000);` doesn't return a promise. So there is no meaningful promise to be displayed here.
Yes, having a message would be great. But it will be quite challenging to know when to display it.

I think we should first focus on making "await" work with the imperfect behavior,
and then try to see if we can followup with something even better than chrome.

It is already quite unclear how to make the console actor compile its evaled scripts as async function.
The console actor relies on DebuggerAPI's executeInGlobalWithBindings and evalWithBindings:
http://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#1399-1401
      result = frame.evalWithBindings(string, bindings, evalOptions);
    } else {
      result = dbgWindow.executeInGlobalWithBindings(string, bindings, evalOptions);

And it doesn't seem to support any option to make the script be compiled as async function.

executeInGlobalWithBindings is implemented over here:
  http://searchfox.org/mozilla-central/source/js/src/vm/Debugger.cpp#9747

evalWithBindings is here:
  http://searchfox.org/mozilla-central/source/js/src/vm/Debugger.cpp#8679

Jim, Can we make executeInGlobalWithBindings compile the given script as async function?
If not, any other idea on how to address this bug?

Comment 8

[Tim Nguyen :ntim]

The simplest thing would be making hovering the output also highlight the input with it for async evaluation.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tim Nguyen :ntim from comment #8)
> The simplest thing would be making hovering the output also highlight the
> input with it for async evaluation.

This works only for close messages. If you log 100 messages between the time you evaluated `await` and the promise resolution, there is a high risk the initial evaluation is out of sight

Comment 11

[Arthur Edelstein [:arthur]]

It seems to me the behavior should be the following:

❯ await sleep(2000);
*console waits for 2000 ms*
← 2000
❯ 1
← 1
❯ sleep(3000);
← Promise { <state>: "pending" }
❯ await $_;
*console waits for pending promise to resolve*
← 3000

In other words, the console behavior should be strictly single-threaded. When an input contains an await the console will need to pause until the promise resolves. This approach mimics the model of how async/await actually works in scripts. No timestamps would be shuffled. (Other suggestions in this ticket seem to be multi-threaded which look confusing to me and inconsistent with the coroutines model.)

Conceptually, this could be implemented by wrapping every input in an async function:

(async function() {
  return eval(inputString);
})();

Of course, that means there needs to be a way to "abort" the await so the console doesn't hang forever. Maybe a button or command-key could be used for this.

Comment 12

[Arthur Edelstein [:arthur]]

FWIW, I agree there should be some kind of user feedback for a long-pending promise. Maybe some text in a different font would temporarily appear saying "awaiting Promise" and then disappear after the Promise resolves and `await` returns a value.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> (In reply to Tim Nguyen :ntim from comment #8)
> > The simplest thing would be making hovering the output also highlight the
> > input with it for async evaluation.
> 
> This works only for close messages. If you log 100 messages between the time
> you evaluated `await` and the promise resolution, there is a high risk the
> initial evaluation is out of sight

I'd still prefer this to somehow blocking console execution while waiting for the resolution

Comment 14

[Tim Nguyen :ntim]

How about adding a button in the output line to jump back to the input line in the console ?

Comment 15

[Jim Blandy :jimb]

I strongly encourage the folks suggesting behaviors to play around with Chrome's console for a bit, if you haven't. There are some limitations, which I mention below, but I think the interaction is quite nice, and I don't find myself agreeing with comment 11 that the console should block.

> Jim, Can we make executeInGlobalWithBindings compile the given script as async function?
> If not, any other idea on how to address this bug?

It would be simple for Debugger to offer functions that are basically equivalent to obtaining the original Function, AsyncFunction, and AsyncGeneratorFunction constructors and then applying them to text the user supplied.

Then evaluating an expression in the console would:
- Wrap that text up as the body of an async function that returns the value of the expression
- Call that function (perhaps passing the injected utility bindings like `$` as arguments?).
- Take the resulting promise and arrange for the console to display its resolution when available.

But this isn't good enough. When stopped in a debuggee stack frame, evaluation is supposed to occur in the environment of that stack frame. We need a way to permit the `async` keyword in evaluations in an existing scope. If the frame has a local variable `x`, then evaluating:

    await sleep(10000).then(() => x);

should wait 10 seconds and then print the value of that local `x`.

The constructors mentioned above can create functions, but they can't site them in the midst of some new frame's scope. So that's not enough.

I note that Chrome has a bug here: an expression that uses `await` evaluated at the top level presents the expression's value once the `await`s are resolved, but when evaluated in a paused frame's scope, it seems to evaluate to a Promise of the expression's value. This is completely incorrect: the value of an await expression is the value of the promise it is passed, not a promise of that. Somehow, whatever techniques Chrome is using to handle expressions that use `await` is not quite correct.

On the protocol front: Allowing further evaluation while `await` expressions are pending will require some protocol changes, since now console evaluation can effectively provide unsolicited notifications of newly resolved values.

Comment 16

[Masatoshi Kimura [:emk]]

What happens if async functions block the console and something (e.g. the web page) call console.log() during the async function execution?

Comment 17

[Arthur Edelstein [:arthur]]

(In reply to Jim Blandy :jimb from comment #15)
> I strongly encourage the folks suggesting behaviors to play around with
> Chrome's console for a bit, if you haven't. There are some limitations,
> which I mention below, but I think the interaction is quite nice, and I
> don't find myself agreeing with comment 11 that the console should block.

Thanks for pointing out Chrome's implementation. I tried it and I agree that it's nice that the console doesn't block. So I retract comment 11. :)

Comment 18

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Update_affecteds

Comment 19

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I agree with Jim's comment #15, Chrome's implementation is quite nice.

In order to move work on this bug forward, I am suggesting the following:

1) Implement this the same way as Chrome does
2) Make sure we fix the bug when executing an expression in a paused frame's scope
3) Come up with improvements in follow-ups

Honza

Comment 22

[Jason Laster [:jlast]]

Here is a babel prototype, which is good for testing some of the interactions. It might also make sense to land.
https://github.com/devtools-html/debugger.html/pull/6742


It supports top-level awaits - http://g.recordit.co/KTkgjovUO0.gif
It also supports local bindings - http://g.recordit.co/5wCoWg1r6k.gif

The approach takes an expression `await sleep(10)` and translates it to `(async () => { return sleep(10) })().then(r => console.log(r)`

The benefit to this approach is that it is easier to understand what will happen because the output is just JS and it does not require any special server or RDP support. With that said, I could see how long term there are trade-offs similar to console bindings.

Comment 23

[Jason Laster [:jlast]]

Weird, did i change the status of the bug?
Also, the gif links might require refreshing once to see. They should also just work in the github PR.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

So, re-using Jason work for wrapping input containing top-level await, I'm able to have something naive working on the console (which is only when not paused, with only one "await" at a time).

So when a user evaluates: 

```js
await new Promise(resolve => setTimeout(() => resolve("hello"), 1000))
```

we transform it into: 

```js
(async () => {
  return await new Promise(resolve => setTimeout(() => resolve("hello"), 1000));
})()
```

and evaluates it with the DebuggerServer.

Since this returns a Promise, I then use the console debugger `onPromiseSettled` hook to detect when the created wrapping function can give us the resolved value (or rejection cause). This requires the evalWindow to be added as a debuggee, which we can then remove when the promise is settled.

Good.

Now this is fine when the user only evaluates one top-level await expression. If there's multiple one, it gets a bit tricky as we can't re-declare dbg.onPromiseSettled each time (as it overrides what was done in the previous evaluation).
It's also tricky to know when we should add the evalWindow as a debuggee and when it's safe to remove it.

One solution is to always have the evalWindow as a debuggee and declare dbg.onPromiseSettled in the WebConsole Actor's init function. I guess this is not something we'd want for performance reason.

So I was wondering if we could have a similar onPromiseSettled hook on directly on the Promise, and not the global holding it. This way, we wouldn't have to deal with adding the whole evalWindow as a debuggee and would save some operation / make the code more clear.

Jim, could this be an option, and how hard would it be to implement such thing ? There might be more simple/obvious ways to do this I'm not aware of, and if it's the case, I'd love to hear about them.

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

So, I feel quite dumb after talking with Jason.
On the promise we get back from the server, we can also use `unsafeDereference` which gives us the actual promise object.
Then we can simple await for it to be resolved/rejected and send the value/cause to the console.
Multiple concurrent top-level await expression works like a charm.

Now, the question is, is there any risk using this on the server ? I'd tend to say no since this is a promise _we_ created (by wrapping into the async function), but again, I may miss something very obvious.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Updated bug title and user story as initial-basic support might happened in Bug 1491354

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1410820 - top-level await evaluation should be handled as regular evaluation; r=bgrins,jlast.

This patch turns the current top-level handling, which relies on
the console API to print the result of the await expression, into
something natively handled by the server.

First, we don't add a .then handler to the generated async iife by
the mapper. We also removes the case we added in the JsTerm to *not*
print the result for top-level await expression.

In order to make the server capable of handling generated async iife
caused by the mapper, we send to evaluateJsAsync the `mapped` object
that `mapExpression` returns. This way, the server can check if an
expression was originally a top-level await.

If it is the case, we get the promise from the async iife and wait for
it to settle. If it resolves, we simply return the result, as a grip,
to the client. If it rejects, we return a special packet indicating to
not print anything to the client. The error will be reported by the
engine as `uncaught exception: …`.

We add several tests and modify the existing one to make sure we handle
top level await correctly in different situation (resolving, rejecting,
when paused in the debugger, when using $_, …).

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1410820 - Enhance webconsole test helpers; r=bgrins.

This will allow to wait for specific message (warning, error, result, …).
This patch also fixes 2 tests that were already passing an erroneous selector
to waitForMessage.

Depends on D6903

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1410820 - Extensively test top-level await evaluation; r=bgrins.

We add several tests and modify the existing one to make sure we handle
top level await correctly in different situation (resolving, rejecting,
when paused in the debugger, when using $_, …).

Depends on D6925

Comment 30

[Brian Grinstead [:bgrins]]

Comment on attachment 9012219 [details]
Bug 1410820 - Enhance webconsole test helpers; r=bgrins.

Brian Grinstead [:bgrins] has approved the revision.

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 9012221 [details]
Bug 1410820 - Extensively test top-level await evaluation; r=bgrins.

Brian Grinstead [:bgrins] has approved the revision.

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 9012163 [details]
Bug 1410820 - top-level await evaluation should be handled as regular evaluation; r=bgrins,jlast.

Brian Grinstead [:bgrins] has approved the revision.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fabe4062700
top-level await evaluation should be handled as regular evaluation; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/a7edd08b7f6d
Enhance webconsole test helpers; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/95ae87f52148
Extensively test top-level await evaluation; r=bgrins.

Comment 34

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6fabe4062700
https://hg.mozilla.org/mozilla-central/rev/a7edd08b7f6d
https://hg.mozilla.org/mozilla-central/rev/95ae87f52148