timers: fix unref() memory leak #1152
Conversation
Fishrock123
added
the
timers
| @@ -0,0 +1,14 @@ | |||
| var assert = require('assert') | |||
There was a problem hiding this comment.
I know the original test didn't have semicolons, but I think that we should add them to conform to style, no?
There was a problem hiding this comment.
oh, haha yeah. I'm used to not really caring about semicolons. Will fix.
|
@trevnorris I think the original test here tested something different, I'll add that one soon too. Any suggestions on the first test? I'm not super confident about it relying on the |
|
@Fishrock123 Not really. Seems you got that test from #1151, which is unfortunate because the test is a race condition. But doing the best we can here. I'd stick with the test you have and add the additional changes from the aforementioned additional patch in Node. |
|
Problem: the original test doesn't pass without function unrefdHandle() {
this.owner._onTimeout();
// if (!this.owner._repeat)
// this.owner.close();
} |
|
Original test, as in the one that uses |
|
Correct, |
|
@Fishrock123 Can you go ahead and update this PR with the additional patch and I'll take a look at the failures. |
|
@trevnorris updated with original commits and the regress-1151 test, which fails under the conditions I mentioned above. |
|
@Fishrock123 I can't get |
|
@trevnorris Without 9ab35aa it fails instantly for me. OS X, if that matters. |
|
@Fishrock123 Sorry, I'm missing the issue. This PR as it stands at the moment passes all tests. Why do we care if a test fails without one of the patches? |
|
Because it also passes with these lines commented out, which seems to be worrisome?: function unrefdHandle() {
this.owner._onTimeout();
// if (!this.owner._repeat)
// this.owner.close();
} |
|
@Fishrock123 That conditional will only fire on |
|
Sure, but that still doesn't answer why this patch fixes the regress-1151 test: diff --git a/lib/timers.js b/lib/timers.js
index e5c0c9e..5af6df3 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -313,6 +313,14 @@ const Timeout = function(after) {
this._repeat = false;
};
+
+function unrefdHandle() {
+ this.owner._onTimeout();
+ // without the close()
+}
+
+
Timeout.prototype.unref = function() {
if (this._handle) {
this._handle.unref();
@@ -323,7 +331,8 @@ Timeout.prototype.unref = function() {
if (delay < 0) delay = 0;
exports.unenroll(this);
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(); |
|
It's because That's still confusing. Let me know if you have any additional questions. |
|
@Fishrock123 LGTM. Merge yourself, or I can merge. Just let me know. |
|
I can merge myself. I'll try looking at it again tomorrow. Mostly I'm just not happy with the state of testing what is going on. |
|
Started investigating if this fixes all problems. I tested on OSX at this point. In Release mode that test runs fine. Fo the other test it is worse (getting To be clear I rebased this on the latest When running the latter test from So clearly that problem is introduced with this change. The Additionally the issue @brycebaril logged is not fixed either. Going to investigate on Linux as well, but at this point we shouldn't merge this IMHO. |
|
Ok, tried this on an Arch Linux box with similar results: With this FixWithout this FixSo basically the |
|
The segfault must be related to a more recent change in v1.x -- @thlorenz can you verify this by basing the commits on 056ed4b? Perhaps we need to bisect a bug somewhere. I'll try rebasing to v1.x and see if I get similar results. |
|
@Fishrock123 I'll do that, but this isn't happening in |
|
So I rebased on 056ed4b as @Fishrock123 suggested and get no more segfaults. OSXRegress test still failing (only in debug mode): LinuxAll seems fixed, I can't repro any issues at this point. So looks like something got introduced in the meantime that doesn't jive with this fix to cause it to segfault :( |
|
currently investigating pre/post 31da975 |
The common case is where setInterval() is called with two arguments, the callback and the timeout. Specifying optional arguments in the parameter list forces common case calls to go through an arguments adaptor stack frame. PR-URL: #1221 Reviewed-By: Trevor Norris <[email protected]>
|
@Fishrock123 Here's a new test that never completes without this patch, but still fails with this patch due to being fired multiple times: var assert = require("assert")
var intervals = 0
var i = setInterval(function () {
intervals++
i.unref()
eatTime()
}, 10)
function eatTime() {
// the goal of this function is to take longer than 10ms, i.e. longer than the interval
var count = 0
while (count++ < 1e7) {
Math.random()
}
}
process.on("exit", function () {
assert.equal(intervals, 1)
})FYI of [io.js, 0.12, 0.10] this only works "correctly" with 0.10 |
|
@brycebaril #1231 should definitely fix these double callbacks. It is a bit of a bandaid fix right now but I think it works good enough to include now and reasearch why exactly it double fires later. |
|
@silverwind from manual patch application #1231 does not seem to be related to this one (and it didn't fix for me). @trevnorris did a git bisect to find the offending patch and it looks like nodejs/node-v0.x-archive@a2eeb43 |
|
Strange it doesn't fix it, will try to investigate this. |
|
This patch likely broken in it's current state as exposed by 33fea6e#commitcomment-10436644. I'll update if we find a way to fix it, but it may be better to just wait on @bnoordhuis's timer re-write. |
Unlike the 'exit' event, this event allows the user to schedule more work and thereby postpone the exit. That also means that the 'beforeExit' event may be emitted many times, see the attached test case for an example. Refs #6305.
|
Got yet another sample (io.js v1.6.2) // To collect heap
require('heapdump');
var o = [];
function setup() {
// to generate some amout of data
var randomData = require('crypto').randomBytes(1024).toString('hex');
var to = setTimeout(function(){
// uncomment next line to prevent leak
// clearTimeout( to );
// to keep refenreces to big data chunk inside timer's scope
o[0] = randomData;
// monitoring
console.log(process.memoryUsage().heapUsed / (1024 * 1024));
// plan next execution
setup();
}, 10);
// without unref() there are no leak
to.unref();
}
// begin
setup();
// just to keep prcess running
setInterval(function(){}, 10000);Here is the screenshot of heapdump data |
|
Re: 33fea6e#commitcomment-10440550
@trevnorris is there a way to check if the timer is the last one attached? Looks like this affecting #1075... |
|
Hooray! |
|
Please add following patch here: commit b926a62c719677d091019164a895a2f93feea682
Author: Fedor Indutny <[email protected]>
Date: Fri Apr 3 01:14:08 2015 +0300
timers: do not touch timer handle after closing it
diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc
index d71213f..8002f75 100644
--- a/src/timer_wrap.cc
+++ b/src/timer_wrap.cc
@@ -73,6 +73,9 @@ class TimerWrap : public HandleWrap {
static void Start(const FunctionCallbackInfo<Value>& args) {
TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
+ if (!HandleWrap::IsAlive(wrap))
+ return args.GetReturnValue().Set(UV_EINVAL);
+
int64_t timeout = args[0]->IntegerValue();
int64_t repeat = args[1]->IntegerValue();
int err = uv_timer_start(&wrap->handle_, OnTimeout, timeout, repeat);
@@ -82,6 +85,9 @@ class TimerWrap : public HandleWrap {
static void Stop(const FunctionCallbackInfo<Value>& args) {
TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
+ if (!HandleWrap::IsAlive(wrap))
+ return args.GetReturnValue().Set(UV_EINVAL);
+
int err = uv_timer_stop(&wrap->handle_);
args.GetReturnValue().Set(err);
}
@@ -89,6 +95,9 @@ class TimerWrap : public HandleWrap {
static void Again(const FunctionCallbackInfo<Value>& args) {
TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
+ if (!HandleWrap::IsAlive(wrap))
+ return args.GetReturnValue().Set(UV_EINVAL);
+
int err = uv_timer_again(&wrap->handle_);
args.GetReturnValue().Set(err);
}
@@ -96,6 +105,9 @@ class TimerWrap : public HandleWrap {
static void SetRepeat(const FunctionCallbackInfo<Value>& args) {
TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
+ if (!HandleWrap::IsAlive(wrap))
+ return args.GetReturnValue().Set(UV_EINVAL);
+
int64_t repeat = args[0]->IntegerValue();
uv_timer_set_repeat(&wrap->handle_, repeat);
args.GetReturnValue().Set(0);
@@ -104,6 +116,9 @@ class TimerWrap : public HandleWrap {
static void GetRepeat(const FunctionCallbackInfo<Value>& args) {
TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
+ if (!HandleWrap::IsAlive(wrap))
+ return args.GetReturnValue().Set(0);
+
int64_t repeat = uv_timer_get_repeat(&wrap->handle_);
if (repeat <= 0xfffffff)
args.GetReturnValue().Set(static_cast<uint32_t>(repeat));It appears to be fixing all crashes. |
The destructor isn't being called for timers that have been unref'd. Fixes: nodejs/node-v0.x-archive#8364
This change fixes a regression introduced by commit 0d05123, which contained a typo that would cause every unrefd interval to fire only once. Fixes: nodejs/node-v0.x-archive#8900 Reviewed-By: Timothy J Fontaine <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-by: Trevor Norris <[email protected]>
|
@indutny am I doing something wrong? that doesn't seem to want to apply. Jeremiahs-MacBook-Pro:io.js Jeremiah$ pbpaste | git am --whitespace=fix
Patch does not have a valid e-mail address.
Jeremiahs-MacBook-Pro:io.js Jeremiah$ git am --abort
Jeremiahs-MacBook-Pro:io.js Jeremiah$ pbpaste | git apply
fatal: corrupt patch at line 60Edit: line 54 should have been 60 |
|
@Fishrock123 I'll push it to new PR. |
|
@indutny's patch fixes the memory corruption as had surfaced in the tests. Moving to his PR. |
|
Fixed by #1330 |
ChALkeR
added
the
memory
Port of nodejs/node-v0.x-archive@0d05123
Fixes: #1151(Not really. It's complicated. #1152 (comment))Not sure what to call the test, or even if this test is particularly good. (or if it even needs a test)
R=@trevnorris