Implement async/await syntax #460
Comments
|
Having played a little bit with async/await being transpiled by babel: It will be nice if goja let you more easily:
|
What do you need it for? Which problem are you trying to solve? |
|
I will try to give you the concreate big example - k6 has a particular call group, which has two things it does:
Preface: I did try to differintiate between functions that are called asyncrhnously and js As part of us going toward more async APIs I did start grafana/k6#2728 which outlines a lot of the assumptions that are made in the Unfortunately (as mentioned in that issue) it is also used ... quite a lot. So the blank "this just doesn't work with asynchronous calls" isn't really on the table. Our current working variant is trying to make This means that we do not handle the cases of group("something", async () => {
somePromise.then(() => {
// this here is withing group and will not make the group_duration go up
})
apiWithCallback(() => {
// same as above
})
})This is under ... discussion and trying to figure out if things are even possible to be done. The other thing is that k6 does have predominantly sync APIs that we can not and will not break, which means that using them within While for I guess big parts of this also asks the question "how is goja going to restore the execution context if part of it is go code called by js that calls js?". And I guess you have more inside into this or am I just overthinking the problem. |
|
As far as I understand you need some kind of global context for async functions, something like this, but accessible from a Go function. I have now pushed an initial implementation of async/await into a branch and to solve your issue I propose the following:
This way you can access the same AsyncContext value from any Go function that is called from an async function. Would that be enough? |
|
This solution seems sufficient to me at this time. I have tried the branch and some basic functionality seems to work 🎉 - I will try to report issues once there is open PR as otherwise I think you probably expect that there are some things that won't work. On the AsyncContext proposal I have some comments:
None of the above are IMO a thing to not do it this way but just things I noticed. I also was wondering if instead of getting something like that we can't get HostMakeJobCallback and if that won't give us enough info to basically implement what you propose but on the "host" side while keeping goja more spec compliant. I then went down a rabbit hole based on
Which seems pretty strange. I guess we can argue k6 is a very ... bad web browser and goja just needs to let us define this just in case somebody makes a browser with goja 🤷 . But my idea was that implementing more of the host layering points connected with jobs might be a better way to implement this while keeping spec compliant and not adding ambiguous fields. In particular with HostMakeJobCallback I should be able to (when it's called) check what "context" I am currently in and wrap the new callback in it. Practically propagating downwards. And then un set it once the callback returns. I would guess this is close to what: import { Counter } from "k6/metrics";
import { group } from "k6";
const delay = () => { return Promise.resolve(); }
let l = new Counter("my coutner")
let originalThen = Promise.prototype.then;
Promise.prototype.then = function(onResolved, onRejected) {
let o = onResolved
onResolved = function(res) {
group("coolgroup", () => { // this in practice will need to get hte group name
o(res);
})
}
// https://stackoverflow.com/questions/55413715/cant-i-overwrite-the-then-function-of-promise
return originalThen.call(originalThen.call(this, onResolved, onRejected), undefined, error => {
console.log(`upload the error to server: ${error}`);
throw error;
});
}
export default () => {
group("coolgroup", () => {
l.add(1) // nice
delay(1).then(() => {
l.add(1)// not so nice
})
})
}does 🤔 |
Please do report them (maybe just as comments here rather than separate issues). It passes the current (and the most recent) tc39 tests, and I have no better means of testing it.
It's just a variable name, not a formal definition, I see no problem with that, but if you have a better name in mind I'm open to suggestions.
I don't mind that in general (and I don't really care about what the specification says in this case), but I'm struggling to see how this would help you. The key point here is to get some reference to the group (at least its name), how would you do that assuming the hook is implemented? |
HostMakeJobCallback will be called whenever So I will have access to the current state including whether or not I am currently in a group. At this point I can:
We already need to keep some global state as the Runtime has no place for that, so that already is a solved problem. |
Yes, but that also means it will be called on every async function sleep(timeout) {
await new Promise((resolve, reject) => {
setTimeout(resolve, timeout);
})
}
async function runInGroup() {
sleep(2000).then(() => {
console.log("surprise, I'm still in the group!");
})
await sleep(1000);
}
let startTime = new Date();
runInGroup().then(() => {
console.log("group function finished, took " + (new Date() - startTime) + " ms");
});I tried looking up the story behind HostMakeJobCallback to see what it was for, couldn't find anything except this: https://html.spec.whatwg.org/multipage/webappapis.html#hostmakejobcallback. But I've got an impression it was not designed for our purpose. How about this: type AsyncContextTracker interface {
Suspended() (ctx interface{}) // called whenever an async function is suspended due to await. The returned `ctx` will be supplied to Resumed() when the function resumes.
Resumed(ctx interface{})
}
runtime.SetAsyncContextTracker(/* an instance of AsyncContextTracker*/)I believe this solves your problem and is very cheap to implement (both from the complexity and performance impact point of view). |
I see what you mean there. Thinking about it:
async function inGroup() {
await Promise.All([
doSomethingReturningPromise.then(() => { /* here we don't know we are in the group */ })
doSomethingElseReturningAPromise.then(() => { /* here we don't know we are in the group */ })
])
}like in this case the
group("name", async () => {}).then(()=> {console.log("group ended")})I can see how we can return a promise from All of this is getting a bit too messy though, and I feel like I am blocking this on things we can add or enhance later. From the very beginning of looking at this I have repeated to others that "what we can do depends on what we have as information". And working with the babellified async/await gave us really ... no additional information so we scaled down what Looking at this now it seems to me I will be able to do stuff even without any additional support, but just by rewriting But I can also see how Both the AsyncContext* ideas and the hostmakejobcallback can be beneficial. So I am 👍 on whichever one you decide to implement. I would just come back with more questions and ideas once we have something that we couldn't figure out how to implement. I don't expect we will figure out every possible case for everybody right from the start. |
|
I just tried to get what tc39 test fail, but ran into some problems where goja returns an error and then babel transpiles, and we get a different error 🤦. But while I fixed that I also noticed that you still skip a bunch of the async tests Lines 277 to 281 in a008913 Is this on purpose or did you just miss it? (maybe locally you have them enabled 🤷 ) |
I think you should document that
Rewriting
For now I'm leaning towards implementing the AsyncContextTracker. I think I'm also going to implement async stack traces using a similar approach to this. I think this will be useful.
|
No, I've missed this section completely. Thanks for pointing out. |
|
I currently experience a panic that happens if a constructor calls a function that was package goja
import (
"testing"
)
func TestConstructorPanic(t *testing.T) {
// this panics with async-await branch
r := New()
c := func(call ConstructorCall, rt *Runtime) *Object {
c, ok := AssertFunction(rt.ToValue(func() {}))
if !ok {
panic("wat")
}
if _, err := c(Undefined()); err != nil {
panic(err)
}
return nil
}
if err := r.Set("C", c); err != nil {
panic(err)
}
if _, err := r.RunString("new C()"); err != nil {
panic(err)
}
}
func TestConstructorPanic2(t *testing.T) {
// this panics before async-await as well
r := New()
c := func(call ConstructorCall, rt *Runtime) *Object {
if _, err := rt.RunString("(5)"); err != nil {
panic(err)
}
return nil
}
if err := r.Set("C", c); err != nil {
panic(err)
}
if _, err := r.RunString("new C()"); err != nil {
panic(err)
}
}The panic I get is both of those work if this is a Given the panic it seems like the stack doesn't get restored after the function call inside the constructor, but it does if it was inside a function |
|
I've pushed a fix to the branch. This actually has nothing to do with async, but the change won't apply clearly to master so I did not do a separate fix in master hoping this branch will be merged soon. Let me know if there is any problem. |
Everything seems to work, and I have now opened a WIP PR for k6 using it. So this seems okay. But I will be on PTO for the rest of the year, so I might not be available to give you any more specific feedback :( I would argue you can merge this as is, unless you have some concerns. And then expand with the |
|
If you mean |
|
Ok, I think it is now ready to be merged. There may be some bugs, but in order for me to fix them they need to be found first :) Unless you have any objections @mstoykov I'll merge it shortly. |
|
I am on PTO until the end of the year, but I did bump the version and re ran the test. I needed to fix some asserts around stacktraces, by deleting the last/first file which ... seems to be superficial and is now gone, so seems good to me. But I will only be able to look into using So I guess 👍 and happy holidays 🎉 ! |
|
Cheers, to you too! |
* Implemented async/await. See dop251#460. (cherry picked from commit 33bff8f)
async/await syntax was added in ES2017 and makes the asynchrnous and non-blocking Promises from ES2015 more syncrhnous in syntax while still being non-blocking.
The issue is about adding basic async/await syntax:
asyncmarking async functions in which you can useawaitto signal to the VM that it should unwind the stack and come back to this particular point after the provided promise is resolved or rejected.This like #436 is blocked on the ability of the goja runtime save and restore the execution context.
Things that should (likely) not be part of this issue:
This also blocks #430 as mentioned in it.
The text was updated successfully, but these errors were encountered: