node: invoke `beforeExit` again if loop was active #7209

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Owner

indutny commented Feb 28, 2014

When setImmediate(cb) is called in beforeExit event handler the
consequent uv_run(..., UV_RUN_NOWAIT) may return 0, even if there
was some active handles at start.

Fixes simple/test-beforeexit-event.js.

cc @saghul @bnoordhuis

@indutny indutny node: invoke `beforeExit` again if loop was active
When `setImmediate(cb)` is called in `beforeExit` event handler the
consequent `uv_run(..., UV_RUN_NOWAIT)` may return `0`, even if there
was some active handles at start.

Fixes simple/test-beforeexit-event.js.
799e58b

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Fedor Indutny

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Mar 1, 2014

@@ -3536,7 +3536,8 @@ int Start(int argc, char** argv) {
more = uv_run(env->event_loop(), UV_RUN_ONCE);
if (more == false) {
EmitBeforeExit(env);
- more = uv_run(env->event_loop(), UV_RUN_NOWAIT);
+ more = uv_loop_alive(env->event_loop());
+ uv_run(env->event_loop(), UV_RUN_NOWAIT);
@bnoordhuis

bnoordhuis Mar 1, 2014

Owner

I'm not 100% sure that's correct. I think it needs to take the return value of uv_run() into account in case it starts handles, i.e. that it should look like this:

diff --git a/src/node.cc b/src/node.cc
index 0a9a0cd..555d734 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -3536,7 +3536,8 @@ int Start(int argc, char** argv) {
       more = uv_run(env->event_loop(), UV_RUN_ONCE);
       if (more == false) {
         EmitBeforeExit(env);
-        more = uv_run(env->event_loop(), UV_RUN_NOWAIT);
+        more = uv_loop_alive(env->event_loop());
+        more = uv_run(env->event_loop(), UV_RUN_NOWAIT) || more;
       }
     } while (more == true);
     code = EmitExit(env);

(No idea why GitHub is mangling the diff like that.)

@indutny

indutny Mar 1, 2014

Owner

Indeed, this is correct.

@indutny

indutny Mar 1, 2014

Owner

Though, actually not. It is impossible for loop to become active, if it was inactive before entering uv_run().

@indutny

indutny Mar 1, 2014

Owner

Anyway, pushed fix.

@bnoordhuis

bnoordhuis Mar 1, 2014

Owner

It is impossible for loop to become active, if it was inactive before entering uv_run().

Oh, I guess you're right. At least this way makes it a little more future-proof. LGTM.

Owner

indutny commented Mar 1, 2014

Thanks, landed in 5596f93

indutny closed this Mar 1, 2014

indutny deleted the indutny:fix/beforeexit branch Mar 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment