Timer.unref() memory leak

[patriksimek]

Consider this code:

// script.js

var formatBytes = function(bytes) {
    var sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB'];
    var i = parseInt(Math.floor(Math.log(bytes) / Math.log(1024)))
    return (bytes / Math.pow(1024, i)).toFixed(1) + sizes[i]
};

setInterval(function() {
    var mem = process.memoryUsage();
    console.log('rss:', formatBytes(mem.rss), 'heapTotal:', formatBytes(mem.heapTotal), 'heapUsed:', formatBytes(mem.heapUsed));
}, 1000);

var method = function() {
    var tmr = setTimeout(function() { method() }, 0);
    if (process.argv[2] == '--unref') tmr.unref();
};

method();

To reproduce the issue, start the script with node script.js --unref. Heap snapshot taken after few minutes of running the script:

There are thousands of Timers leaking memory:

And this is a heap snapshot taken from the version without unref (node script.js):

This is a hourly report from our app that uses unref frequently:

OSX 10.9.4 & Debian 7.6
Node 0.10.31

[trevnorris]

Confirmed. Simplest example:

var iter = 3e6;

(function runner() {
  if (--iter < 0)
    return;
  setTimeout(function() { }, 0).unref();
  setImmediate(runner);
}());

Output:

$ /usr/bin/time ./node /tmp/gh-8364.js                     
12.11user 0.80system 0:12.89elapsed 100%CPU (0avgtext+0avgdata 1582756maxresident)k

Remove the .unref() and memory usage only grows to around ~30MB.

Ran my test on v0.12. So still exists.

[tjfontaine]

So running @trevnorris's test case and grabbing a core file from it, I was able to see that we had stable counts of the JS instantiated Timer classes, and a large growth of the C++ created TimerWrap objects

# setImmediate
81e8ff21    31439        2 Object: _idleNext, _idlePrev
# setTimeout
81e8fff5    31436        3 Timer: _idleNext, _idlePrev, msecs
# TimerWrap
81e080a5   218138        2 Timer: ontimeout, domain

looking briefly through, we don't seem to have the case covered where the C++ class's destructor is called that we're cleaning up the rest of the v8 memory, so the persistent is never being disposed.

Here's a quick fix, that I think should solve your problem:

diff --git a/lib/timers.js b/lib/timers.js
index be39ea6..694d5ce 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -303,7 +303,15 @@ Timeout.prototype.unref = function() {
     if (delay < 0) delay = 0;
     exports.unenroll(this);
     this._handle = new Timer();
-    this._handle.ontimeout = this._onTimeout;
+
+    var self = this;
+
+    this._handle.ontimeout = function closeIfTimeout() {
+      self._onTimeout();
+      if (!self.repeat)
+        self.close();
+    };
+
     this._handle.start(delay, 0);
     this._handle.domain = this.domain;
     this._handle.unref();

please apply that to the v0.10 branch and let me know if it solves your problem

[trevnorris]

In order to prevent any unnecessary function wrapping, here's a similar patch for v0.12:

diff --git a/lib/timers.js b/lib/timers.js
index 3039b49..d8554ca 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -315,6 +315,14 @@ var Timeout = function(after) {
   this._repeat = false;
 };

+
+function unrefdHandle() {
+  this.owner._onTimeout();
+  if (!this.owner.repeat)
+    this.owner.close();
+}
+
+
 Timeout.prototype.unref = function() {
   if (!this._handle) {
     var now = Timer.now();
@@ -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();

[patriksimek]

I've just tested that, the patch fixes it. Thanks.

[indutny]

cc @trevnorris let's land it.

[aheckmann]

was this ever landed?

[cjihrig]

@aheckmann it doesn't appear so.

[trevnorris]

Fixed in v0.10 by 0d05123.