From 340be322d207735f069103987e6928881248f526 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 26 Nov 2014 12:27:57 -0800 Subject: [PATCH 1/5] timers: fix unref() memory leak The destructor isn't being called for timers that have been unref'd. Fixes: https://github.com/joyent/node/issues/8364 --- lib/timers.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/timers.js b/lib/timers.js index 668d5536c8186c..302a11e18b5624 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -301,6 +301,14 @@ const Timeout = function(after) { this._repeat = null; }; + +function unrefdHandle() { + this.owner._onTimeout(); + if (!this.owner.repeat) + this.owner.close(); +} + + Timeout.prototype.unref = function() { if (this._handle) { this._handle.unref(); @@ -315,7 +323,8 @@ Timeout.prototype.unref = function() { if (this._called && !this._repeat) return; this._handle = new Timer(); - this._handle[kOnTimeout] = this._onTimeout; + this._handle.owner = this; + this._handle[kOnTimeout] = unrefdHandle; this._handle.start(delay, 0); this._handle.domain = this.domain; this._handle.unref(); From 31d880112f1300fa4631d0b4d45a612a3e7d911c Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Thu, 18 Dec 2014 16:51:08 -0800 Subject: [PATCH 2/5] timers: don't close interval timers when unrefd This change fixes a regression introduced by commit 0d051238be2e07e671d7d9f4f444e0cc1efadf1b, which contained a typo that would cause every unrefd interval to fire only once. Fixes: https://github.com/joyent/node/issues/8900 Reviewed-By: Timothy J Fontaine Reviewed-By: Colin Ihrig Reviewed-by: Trevor Norris --- lib/timers.js | 2 +- .../test-timers-unrefd-interval-still-fires.js | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-timers-unrefd-interval-still-fires.js diff --git a/lib/timers.js b/lib/timers.js index 302a11e18b5624..442789439bc7c2 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -304,7 +304,7 @@ const Timeout = function(after) { function unrefdHandle() { this.owner._onTimeout(); - if (!this.owner.repeat) + if (!this.owner._repeat) this.owner.close(); } diff --git a/test/parallel/test-timers-unrefd-interval-still-fires.js b/test/parallel/test-timers-unrefd-interval-still-fires.js new file mode 100644 index 00000000000000..3ea94454cfdb49 --- /dev/null +++ b/test/parallel/test-timers-unrefd-interval-still-fires.js @@ -0,0 +1,18 @@ +/* + * This test is a regression test for joyent/node#8900. + */ +var assert = require('assert'); + +var N = 5; +var nbIntervalFired = 0; +var timer = setInterval(function() { + ++nbIntervalFired; + if (nbIntervalFired === N) + clearInterval(timer); +}, 1); + +timer.unref(); + +setTimeout(function onTimeout() { + assert.strictEqual(nbIntervalFired, N); +}, 100); From 0b39408915d0d171b2cef3269a46aa53d26b9dfb Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Fri, 3 Apr 2015 01:14:08 +0300 Subject: [PATCH 3/5] timers: do not restart the interval after close Partially revert 776b73b24306bac0ce299df4f90b7645d5efca31. Following code crashes after backported timer leak fixes: ```javascript var timer = setInterval(function() { clearInterval(timer); }, 10); timer.unref(); ``` Note that this is actually tested in a `test-timers-unref.js`, and is crashing only with 776b73b24306bac0ce299df4f90b7645d5efca31. Calling `clearInterval` leads to the crashes in case of `.unref()`ed timers, and might lead to a extra timer spin in case of regular intervals that was closed during the interval callback. All of these happens because `.unref()`ed timer has it's own `_handle` and was used after the `.close()`. --- lib/timers.js | 5 +++++ src/timer_wrap.cc | 10 ++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/timers.js b/lib/timers.js index 442789439bc7c2..dea10136370a09 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -272,6 +272,11 @@ exports.setInterval = function(callback, repeat) { function wrapper() { timer._repeat.call(this); + + // Timer might be closed - no point in restarting it + if (!timer._repeat) + return; + // If timer is unref'd (or was - it's permanently removed from the list.) if (this._handle) { this._handle.start(repeat, 0); diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index d71213f2202984..f65290a5162398 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -73,6 +73,8 @@ class TimerWrap : public HandleWrap { static void Start(const FunctionCallbackInfo& args) { TimerWrap* wrap = Unwrap(args.Holder()); + CHECK(HandleWrap::IsAlive(wrap)); + int64_t timeout = args[0]->IntegerValue(); int64_t repeat = args[1]->IntegerValue(); int err = uv_timer_start(&wrap->handle_, OnTimeout, timeout, repeat); @@ -82,6 +84,8 @@ class TimerWrap : public HandleWrap { static void Stop(const FunctionCallbackInfo& args) { TimerWrap* wrap = Unwrap(args.Holder()); + CHECK(HandleWrap::IsAlive(wrap)); + int err = uv_timer_stop(&wrap->handle_); args.GetReturnValue().Set(err); } @@ -89,6 +93,8 @@ class TimerWrap : public HandleWrap { static void Again(const FunctionCallbackInfo& args) { TimerWrap* wrap = Unwrap(args.Holder()); + CHECK(HandleWrap::IsAlive(wrap)); + int err = uv_timer_again(&wrap->handle_); args.GetReturnValue().Set(err); } @@ -96,6 +102,8 @@ class TimerWrap : public HandleWrap { static void SetRepeat(const FunctionCallbackInfo& args) { TimerWrap* wrap = Unwrap(args.Holder()); + CHECK(HandleWrap::IsAlive(wrap)); + int64_t repeat = args[0]->IntegerValue(); uv_timer_set_repeat(&wrap->handle_, repeat); args.GetReturnValue().Set(0); @@ -104,6 +112,8 @@ class TimerWrap : public HandleWrap { static void GetRepeat(const FunctionCallbackInfo& args) { TimerWrap* wrap = Unwrap(args.Holder()); + CHECK(HandleWrap::IsAlive(wrap)); + int64_t repeat = uv_timer_get_repeat(&wrap->handle_); if (repeat <= 0xfffffff) args.GetReturnValue().Set(static_cast(repeat)); From ca615ac4f3d382df6fc10c6dec73523dda324eec Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 4 Apr 2015 01:08:20 +0300 Subject: [PATCH 4/5] test: add test for a unref'ed timer leak --- test/parallel/test-timers-unref-leak.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 test/parallel/test-timers-unref-leak.js diff --git a/test/parallel/test-timers-unref-leak.js b/test/parallel/test-timers-unref-leak.js new file mode 100644 index 00000000000000..c8f958a47c40d2 --- /dev/null +++ b/test/parallel/test-timers-unref-leak.js @@ -0,0 +1,25 @@ +var assert = require('assert'); + +var called = 0; +var closed = 0; + +var timeout = setTimeout(function() { + called++; +}, 10); +timeout.unref(); + +// Wrap `close` method to check if the handle was closed +var close = timeout._handle.close; +timeout._handle.close = function() { + closed++; + return close.apply(this, arguments); +}; + +// Just to keep process alive and let previous timer's handle die +setTimeout(function() { +}, 50); + +process.on('exit', function() { + assert.equal(called, 1); + assert.equal(closed, 1); +}); From 452571b8cfc870db6d4f0eb410c093815e997bd1 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Fri, 3 Apr 2015 02:28:45 +0300 Subject: [PATCH 5/5] timers: remove redundant code --- lib/timers.js | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/timers.js b/lib/timers.js index dea10136370a09..27c4b0717653ca 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -157,15 +157,8 @@ exports.enroll = function(item, msecs) { // it will reset its timeout. exports.active = function(item) { var msecs = item._idleTimeout; - if (msecs >= 0) { - var list = lists[msecs]; - if (!list || L.isEmpty(list)) { - insert(item, msecs); - } else { - item._idleStart = Timer.now(); - L.append(list, item); - } - } + if (msecs >= 0) + insert(item, msecs); };