Uh oh!
There was an error while loading. Please reload this page.
timers: fix unref() memory leak - #1152
Conversation
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.
Fishrock123
commented
Mar 16, 2015
@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 |
trevnorris
commented
Mar 17, 2015
@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. |
Fishrock123
commented
Mar 17, 2015
Problem: the original test doesn't pass without functionunrefdHandle(){this.owner._onTimeout();// if (!this.owner._repeat)// this.owner.close();} |
trevnorris
commented
Mar 17, 2015
Original test, as in the one that uses |
Fishrock123
commented
Mar 17, 2015
Correct, |
trevnorris
commented
Mar 17, 2015
@Fishrock123 Can you go ahead and update this PR with the additional patch and I'll take a look at the failures. |
Fishrock123
commented
Mar 17, 2015
@trevnorris updated with original commits and the regress-1151 test, which fails under the conditions I mentioned above. |
trevnorris
commented
Mar 17, 2015
@Fishrock123 I can't get |
Fishrock123
commented
Mar 17, 2015
@trevnorris Without 9ab35aa it fails instantly for me. OS X, if that matters. |
trevnorris
commented
Mar 17, 2015
@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? |
Fishrock123
commented
Mar 17, 2015
Because it also passes with these lines commented out, which seems to be worrisome?: functionunrefdHandle(){this.owner._onTimeout();// if (!this.owner._repeat)// this.owner.close();} |
trevnorris
commented
Mar 18, 2015
@Fishrock123 That conditional will only fire on |
Fishrock123
commented
Mar 18, 2015
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(); |
trevnorris
commented
Mar 18, 2015
It's because That's still confusing. Let me know if you have any additional questions. |
trevnorris
commented
Mar 24, 2015
@Fishrock123 LGTM. Merge yourself, or I can merge. Just let me know. |
Fishrock123
commented
Mar 25, 2015
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. |
thlorenz
commented
Mar 25, 2015
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. |
thlorenz
commented
Mar 25, 2015
Ok, tried this on an Arch Linux box with similar results: With this FixWithout this FixSo basically the |
Fishrock123
commented
Mar 25, 2015
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. |
thlorenz
commented
Mar 25, 2015
@Fishrock123 I'll do that, but this isn't happening in |
thlorenz
commented
Mar 25, 2015
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 :( |
Fishrock123
commented
Mar 25, 2015
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 <trev.norris@gmail.com>
brycebaril
commented
Mar 25, 2015
@Fishrock123 Here's a new test that never completes without this patch, but still fails with this patch due to being fired multiple times: varassert=require("assert")varintervals=0vari=setInterval(function(){intervals++i.unref()eatTime()},10)functioneatTime(){// the goal of this function is to take longer than 10ms, i.e. longer than the intervalvarcount=0while(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 |
silverwind
commented
Mar 25, 2015
@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. |
brycebaril
commented
Mar 25, 2015
@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 |
silverwind
commented
Mar 25, 2015
Strange it doesn't fix it, will try to investigate this. |
Fishrock123
commented
Mar 27, 2015
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.
Olegas
commented
Apr 2, 2015
Got yet another sample (io.js v1.6.2) // To collect heaprequire('heapdump');varo=[];functionsetup(){// to generate some amout of datavarrandomData=require('crypto').randomBytes(1024).toString('hex');varto=setTimeout(function(){// uncomment next line to prevent leak// clearTimeout( to );// to keep refenreces to big data chunk inside timer's scopeo[0]=randomData;// monitoringconsole.log(process.memoryUsage().heapUsed/(1024*1024));// plan next executionsetup();},10);// without unref() there are no leakto.unref();}// beginsetup();// just to keep prcess runningsetInterval(function(){},10000);Here is the screenshot of heapdump data |
Fishrock123
commented
Apr 2, 2015
Re: 33fea6e#commitcomment-10440550
@trevnorris is there a way to check if the timer is the last one attached? Looks like this affecting #1075... |
indutny
commented
Apr 2, 2015
Hooray! |
indutny
commented
Apr 2, 2015
Please add following patch here: commit b926a62c719677d091019164a895a2f93feea682
Author: Fedor Indutny <fedor@indutny.com>
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 <tjfontaine@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Fishrock123
commented
Apr 2, 2015
@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 |
indutny
commented
Apr 2, 2015
@Fishrock123 I'll push it to new PR. |
Fishrock123
commented
Apr 2, 2015
@indutny's patch fixes the memory corruption as had surfaced in the tests. Moving to his PR. |
Fishrock123
commented
Apr 4, 2015
Fixed by #1330 |
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