-
Notifications
You must be signed in to change notification settings - Fork 7
micro tasks: run micro tasks on isolate every 1 millisecond #291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/loop.zig
Outdated
// If the loop's context id has changed, don't call the js callback | ||
// function. The callback's memory has already be cleaned and the | ||
// events nb reset. | ||
if (ctx.ctx_id != ctx.loop.ctx_id) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check necessary? I don't think there's an harm in continuously executing this, even if the "ctx_id" has been increased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. My PR doesn't work if the browser removes the page and reset the loop.
My idea was: the loop has a longer lifetime than the isolate. That's why I wanted to use the ctx_id
to avoid callbacks after any env deinit.
Currently we have:
- main creates the loop
- a new conn arrives
- we create and init the CDP which init the browser
- eventually a browser context is created which create a brower's session which init a js env => the runMicrotasks loop starts
- eventually the session's
removepage
is called which reset the loop's context id => microtasks loop stops, we don't want that. - envetually the browser context is discarded along with the session and the env => the microtasks loop should stop
- the session, the env, the browser and the cdp conn are deinit => the microtasks loop should stop
=> I don't want a future timeout is called after the env deinit
But the current ctx_id
doesn't match to what I want for running micro tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I introduced 2 differents ctx_id
in the loop: one for the js callbacks and another for the zig one.
This is not perfect b/c we could have other zig callback independent lifetimes... But it unblocks me for now.
I haven't found a better way to handle that for now.
d75c70b
to
b037202
Compare
This PR exposes
runMicrotasks
andpumpMessageLoop
.It also run
runMicrotasks
every millisecond using timeouts.I'm not sure if it's correct to implement this loop using timeout to run the
runMicrotasks
.pro: it is handled by zig js runtime transparently
con: the caller doesn't control the delay between the runs
Fix #56