Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

net: memory leak in net.connect #4308

Closed
bnoordhuis opened this Issue Nov 24, 2012 · 3 comments

Comments

Projects
None yet
1 participant
Owner

bnoordhuis commented Nov 24, 2012

The connect listener is added with .on(), not .once(), stopping it from getting garbage collected.

Here is a test case:

var common = { PORT: 5678 };
var assert = require('assert');
var net = require('net');

assert(typeof gc === 'function', 'Run this test with --expose-gc');
net.createServer(function() {}).listen(common.PORT);

(function() {
  // 2**26 == 64M entries
  for (var i = 0, junk = [123.456]; i < 26; ++i) junk = junk.concat(junk);

  if (process.argv[2] === 'once') {
    var conn = net.createConnection(common.PORT, '127.0.0.1');
    conn.once('connect', onConnect);
  }
  else {
    net.createConnection(common.PORT, '127.0.0.1', onConnect);  // uses on()
  }

  function onConnect() {
    assert(junk.length != 0);  // keep reference alive
    setTimeout(cleanup, 10);
    gc();
  }
})();

function cleanup() {
  var before = process.memoryUsage().rss;
  gc();
  var after = process.memoryUsage().rss;
  var reclaimed = (before - after) / 1024;
  console.log('%d kB reclaimed', reclaimed);
  assert(reclaimed > 384 * 1024);  // it's more like 512M on x64
  process.exit();
}

Using net.createConnection(cb):

$ out/Release/node --expose-gc tmp/test-net-connect-gc.js
-48 kB reclaimed

timers.js:102
            if (!process.listeners('uncaughtException').length) throw e;
                                                                      ^
AssertionError: false == true
    at cleanup [as _onTimeout] (/home/bnoordhuis/src/nodejs/master/tmp/test-net-connect-gc.js:33:3)
    at Timer.list.ontimeout (timers.js:100:19)

Using connection.once('connect', cb):

$ out/Release/node --expose-gc tmp/test-net-connect-gc.js once
524260 kB reclaimed

It's easy to fix but an audit of lib/ for variations of this bug might be in order.

Owner

bnoordhuis commented Nov 24, 2012

Addressed in 4cb17cb.

@bnoordhuis bnoordhuis was assigned Nov 25, 2012

Owner

bnoordhuis commented Nov 25, 2012

Reopening, this probably needs to be addressed separately in the tls module.

@bnoordhuis bnoordhuis reopened this Nov 25, 2012

Owner

bnoordhuis commented Nov 26, 2012

Fixed for tls.connect() in 121ed91.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment