Skip to content
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

Verify TickEventLoop code is correct #27

Closed
cmfcmf opened this issue Jan 16, 2018 · 2 comments
Closed

Verify TickEventLoop code is correct #27

cmfcmf opened this issue Jan 16, 2018 · 2 comments
Assignees

Comments

@cmfcmf
Copy link
Collaborator

cmfcmf commented Jan 16, 2018

Carefully review what each function does in our TickEventLoop implementation and if we correctly set the more variable:

node/src/node.cc

Lines 4802 to 4818 in 2fae0db

inline static bool TickEventLoop(Environment & env) {
bool more;
uv_run(env.event_loop(), UV_RUN_ONCE);
v8_platform.DrainVMTasks();
more = uv_loop_alive(env.event_loop());
if (more)
return more;
EmitBeforeExit(&env);
// Emit `beforeExit` if the loop became alive either after emitting
// event, or after running some callbacks.
more = uv_loop_alive(env.event_loop());
return more;
}

@justus-hildebrand justus-hildebrand self-assigned this Jan 24, 2018
@justus-hildebrand
Copy link
Collaborator

justus-hildebrand commented Jan 24, 2018

The only potential issue I see here at the moment is more being a bool, but uv_loop_alive() returning an int. uv_loop_alive returns non-zero if there are referenced active handles, active requests or closing handles in the loop. Since this code stems from the original Node.js implementation, I think this should be fine, though.

DrainVMTasks() has something to do with V8 internals, but since we run uv_run with UV_RUN_ONCE instead of UV_RUN_DEFAULT, it should get called way more often than it used to. I am not quite sure of the implications of this just yet, but I am looking into it right now. Seems to be a pretty deep rabbit hole...

@msoechting msoechting added this to the Feature Complete milestone Jan 29, 2018
@justus-hildebrand
Copy link
Collaborator

According to this commit, PR with discussion here, DrainVMTasks() is called to handle outstanding V8 threads/tasks that might still exist while node is already shutting down. Apparently, without draining the tasks, UV could think there are no more events and shut down node, eventhough there may still be events that would get queued by V8 later.

For us, I propose a 3rd call to uv_loop_alive() (and returning if true) before calling DrainVMTasks(), to check if there are more UV events pending before draining the VM, which in theory may take rather long under the right circumstances.

When making TickEventLoop configurable (Issue #36) this call may also be protected with a if (mode != UV_RUN_DEFAULT), as it is obsolete in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants