timers: refactor how setTimeout and co are made available globally #4563
Conversation
This makes the setTimeout and co. timers package into a not js module package. The current one is tc55, the newly made common API to make technical committee alongside tc39. This might be changed later. This also allows us to make them globally available without importing js module "k6/timers" allowing us to fix #3814.
|
|
||
| func (e *timers) setupTaskTimeout() { | ||
| e.queue.stopTimer() | ||
| delay := -time.Since(e.timers[e.queue.first().id]) |
There was a problem hiding this comment.
Wouldn't this panic if first() returns nil?
There was a problem hiding this comment.
this is always ran after either a thing has been added or after it was checked that it isn't empty, so there is no way for this to happen.
Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com>
d290903 to
a4e0299
Compare
|
How is it coupled ? Do you want me to not have |
Yes, that's what I meant. Like I said, if you have a plan, it's good, for now. I don't want changes right now. Frankly, I mostly care about wrapping the work on the #4278 which I'm going to align, once this will be merged |
There was a problem hiding this comment.
LGTM as-is 👍🏻 🚀
Absolutely non-blocking comment from me—just sharing my perspective (feel free to proceed as-is). The only real downside I see here is that the way the timers module integrates feels somewhat implicit. Specifically, it relies on certain functions being injected into the global object as a side effect of calling a function from the tc55/timers module in the bundle code.
It took me about five minutes to connect the dots in the context of this PR, but I’m not confident it will be as easy six months from now. If I were to revisit this code in isolation—outside the context of this PR—I’d likely be a bit confused about the expectation that the timers functions should be set in the global object. Tracing back where and how this setup happens would take some effort.
I don’t have a concrete alternative in mind, just an intuition: since we’ll likely be adding more to the global object over time, maybe it would be worth having a single, explicit place in the code dedicated to managing its state? Something like a centralized abstraction or entry point where all GlobalObject().Set calls related to module function injection live, making it clear where to look.
Once again, I'm happy to merge as-is, this is really me dumping my brain 🧠 🙇🏻
|
modules.VU isn't really about modules to be honest, but removing that means providing all the things we use from it to the timers implementation, which currently means context, logger, runtime and a way to register the callback. Context gets reset every iteration, so for that we either need a function or we need to make new timers every iteration. Arguably I can just make a different interface that only has the things timers cares about put it in timers. I do not currently think that this iwll benefit us much unless we also move event loop to not be k6 dependant and start making a much bigger separation between this components and k6 and make them only depend on Sobek. IIRC this has been shelved for later. tl;dr modules.VU is just a way to get stuff from the VU, it has very little to do with the modules. It is what the modules need so it is defined as n interface there. |
|
I wrote the above comment before I saw the approvals or your comment @oleiade .
I did originally set them globally from the bundle and modulestest and had to have
I will try to get through bundle and maybe reorder stuff and see if I can make most of them in |
What?
This makes the setTimeout and co. timers package into a not js module package. The current one is tc55, the newly made common API to make technical committee alongside tc39. This might be changed later.
This also allows us to make them globally available without importing js module "k6/timers" allowing us to fix #3814.
Why?
Reporting k6/timers as always used is not good.
Also, setTimeout and co shouldn't; be dependent on being in a module.
Likely more changes will happen in the future and tc55 will either grow or disappear as a package.
breaking change is set only because previously in modulestest
k6/timerswas automatically made available - I doubt anyone used that, but it is a thing.Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)