Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
Tests for memory leaks
Browse files Browse the repository at this point in the history
  • Loading branch information
@isaacs
isaacs committed May 3, 2012
1 parent b7e8e35 commit 91120e0
Show file tree
Hide file tree
Showing 9 changed files with 466 additions and 3 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Expand Up @@ -40,3 +40,4 @@ ipch/
email.md
blog.html
deps/v8-*
node_modules
13 changes: 11 additions & 2 deletions Makefile
View file
Expand Up @@ -32,7 +32,7 @@ install:
uninstall:
@$(WAF) uninstall

test: all
test: all node_modules/weak

This comment has been minimized.

Sign in to view

Copy link
@kapouer

kapouer May 4, 2012

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

This comment has been minimized.

Sign in to view

Copy link
@isaacs

isaacs May 5, 2012

Author

Ah, good catch. Not sure how that got on there.
Fixed in 8cd2b0e.
$(PYTHON) tools/test.py --mode=release simple message

test-http1: all
Expand 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

Expand Down Expand Up @@ -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
Expand Down
61 changes: 61 additions & 0 deletions test/gc/test-http-client-connaborted.js
View file
@@ -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);

This comment has been minimized.

Sign in to view

Copy link
@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.

This comment has been minimized.

Sign in to view

Copy link
@isaacs

isaacs May 4, 2012

Author

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.

This comment has been minimized.

Sign in to view

Copy link
@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);
}
}
66 changes: 66 additions & 0 deletions test/gc/test-http-client-onerror.js
View file
@@ -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);
}
}

69 changes: 69 additions & 0 deletions test/gc/test-http-client-timeout.js
View file
@@ -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);
}
}

63 changes: 63 additions & 0 deletions test/gc/test-http-client.js
View file
@@ -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);
}
}
61 changes: 61 additions & 0 deletions test/gc/test-net-timeout.js
View file
@@ -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);
}
}

0 comments on commit 91120e0

Please sign in to comment.