Permalink
Browse files

Tests for memory leaks

  • Loading branch information...
1 parent b7e8e35 commit 91120e04297009ddabb6b7c68ec83a092041903c @isaacs isaacs committed May 3, 2012
View
@@ -40,3 +40,4 @@ ipch/
email.md
blog.html
deps/v8-*
+node_modules
View
@@ -32,7 +32,7 @@ install:
uninstall:
@$(WAF) uninstall
-test: all
+test: all node_modules/weak
@kapouer
kapouer May 4, 2012

Only simple message tests are run : there is no need to depend on 'node_modules/weak' target here.

@isaacs
isaacs May 5, 2012 Collaborator

Ah, good catch. Not sure how that got on there.
Fixed in 8cd2b0e.

$(PYTHON) tools/test.py --mode=release simple message
test-http1: all
@@ -41,7 +41,15 @@ test-http1: all
test-valgrind: all
$(PYTHON) tools/test.py --mode=release --valgrind simple message
-test-all: all
+node_modules/weak:
+ @if [ ! -f node ]; then make all; fi
+ @if [ ! -d node_modules ]; then mkdir -p node_modules; fi
+ ./node deps/npm/bin/npm-cli.js install weak --prefix="$(shell pwd)"
+
+test-gc: all node_modules/weak
+ $(PYTHON) tools/test.py --mode=release gc
+
+test-all: all node_modules/weak
$(PYTHON) tools/test.py --mode=debug,release
make test-npm
@@ -153,6 +161,7 @@ clean:
$(WAF) clean
-find tools -name "*.pyc" | xargs rm -f
-rm -rf blog.html email.md
+ -rm -rf node_modules
distclean: docclean
-find tools -name "*.pyc" | xargs rm -f
@@ -0,0 +1,61 @@
+// just like test/gc/http-client.js,
+// but aborting every connection that comes in.
+
+function serverHandler(req, res) {
+ res.connection.destroy();
+}
+
+var http = require('http'),
+ weak = require('weak'),
+ done = 0,
+ count = 0,
+ countGC = 0,
+ todo = 18,
+ common = require('../common.js'),
+ assert = require('assert'),
+ PORT = common.PORT;
+
+console.log('We should do '+ todo +' requests');
+
+var http = require('http');
+var server = http.createServer(serverHandler);
+server.listen(PORT, getall);
+
+function getall() {
+ for (var i = 0; i < todo; i++) {
+ (function(){
+ function cb(res) {
+ done+=1;
+ statusLater();
+ }
+
+ var req = http.get({
+ hostname: 'localhost',
+ pathname: '/',
+ port: PORT
+ }, cb).on('error', cb);
+
+ count++;
+ weak(req, afterGC);
@Filirom1
Filirom1 May 4, 2012

It's quite normal that req is not gargabe collected because ClientRequest are stored in agent.requests.

I would prefer to test a variable define in a closure, for exemple :

var big = new Big()
count++;
weak(big, afterGC);

function cb(rs){
  done++
  statusLater();
  return big
}
var req = http.get({ ... }, cb)
req.on('error', function(){ return big })

and the same for timeout

req.setTimeout(10, function(){
  console.log('timeout (expected)')
  return big;
});

In my program, the ClientRequest instance do not retain much memory, it's the closure the problem.

@isaacs
isaacs May 4, 2012 Collaborator

No, it is not normal to see req objects leak in this case. The agent.requests collection is merely requests that are pending or in progress. The role of the agent is to keep a pool of sockets, which are re-used between different requests. The requests themselves are thrown away when they're completed.

The closure in your example is only leaked because the request was leaked. The req object was leaked because it was attached to a http_parser and never removed. The req refers to the closure, which traps a reference to the big object. Breaking the chain at any point will allow GC to happen, but it must be broken at the right place, not just smashed to bits.

For example, you could also have prevented the leak of the big object by setting the big reference to null. But that's not the root cause. The root cause is that the parser was not getting cleaned up properly. So the test focuses on the root cause, not the noisy symptom.

@Filirom1
Filirom1 May 4, 2012

Thank you for your enlighting me.

+ })()
+ }
+}
+
+function afterGC(){
+ countGC ++;
+}
+
+function statusLater() {
+ setTimeout(status, 1);
+}
+
+function status() {
+ gc();
+ console.log('Done: %d/%d', done, todo);
+ console.log('Collected: %d/%d', countGC, count);
+ if (done === todo) {
+ console.log('All should be collected now.');
+ assert(count === countGC);
+ process.exit(0);
+ }
+}
@@ -0,0 +1,66 @@
+// just like test/gc/http-client.js,
+// but with an on('error') handler that does nothing.
+
+function serverHandler(req, res) {
+ res.writeHead(200, {'Content-Type': 'text/plain'});
+ res.end('Hello World\n');
+}
+
+var http = require('http'),
+ weak = require('weak'),
+ done = 0,
+ count = 0,
+ countGC = 0,
+ todo = 18,
+ common = require('../common.js'),
+ assert = require('assert'),
+ PORT = common.PORT;
+
+console.log('We should do '+ todo +' requests');
+
+var http = require('http');
+var server = http.createServer(serverHandler);
+server.listen(PORT, getall);
+
+function getall() {
+ for (var i = 0; i < todo; i++) {
+ (function(){
+ function cb(res) {
+ done+=1;
+ statusLater();
+ }
+ function onerror(er) {
+ throw er;
+ }
+
+ var req = http.get({
+ hostname: 'localhost',
+ pathname: '/',
+ port: PORT
+ }, cb).on('error', onerror);
+
+ count++;
+ weak(req, afterGC);
+ })()
+ }
+}
+
+function afterGC(){
+ countGC ++;
+}
+
+function statusLater() {
+ setTimeout(status, 1);
+}
+
+function status() {
+ gc();
+ console.log('Done: %d/%d', done, todo);
+ console.log('Collected: %d/%d', countGC, count);
+ if (done === todo) {
+ console.log('All should be collected now.');
+ assert(count === countGC);
+ process.exit(0);
+ }
+}
+
@@ -0,0 +1,69 @@
+// just like test/gc/http-client.js,
+// but with a timeout set
+
+function serverHandler(req, res) {
+ setTimeout(function () {
+ res.writeHead(200)
+ res.end('hello\n');
+ }, 100);
+}
+
+var http = require('http'),
+ weak = require('weak'),
+ done = 0,
+ count = 0,
+ countGC = 0,
+ todo = 18,
+ common = require('../common.js'),
+ assert = require('assert'),
+ PORT = common.PORT;
+
+console.log('We should do '+ todo +' requests');
+
+var http = require('http');
+var server = http.createServer(serverHandler);
+server.listen(PORT, getall);
+
+function getall() {
+ for (var i = 0; i < todo; i++) {
+ (function(){
+ function cb() {
+ done+=1;
+ statusLater();
+ }
+
+ var req = http.get({
+ hostname: 'localhost',
+ pathname: '/',
+ port: PORT
+ }, cb);
+ req.on('error', cb);
+ req.setTimeout(10, function(){
+ console.log('timeout (expected)')
+ });
+
+ count++;
+ weak(req, afterGC);
+ })()
+ }
+}
+
+function afterGC(){
+ countGC ++;
+}
+
+function statusLater() {
+ setTimeout(status, 1);
+}
+
+function status() {
+ gc();
+ console.log('Done: %d/%d', done, todo);
+ console.log('Collected: %d/%d', countGC, count);
+ if (done === todo) {
+ console.log('All should be collected now.');
+ assert(count === countGC);
+ process.exit(0);
+ }
+}
+
@@ -0,0 +1,63 @@
+// just a simple http server and client.
+
+function serverHandler(req, res) {
+ res.writeHead(200, {'Content-Type': 'text/plain'});
+ res.end('Hello World\n');
+}
+
+var http = require('http'),
+ weak = require('weak'),
+ done = 0,
+ count = 0,
+ countGC = 0,
+ todo = 5,
+ common = require('../common.js'),
+ assert = require('assert'),
+ PORT = common.PORT;
+
+console.log('We should do '+ todo +' requests');
+
+var http = require('http');
+var server = http.createServer(serverHandler);
+server.listen(PORT, getall);
+
+
+function getall() {
+ for (var i = 0; i < todo; i++) {
+ (function(){
+ function cb(res) {
+ console.error('in cb')
+ done+=1;
+ res.on('end', statusLater);
+ }
+
+ var req = http.get({
+ hostname: 'localhost',
+ pathname: '/',
+ port: PORT
+ }, cb)
+
+ count++;
+ weak(req, afterGC);
+ })()
+ }
+}
+
+function afterGC(){
+ countGC ++;
+}
+
+function statusLater() {
+ setTimeout(status, 1);
+}
+
+function status() {
+ gc();
+ console.log('Done: %d/%d', done, todo);
+ console.log('Collected: %d/%d', countGC, count);
+ if (done === todo) {
+ console.log('All should be collected now.');
+ assert(count === countGC);
+ process.exit(0);
+ }
+}
@@ -0,0 +1,61 @@
+// just like test/gc/http-client-timeout.js,
+// but using a net server/client instead
+
+function serverHandler(sock) {
+ sock.setTimeout(120000);
+ setTimeout(function () {
+ sock.end('hello\n');
+ }, 100);
+}
+
+var net = require('net'),
+ weak = require('weak'),
+ done = 0,
+ count = 0,
+ countGC = 0,
+ todo = 18,
+ common = require('../common.js'),
+ assert = require('assert'),
+ PORT = common.PORT;
+
+console.log('We should do '+ todo +' requests');
+
+var server = net.createServer(serverHandler);
+server.listen(PORT, getall);
+
+function getall() {
+ for (var i = 0; i < todo; i++) {
+ (function(){
+ var req = net.connect(PORT, '127.0.0.1');
+ req.setTimeout(10, function() {
+ console.log('timeout (expected)')
+ req.destroy();
+ done++;
+ statusLater();
+ });
+
+ count++;
+ weak(req, afterGC);
+ })()
+ }
+}
+
+function afterGC(){
+ countGC ++;
+}
+
+function statusLater() {
+ setTimeout(status, 1);
+}
+
+function status() {
+ gc();
+ console.log('Done: %d/%d', done, todo);
+ console.log('Collected: %d/%d', countGC, count);
+ if (done === todo) {
+ console.log('All should be collected now.');
+ assert(count === countGC);
+ process.exit(0);
+ }
+}
+
Oops, something went wrong.

0 comments on commit 91120e0

Please sign in to comment.