Add basic event loop with some API to be used by modules

[mstoykov]

A recent update to goja introduced support for ECMAScript Promise.

The catch here is that Promise's then will only be called when goja
exits executing js code and it has already been resolved.
Also resolving and rejecting Promises needs to happen while no other
js code is being executed as it will otherwise lead to a data race.

This more or less necessitates adding an event loop. Additionally
because a call to a k6 modules such as k6/http might make a promise to
signal when an http request is made, but if (no changes were made) the
iteration then finishes before the request completes, nothing would've
stopped the start of a new iteration. That new iteration would then
probably just again ask k6/http to make a new request with a Promise ...

This might be a desirable behaviour for some cases but arguably will be
very confusing so this commit also adds a way to RegisterCallback that
will return a function to actually queue the callback on the event loop,
but prevent the event loop from ending before the callback is queued and
possible executed, once RegisterCallback is called.

Additionally to that, some additional code was needed so there is an
event loop for all special functions calls (setup, teardown,
handleSummary, default) and the init context.

This also adds handling of rejected promise which don't have a reject
handler similar to what deno does.

It also adds a per iteration context that gets canceled on the end of
each iteration letting other code know that it needs to stop. This is
particularly needed here as if an iteration gets aborted by a syntax
error (or unhandled promise rejection), a new iteration will start right
after that. But this means that any in-flight asynchronous operation (an
http requests for example) will not get stopped. With a context that
gets canceled every time module code can notice that and abort any
operation. For this same reason the event loop needs wait to be
empty before the iteration ends.

This did lead to some ... not very nice code, but a whole package needs
a big refactor which will likely happen once common.Bind and co gets
removed.

fixes #882

You can look at 61aa0e5 for how it can be used to (badly) add a callback based API to k6/http

[yorugac]

Not sure if it's a WIP yet, but just for my understanding: is the plan here to have 2 types of Promises, MakeHandledPromise() vs ~ MakeGlobalPromise() which lives across iterations? Or only the handled promise, promise-within-iteration, will remain?

[mstoykov]

It is in WIP in so far as I need some people to now look at it and give their opinion on how much we want to change and whether this direction is the direction we want to go in :).

I don't have any particular opinion on MakeGlobalPromise apart from ... let's not do it now :). I can't currently remember anyone actually requiring something like that and the idea that the requests that you start in one iteration end in that one iteration seems like a good enough idea that we probably should keep it.

But that is exactly what the PR is supposed to get started discussing - whether we want to start with having just Promises that need to finish in the current iteration or do we want to design something completely different from the start.

[yorugac]

I see, thanks for clarifications. I actually think Promises spanning some (known VS unknown?) number of iterations might be an indicator of bad testing practice: their presence will make it harder to reason about the test run. And as you said in the below comment, it is easier to add than to remove an interface method 🙂

With that said, "promise bound to iteration" as a basis - 👍 And I think it can be simplified down to a ~ NewPromise() method ? (esp. with the ongoing rename of InstanceCore) I.e. if there will be other types of promises, a new and more precise name can be just figured out then.

Also, just for understanding: why wouldn't it work with reject only? From this:

// Caveats: this likely won't work if the Promise is rejected from within the js code

I assume that resolve is fine, but not reject.

[mstoykov]

Also, just for understanding: why wouldn't it work with reject only? From this:

Sorry for that I was left with the impression that you can reject or resolve a Promise, returned from the outside, inside the js code but it turns out Promise.reject returns a completely new Promise that is just rejected. (There is one for resolved ones as well).

This actually makes a lot of sense because otherwise, it will be really hard to signal and reason. So that part of the comment is ... invalid, sorry

[mstoykov]

I kind of wonder if both this and the above should not be removed and just to have something like the eventloop's Reserve method so that AddToEventLoop(func() {dosomething();} is Reserve()(func(){dosomething();} and MakeHandledPromise can be implemented in the common package the same way it's implemented currently in the js one (again using just eventloop#Reserve).

We can always add more API, but removing one is a bit harder and there are more uses than just returning a Promise for telling the eventloop to not stop as you will want to add something to it. While having all 3 seems a ... bit much especially if all 3 can be implemented using only 1 of them

[yorugac]

I wonder also, are there any known use cases that will need this method AddToEventLoop? Because with the condition "something will not let the iteration end", it seems like something rather hard to use correctly 😕 I.e. it might be simpler to fall back to a Promise object that already has those guarantees.

[mstoykov]

I think I might've gotten overboard with this warning. If you are in a call into your extension/module and haven't returned and now are running in a different goroutine, then you are currently blocking so there is no problem :).

But if you for example return (for example a Promise, but not necessary) and in another goroutine you then try to use AddToEventLoop you have every chances that the iteration has already ended.

Reserve (bad name again) basically makes certain that it will block the event loop from exiting, which means that there are no problems .... unless the scenario/execution has ended in which case you have other problems.

[yorugac]

I've looked at the current logic a bit more closely: could you please check / clarify on the following scenario:

export default function () {
    setTimeout(() => {
        console.log(new Date(), "setTimeout")
    }, 20)
    console.log(new Date(), "sleep")
    sleep(10);

    setTimeout(() => {
        console.log(new Date(), "setTimeout 2")
    }, 20000)
}

? It seems that using sleep messes up with the loop: first setTimeout is executed only after sleep period. It can probably be not only sleep but any operation taking some noticeable time.
As far as I understood, it happens because runFn itself is placed on the loop (probably) but even more importantly, should that be an expected behavior? The way it works now, it doesn't seem obvious to me as an API.

[yorugac]

I wonder also, are there any known use cases that will need this method AddToEventLoop? Because with the condition "something will not let the iteration end", it seems like something rather hard to use correctly 😕 I.e. it might be simpler to fall back to a Promise object that already has those guarantees.

[yorugac]

I see, thanks for clarifications. I actually think Promises spanning some (known VS unknown?) number of iterations might be an indicator of bad testing practice: their presence will make it harder to reason about the test run. And as you said in the below comment, it is easier to add than to remove an interface method 🙂

With that said, "promise bound to iteration" as a basis - 👍 And I think it can be simplified down to a ~ NewPromise() method ? (esp. with the ongoing rename of InstanceCore) I.e. if there will be other types of promises, a new and more precise name can be just figured out then.

Also, just for understanding: why wouldn't it work with reject only? From this:

// Caveats: this likely won't work if the Promise is rejected from within the js code

I assume that resolve is fine, but not reject.

[mstoykov]

It seems that using sleep messes up with the loop

actually sleep doesn't mess with the event loop at all. sleep just like any other piece of (js) code is blocking in this case it does nothing and we can theoretically argue that some work can be done in this time, but the way Jobs and Promises(which queue the then/finally are defined in ECMAScript is such that it's required that there is nothing else on the stack - so nothing is in the process of being executed. Here we have the sleep being executed which blocks the event loop as it's usually called, here is an (IMO) cool example.

<tangent>
Coming from go you probably somewhat expect that because sleep is blocking the "scheduler" will go do something else, but again ECMAScript is 100% single-threaded(or at least he execution of the js code should act as it's 100% single-threaded).

Earlier I had an experiment where js module code could signal that it no longer needs the goja.Runtime for now, so if something else needs it, it can use it. Here I used it to fix xk6-amqp panics (because it just access the goja.Runtime whenever. As you can see the code is ... convoluted and I had to fix like 20 race conditions in my new code.

You constantly (not only in the extension code) need to work around the fact that you might or might not have the Runtime and if you forget to lock it something else will potentially use it. I am pretty sure it still has way more bugs than this PR has and it even doesn't add the event loop or support for Promises.

Here is (again) experimental change to xk6-amqp to make it use Promises that has way less problems and I only had one race and ti was in the event loop code not in the extension code.

But even if writing this code wasn't much harder IMO (or I change my opinion) this is still not how it's done in JS or with an event loop in general.
</tangent>

So in conclusion this is exactly the expected behaviour, but I would be happy that some js writing people give their opinion as well :)

[mstoykov]

I think I might've gotten overboard with this warning. If you are in a call into your extension/module and haven't returned and now are running in a different goroutine, then you are currently blocking so there is no problem :).

But if you for example return (for example a Promise, but not necessary) and in another goroutine you then try to use AddToEventLoop you have every chances that the iteration has already ended.

Reserve (bad name again) basically makes certain that it will block the event loop from exiting, which means that there are no problems .... unless the scenario/execution has ended in which case you have other problems.

[mstoykov]

Also, just for understanding: why wouldn't it work with reject only? From this:

Sorry for that I was left with the impression that you can reject or resolve a Promise, returned from the outside, inside the js code but it turns out Promise.reject returns a completely new Promise that is just rejected. (There is one for resolved ones as well).

This actually makes a lot of sense because otherwise, it will be really hard to signal and reason. So that part of the comment is ... invalid, sorry

[yorugac]

@mstoykov thank you for taking the time to clarify and adding examples

I hope nobody would start relying on this pseudo-async stuff in the scripts too much as it'd be a headache to debug! 😄

Expectation of ~ "trying to stay as close to JS behavior as possible" is clear + your example with AMQP appears to also be the use case for AddToEventLoop usage that I was asking about 👍 I'll re-read this closer again and probably will make a couple of more suggestions / clarifications.

[mstoykov]

A clear problem with the current code is that

import exec from "k6/execution";
export const options = {
        scenarios: {
                "first":{
                        executor: "shared-iterations",
                        maxDuration: "2s",
                        iterations: 1,
                        vus: 1,
                        gracefulStop:"1s",
                },
                "second": {
                        executor: "shared-iterations",
                        maxDuration: "10s",
                        iterations: 1,
                        vus: 1,
                        startTime: "5s",
                }
        }
}
export default function() {
        setTimeout(()=> {console.log(exec.scenario.name)}, 5000)
}

Will result in both setTimeout being executed (and printing "second" which is more expected than not ;))

[yorugac]

I've run some experiments with the event loop yesterday and it seems to work as intended 👍 but the overall logic becomes more complicated to reason about (inevitable here I suppose), so I added a few suggestions in an attempt to improve clarity.

Also, WDYT about adding the "async testing scenarios / edge cases" somewhere, as a test suite or even as a sub-folder in samples/? It can be only started now but expanded later on, as needs be. My thinking is that in case of potential regression in future changes, it might be useful to have that test suite to run quick checks.

[yorugac]

This will silently discard f if Reserve was called before Start, right? If this is the intended behaviour, this definitely deserves a very clear warning at least in comments. So the correct workflow for eventLoop objects appears to be like this right now:

1. call RunOnLoop at least once
2. call Start exactly once
3. call either RunOnLoop or Reserve as many times as needed in separate goroutine(s) (since Start is blocking)

Could this info be added somewhere in this file, as a comment?
(I wondered too if Reserve could return an error if it's called at the wrong time here but that'd be adding complexity to the interface so probably best to avoid.)

Given this complex rules to how to use eventLoop, I don't think it's a good idea to re-export / expose any of these methods of eventLoop to the module level interface as they are 😅

[mstoykov]

This will silently discard f if Reserve was called before Start, right?

Yes it will, which likely can be fixed by increasing started at the end instead of at the beginning 🤷

But to be honest I don't think this is a problem.
eventLoop will always be only internally used in the runner/bundle/vu and as such it has very little actual places where we directly interact with it.

Could this info be added somewhere in this file, as a comment?

https://github.com/grafana/k6/pull/2228/files#diff-da0f214f9c82c517bd4fe1f550a32b3c37b378419325e2cce650f5fcbedf73f0R45
But the TODO below is now ... less viable.

Given this complex rules to how to use eventLoop, I don't think it's a good idea to re-export / expose any of these methods of eventLoop to the module level interface as they are sweat_smile

This has never been the idea if anything as I mentioned I want to only export Reserve. The actual module developers can only want to add things to the queue and re-exporting Reserve for them seems to give both the most versatility and we can document that you need to call it before you make a goroutine and return from a function that will return to goja.

func (m *moduleInstance) SomethingThatIsCalledOnTheLoopForExampleJustACallMadeFromJS(...) someObject {
  addToLoop := m.vu.ReserveOnLoop() // will need better name
  go func() {
     // do something
     addToLoop(someThingToBeCalledOnTheLoop) // this is a function and it can use the runtime to call some more js code
  }()
  return someObject{} // this can be a Promise where the above `someThingToBeCalledOnTheLoop` either resolves or rejects it.
}

Here the Loop is definitely started and if it gets stopped in between ReserveOnLoop and addToLoop being called we definitely don't want this to be added to the next queue (even if the eventLoop has been restarted, for example for a new iteration ;) )

A possible redoing of the current API is to have:

1. remove RunOnLoop entirely
2. have Start to take a function that should be queued before executing
3. no changes to Reserve
   Which I am now going to try out

[mstoykov]

While doing some related changes I somewhat feel that

1. we can recreate the eventLoop on each iteration which might help with the other problems (and complexity)
2. It might actually be better if we don't stop on a context or at least not on the same one that is used for everything else. The "main" context also triggers an interrupt on the goja machine so it means the loop should clear either way and as long as that can't be overrun in js I do think modules should probably have a way to queue something on the queue when the scenario is finished, if that is what they want/need.

This, unfortunately, goes more in the direction of iteration/scenario/vu lifecycle hooks for extensions part and I think that problem will likely need a lot more work, so I will try out some stuff and see whether I can remove more stuff from the current code and simplify it even more

[yorugac]

1. remove RunOnLoop entirely
2. have Start to take a function that should be queued before executing
3. no changes to Reserve

👍 yes, that might be a good alternative

[yorugac]

On a related note, right now started acts more like a bool, not an int: I don't think the same eventLoop should be Start-ed more than once at a time, right? If so, then it makes sense to have if e.started { return } in the beginning of Start and defer func() {e.started = false}() somewhere before the for loop
Maybe e.started -> e.processing? 🤔

[mstoykov]

The idea is that we don't want to get something that was reserved to be queued in a previous iteration to be executed in a future Start of the eventLoop as in #2228 (comment)

[yorugac]

Oh, I get it, so Start is allowed to run more than once concurrently right now... Yes, now I also think that creating eventLoop per iteration would be simpler 😄 Definitely easier to work with, at least.

[yorugac]

My proposal on name changes:

• RunOnLoop(f) -> Add(f)
• Reserve remains as is
• Start -> Process

Rationale: eventLoop is basically a queue and Start is a blocking call that goes through that queue and it is allowed to call it more than once sequentially. If naming would reflect that more closely, we would have modules.VU interface free to use the names more fitting to the main purpose (like 'promise', 'loop', etc.) without creating duplicate-looking methods and with less confusion between different objects. IOW, eventLoop can be seen as a low-level queue while modules.VU is a higher level interface to work with that queue.

[mstoykov]

Thanks for playing with it, I will try to make some more changes now that I have also played with it some more that hopefully will simplify it and also try to document it some more.

About tests -I did on eventloop unit tests but I guess I should also add some more e2e tests in the core/engine probably 🤔

[mstoykov]

Turned out I had some local changes that I both thought were actually pushed, and that I've actually completely fixed ... but the latest commit should be stable.

After trying to make an eventLoop per iteration I quickly realized that to implement this I will need to refactor big parts of the code as currently the same loop that is used in the init context where setTimeout is defined is used everywhere, just through providing it everywhere. Also the modules.VU implementation also gets it in the init context. Updating all of this also seems kind of ... not really relevant as you can just keep old reference to those.

Given the above and that the eventLoop exiting on a context.Context creates a race between code, that previously could cleanup on context being done, but now the eventLoop will just return before it (possibly) had any chance to do so. A similar race already happens if the js code returns before we interrupt the goja VM and we do have checks for that.

The new design expects that whatever calls Reserve will correctly (whatever that means) call back into the eventloop on context being done to unblock it. This also allows some amount of cleaning up in those situations.

This isn't really a new complexity as even now code such as http.get does have handling that it should stop the whole execution on context being Done.

[mstoykov]

The current test failures is because we do now return error whenever it happens, without waiting on the reserved callback to be called.

Now I guess the question is should that even be the behaviour?

• Should we return on error?
• maybe remove the returning of error at all 🤷 ?
• maybe keep it, but wait until the loop is empty again and return a list of errors?

This more and more seems like it needs to be discussed with more of "vu lifecycle" approach where js modules can actually be notified that an iteration has ended, had errors and have the option of doing something before that happens, in this way anything using Reserved will be able to be notified.

I kind of don't want to go down that rabbit hole as well and was really hoping that for a first version we can have a very small API as it's currently a single Reserve function that can be used at least somewhat correctly. To this end I kind of wonder if I should return the started checks I removed in 9140e32 which seem to keep this in check 🤔 and fix that we are not notifying any code about the fact the loop has ended at a later point.

Possibly even now through adding 1 more context that is cancelled at the end of each iteration.

WDYT?

p.s. I will spend some time to try to reduce the amount of hanged goroutines that are seen in the current test failures

[oleiade]

As far as I can tell: excellent work overall. I don't understand all the details yet, but the implementation is simple and clear enough that I could make sense of it.

My understanding of the scope is that we want to essentially support setTimeout in the context of test scripts. Is that correct?

Also, after reviewing the code and reading the other comments I get a feel that there are quite a bit of limitations, things to be careful about, and places where we're not certain if it could break? It left me wondering how risky that would be to expose the feature to users if that's the case?

Cheers 🐻

[oleiade]

My (limited) understanding of the JS event loop is that the loop acts like a task queue? The rest of the comment seems to hint at that too. Thus, what would you think of using a "queue-related" terminology such as enqueue/deque?
Otherwise maybe a terminology around "booking" feels like it could work? Like booking a spot in a process or something like that? Somehow "reserve" is linked to memory in my brain, not to execution (if that makes sense).

[mstoykov]

I don't have a particular thing against enqueue in general, although I would use add in those cases to be honest. But reserve as the names suggests, and I've tried to explain, doesn't add anything to the event loop when called. It says that we will add something in the future and until then the event loop should not block. And it just so happens that the easiest way I could come up to do that is for it to return a function that when called will enqueue on the event loop.

it's old

[na--]

@mstoykov 👍 for mentioning this is experimental and might change in the docs / release notes.

And big 👍 for making it a helper, I didn't consider it, but it should be easy to do and probably better! 🎉

[na--]

FWIW, I am against adding the current setTimeout implementation to the k6 globals in v0.37.0 by default, even if we don't announce it anywhere... I am fine with sticking it in the k6 module silently, or something like that, so it has to be manually import-ed, but not in the globals unless and until it's compliant with the standard (and it's currently definitely not).

I imagine there are JS polyfills and libraries that might check if a function with the setTimeout name exists and behave differently based on it, so 👎 for having a single k6 version with the current broken version. We should either have nothing, or work on properly implementing it and the rest of the other similar functions, e.g. by testing and hardening my PoC from #2228 (comment)

[codebien]

Now that we have it as an explicit experiment could we add the expected timeoutID as returned value and set it always to zero?

[na--]

I plan to make a PR with my PoC from #2228 (comment) tomorrow

[mstoykov]

I would prefer if we leave this for the next release. Given that this won't be used by anyone.

We can have this changes in :

1. extension
2. in that module temporarily through the development cycle

but I would argue we would probably want them globally available by next release.

Adding unnecessary changes days before cutoff IMO just adds more ways for this to break. We currently only care about this call in order to test the event loop - anyone actually wanting to use it - shouldn't

[na--]

Why does isDefault matter here?

[mstoykov]

Only when isDefault is true is cancel not nil, but I guess I should probably just check that cancel isn't nil :)

Also on related note looking at this now again, we only cancel the context on error ... which likely isn't so great.

Shall I :

1. do nothing now? it works for the cases we care about and this code likely will change eitehr way
2. chance to check cancel!=nil instead - practically 0% change in what will happen hopefully 🤞
3. Do actually fix this and make the whole check just check if there is cancel - this makes it so we always will cancel the context. Arguably what the code should've done but it might be a bit too close to the release to change this 🤔 . But still not that big of a change IMO

[mstoykov]

I went to try to do 3 and to see what problems exist and found:

1. actually in all cases there is a cancel that can be provided so we can truly wait on setup,teardown and handleSummary as well. Whether that is that much of an improvement to do is a different thing
2. (which I remember thinking about but apparently never checked ?!?) a side effect of canceling the context is that the code below that checks if this was a full iterations or not will always say that it wasn't a full iteration. Moving that code before here fixes that. So with the current code if there is an exception we will mark an iteration as not full, which is a change as well.

[na--]

I am fine with both 2. and 3. from #2228 (comment), but probably prefer 3 as you also seem to.

(which I remember thinking about but apparently never checked ?!?) a side effect of canceling the context is that the code below that checks if this was a full iterations or not will always say that it wasn't a full iteration. Moving that code before here fixes that. So with the current code if there is an exception we will mark an iteration as not full, which is a change as well.

hmm let's not change this for now, I don't know all of the implications, but it matters for some things (e.g. the cloud output)... so 👍 for moving the code upwards and not changing the logic for full iteration, for now

[mstoykov]

Final analysis (correcting some mistakes from above):
With the current code in all cases the context gets canceled when an iteartion end or setup gets executed. This happens as I've added it for iterations and it was always the case for the other code.
The current code specifically for iterations (of the "default" function) will on error:

1. cancel the context early
2. wait for the event loop to be empty
   this so we don't just keep making more and more promises in the event loop and then an exception abort the iteration, but k6 doesn't actually wait for that concurrent work wind down. But instead just starts a new iteration doing the same.
   This means that this also changed that an iteartion that had an exception is now marked as not full.

Possible changes to the code:

1. check cancel != nil instead of isDefault
2. Move the isFullIteration logic above this code so that isFullIteration is true even on an excepiton - mostly to be compatible with the old version.
3. Make this behaviour also true for not default function

1 and 2 are pretty small and safe changes IMO.
3. is a bit more ... risky

[oleiade]

I'm probably not understanding all the subtleties at play here, but my hunch is 1./2.

[na--]

Agreed, 1. and 2. for now seem like the best bet for v0.37.0. We can probably safely leave 3. for the follow-up event loop changes in v0.38.0 and/or the js.Runner refactoring... Or maybe never? 🤷‍♂️ Generally, setup() and teardown() abort the whole test, and we don't reuse the transient VUs they run on, so I don't think it will matter if we don't wait for any reservations to clear 🤷‍♂️

[na--]

LGTM, though I didn't fully grok something that I left as an inline comment

[na--]

LGTM 🤞 😅