From 737d9b6d033d83cfc7235d465c22e7ecad198765 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Thu, 10 Sep 2015 13:15:43 +0100 Subject: [PATCH 1/6] node,timers: fix beforeExit with unrefed timers --- src/handle_wrap.h | 1 + src/node.cc | 17 +++++++++++++++++ .../test-timers-unrefed-in-beforeexit.js | 15 +++++++++++++++ 3 files changed, 33 insertions(+) create mode 100644 test/parallel/test-timers-unrefed-in-beforeexit.js diff --git a/src/handle_wrap.h b/src/handle_wrap.h index da712b33befbcc..086bc2f08f549b 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -53,6 +53,7 @@ class HandleWrap : public AsyncWrap { private: friend class Environment; friend void GetActiveHandles(const v8::FunctionCallbackInfo&); + friend bool HasUnrefedTimerHandles(Environment* env); static void OnClose(uv_handle_t* handle); ListNode handle_wrap_queue_; unsigned int flags_; diff --git a/src/node.cc b/src/node.cc index a7800f8b4c54fd..4c2c1dadd08c47 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1543,6 +1543,18 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ary); } +bool HasUnrefedTimerHandles(Environment* env) { + + HandleScope handle_scope(env->isolate()); + + for (auto w : *env->handle_wrap_queue()) { + if (!w->persistent().IsEmpty() && w->flags_ & HandleWrap::kUnref) + return true; + } + + return false; +} + static void Abort(const FunctionCallbackInfo& args) { ABORT(); @@ -3969,6 +3981,11 @@ static void StartNodeInstance(void* arg) { v8::platform::PumpMessageLoop(default_platform, isolate); EmitBeforeExit(env); + // We need to run an extra event loop turn to purge unrefed handles. + bool unrefedTimers = HasUnrefedTimerHandles(env); + if (unrefedTimers == true) + uv_run(env->event_loop(), UV_RUN_NOWAIT); + // Emit `beforeExit` if the loop became alive either after emitting // event, or after running some callbacks. more = uv_loop_alive(env->event_loop()); diff --git a/test/parallel/test-timers-unrefed-in-beforeexit.js b/test/parallel/test-timers-unrefed-in-beforeexit.js new file mode 100644 index 00000000000000..163189e44be279 --- /dev/null +++ b/test/parallel/test-timers-unrefed-in-beforeexit.js @@ -0,0 +1,15 @@ +require('../common'); +const assert = require('assert'); + +var once = 0; + +process.on('beforeExit', () => { + setTimeout(() => {}, 1).unref(); + once++; +}); + +process.on('exit', (code) => { + if (code !== 0) return; + + assert.strictEqual(once, 1); +}); From 01df246fa7c3c799fb5b5c0265b1b651032ca53a Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Wed, 7 Oct 2015 17:57:28 -0700 Subject: [PATCH 2/6] fixup! test: use strict --- test/parallel/test-timers-unrefed-in-beforeexit.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/parallel/test-timers-unrefed-in-beforeexit.js b/test/parallel/test-timers-unrefed-in-beforeexit.js index 163189e44be279..019ee8f784dd07 100644 --- a/test/parallel/test-timers-unrefed-in-beforeexit.js +++ b/test/parallel/test-timers-unrefed-in-beforeexit.js @@ -1,3 +1,5 @@ +'use strict'; + require('../common'); const assert = require('assert'); From 85854e033824c1996a1020d40ea8b1944114bf69 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Thu, 8 Oct 2015 20:27:20 -0700 Subject: [PATCH 3/6] !fixup nits, fix test --- src/node.cc | 5 +---- test/parallel/test-timers-unrefed-in-beforeexit.js | 3 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node.cc b/src/node.cc index 4c2c1dadd08c47..2c86e18874c867 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1543,15 +1543,12 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ary); } -bool HasUnrefedTimerHandles(Environment* env) { - - HandleScope handle_scope(env->isolate()); +bool HasUnrefedTimerHandles(Environment* env) { for (auto w : *env->handle_wrap_queue()) { if (!w->persistent().IsEmpty() && w->flags_ & HandleWrap::kUnref) return true; } - return false; } diff --git a/test/parallel/test-timers-unrefed-in-beforeexit.js b/test/parallel/test-timers-unrefed-in-beforeexit.js index 019ee8f784dd07..0ac3311816fb4d 100644 --- a/test/parallel/test-timers-unrefed-in-beforeexit.js +++ b/test/parallel/test-timers-unrefed-in-beforeexit.js @@ -6,6 +6,9 @@ const assert = require('assert'); var once = 0; process.on('beforeExit', () => { + if (once > 1) + throw new RangeError('beforeExit should only have been called once!'); + setTimeout(() => {}, 1).unref(); once++; }); From baaaabf04dcb810c1ef4488e4d8289c32bf9e4e5 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 9 Oct 2015 11:57:06 -0700 Subject: [PATCH 4/6] !fixup make it even better --- src/node.cc | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/node.cc b/src/node.cc index 2c86e18874c867..d3e7d1983ceade 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1544,15 +1544,6 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { } -bool HasUnrefedTimerHandles(Environment* env) { - for (auto w : *env->handle_wrap_queue()) { - if (!w->persistent().IsEmpty() && w->flags_ & HandleWrap::kUnref) - return true; - } - return false; -} - - static void Abort(const FunctionCallbackInfo& args) { ABORT(); } @@ -3970,23 +3961,26 @@ static void StartNodeInstance(void* arg) { { SealHandleScope seal(isolate); bool more; + uv_loop_t* event_loop = env->event_loop(); + do { v8::platform::PumpMessageLoop(default_platform, isolate); - more = uv_run(env->event_loop(), UV_RUN_ONCE); + more = uv_run(event_loop, UV_RUN_ONCE); if (more == false) { v8::platform::PumpMessageLoop(default_platform, isolate); EmitBeforeExit(env); - // We need to run an extra event loop turn to purge unrefed handles. - bool unrefedTimers = HasUnrefedTimerHandles(env); - if (unrefedTimers == true) - uv_run(env->event_loop(), UV_RUN_NOWAIT); + // If there are only unrefed handles left, we need to run an + // extra event loop turn to purge the unrefed handles. + if (event_loop->active_handles == 0 && + event_loop->handle_queue[0] != event_loop->handle_queue[1]) + uv_run(event_loop, UV_RUN_NOWAIT); // Emit `beforeExit` if the loop became alive either after emitting // event, or after running some callbacks. - more = uv_loop_alive(env->event_loop()); - if (uv_run(env->event_loop(), UV_RUN_NOWAIT) != 0) + more = uv_loop_alive(event_loop); + if (uv_run(event_loop, UV_RUN_NOWAIT) != 0) more = true; } } while (more == true); From c3587239fb5d50d2054f3c1b2b3e69a37a067d05 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 9 Oct 2015 14:12:42 -0700 Subject: [PATCH 5/6] !fixup don't need this anymore --- src/handle_wrap.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/handle_wrap.h b/src/handle_wrap.h index 086bc2f08f549b..da712b33befbcc 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -53,7 +53,6 @@ class HandleWrap : public AsyncWrap { private: friend class Environment; friend void GetActiveHandles(const v8::FunctionCallbackInfo&); - friend bool HasUnrefedTimerHandles(Environment* env); static void OnClose(uv_handle_t* handle); ListNode handle_wrap_queue_; unsigned int flags_; From 5d7262c464abc988b11f0e55bcef6f4787e21b03 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 9 Oct 2015 16:03:48 -0700 Subject: [PATCH 6/6] !fixup test: fix test-beforeexit-event Trying to use a net server here is not reasonable, instead try with a file. --- test/parallel/test-beforeexit-event.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-beforeexit-event.js b/test/parallel/test-beforeexit-event.js index f3bd127b408b08..3502a1119c5501 100644 --- a/test/parallel/test-beforeexit-event.js +++ b/test/parallel/test-beforeexit-event.js @@ -1,6 +1,6 @@ 'use strict'; var assert = require('assert'); -var net = require('net'); +var fs = require('fs'); var util = require('util'); var common = require('../common'); var revivals = 0; @@ -23,18 +23,18 @@ function tryTimer() { setTimeout(function() { console.log('timeout cb, do another once beforeExit'); revivals++; - process.once('beforeExit', tryListen); + process.once('beforeExit', tryFile); }, 1); } -function tryListen() { - console.log('create a server'); - net.createServer() - .listen(common.PORT) - .on('listening', function() { - revivals++; - this.close(); - }); +function tryFile() { + console.log('open a file'); + fs.open(__filename, 'r', (err, fd) => { + if (err) throw err; + + revivals++; + fs.closeSync(fd); + }); } process.on('exit', function() {