AsyncContextTracker.Resumed not called if returning from an async function

[mstoykov]

if in

goja/func_test.go

Lines 205 to 208 in 96b1610

	group("2", async () => {
	check("2", "line C");
	await 4;
	check("2", "line D");

you add like await (async () => {})() after await 4 - after it the state isn't being resumed.

This seems like some kind of optimization to me, where goja is skipping a step because it knows it goes back in async function or something like that 🤷

As far as I can see this works whether:

• await is used
• the function actually has a body
• is a lambda or a function

[dop251]

It's not being resumed because it's not being suspended. The first section of an async function (before the first await) is executed synchronously, as per specification, there are no optimisations here.

[mstoykov]

Hmm ... I guess this is because I directly called it and instead I meant to just await it 🤦

This then makes it so that Completed will be almsot impossible to be use correctly in this case:

async function() {
  // set some context
  (async () => {
    // whatever
    // after this line Completed is called with in practice no way of knowing if we are completing the outer async or the inner.
  })() 
 // here Completed will be called again - again we have no idea if we should clean or not clean something. 
}

I would imagine the above will be pretty common if you are using Promises and want to compose a few async functions as in

await Promise.All(
  [someAsyncFunction(), someOtherAsyncFunction()] 
);

I am once again not certain what kind of API will give us the ability to handle those situations.

[dop251]

I can suggest adding a new method Entered() interface{} which will be called upon entering an async function. This then becomes the source of the trackingObject, so the signature for the Suspended() method changes to Suspended(trackingObject). I could also pass the tracking object to the Completed() method, but I think it's not strictly necessary, as it would be known anyway from the previous calls of either Entered() or Resumed(). Let me know what you think.

[mstoykov]

I don't really have any other suggestion at this point so we can at least try that, and I will try to come up with some more cases where this breaks.

[dop251]

Please try with the branch I've just pushed and let me know if it works for you.

[mstoykov]

Hi, sorry about that, but we ended up abandoning the magical way in how we are trying to make group work as it definitely has some problems with how it meshes with the thing we use it for.

We might still use the above functionality and might even implement group after we have tried other less magical approaches.

Sorry about that.

I have tried to make the latest changes work, and might try more after next week (need to finish a different thing by then). But it seems to me like Completed needs a way to know what is "completing" as otherwise there is no reference what needs to happen.

For example

group("some", async () => {
  await async () => {

  } // here we get a complete before the outer one does

  (async () => {
     await 1;
     await 2;
     ...
  })() // but here this function will be entered and then the outer one will complete before the inner one.
  // not being able to diffirentiate them makes it impossible to keep consistent state with this .. I think
})

Getting the trackingObject in Completed is likely enough 🤷.

Corner cases like this are also one of the reason why we think making this work magically has way too many sharp edges. Sharp edges where users will do something, and it will look it will work in one way, but it will work in a totally different. Especially if for some of them it works somewhat magically.

The proposed async context seems like right the magic we want, and I might try to make something like it work for k6 specifically given the provided functionality in goja. I guess I would also need to go through all the prior art and try to see if we can't have one of them do what we want it :(.

Again I will try to get back to this in the coming weeks, but won't be as fast :(.

Thanks for all the work 🙇

[dop251]

I've decided to approach this slightly differently. Could you please check the branch again?

[dop251]

Hi @mstoykov, did you have a chance to look at it? I'm keen to merge it sooner rather than later, before someone actually relied on the current version.

[mstoykov]

Hi @dop251, sorry for the slow reply.

I did try it - but I at this point need to in practice rewrite the whole code in order to fully test this. And I have a slack reminder that just get's moved every day :(.

Looking at the new API it seems like it will now be possible to implement the tc39 proposal you linked earlier. That proposal also happened to be accepted as stage 1 🎉 2 weeks ago.

I wanted to actually try to implement that, but I have been particularly busy as we are getting ready to release a new version of k6 and there are a bunch of small issues and tasks :(.

Having had a bunch of discussions in the last month+ around this it seems likely we (k6 team) will not be working on implementing the proposal in the next 2 months. I personally will be working on #430, which happens to include a lot of work on k6 as well.

I am for you merging this. Especially if the idea is that implementing that proposal is to be possible either in goja itself(once it is stage 4 or at least 3) or that it is possible in a host application like k6 using goja.

I don't know if you want to try to implement it as part of https://github.com/dop251/goja_nodejs for example 🤷

Alternatively dropping the whole interface for now is an option so that nobody depends on something that you might remove.