Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak caused by misfiring callback #287

Closed
wants to merge 1 commit into from
Closed

Fix memory leak caused by misfiring callback #287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions lib/spdy/handle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,22 @@ Handle.prototype._getPeerName = function _getPeerName() {

Handle.prototype._closeCallback = function _closeCallback(callback) {
var state = this._spdyState;
var stream = state.stream;

if (state.ending)
state.stream.end(callback);
else
state.stream.abort(callback);
if (state.ending) {
// The .end() method of the stream may be called by us or by the
// .shutdown() method in our super-class. If the latter has already been
// called, then calling the .end() method below will have no effect, with
// the result that the callback will never get executed, leading to an ever
// so subtle memory leak.
if (stream._spdyState.isServer && stream._writableState.ending) {
process.nextTick(callback);
} else {
stream.end(callback);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, good catch!

}
} else {
stream.abort(callback);
}

// Only a single end is allowed
state.ending = false;
Expand Down
1 change: 0 additions & 1 deletion test/fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,3 @@ exports.everyConfig = function everyConfig(body) {
});
});
}

32 changes: 18 additions & 14 deletions test/server-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,12 @@ describe('SPDY Server', function() {
assert.equal(req.method, 'POST');
assert.equal(req.url, '/post');

res.writeHead(200, {
ok: 'yes'
});
res.end('response');
setTimeout(function() {
Copy link
Contributor Author

@anandsuresh anandsuresh Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my proudest moment, but it was either this or introducing a nested process.nextTick() in handle.js, which was uglier. The issue is that by the time the test gets around to reading the incoming data from the client, the response has ended, and the call to stream.end() ends up terminating the whole stream before the end event fires on the request object.

Also, it was only the HTTP/2 tests that were failing. The SPDY tests were working fine.

res.writeHead(200, {
ok: 'yes'
});
res.end('response');
}, 10);

fixtures.expectData(req, 'request', next);
});
Expand Down Expand Up @@ -207,18 +209,20 @@ describe('SPDY Server', function() {
assert.equal(req.method, 'POST');
assert.equal(req.url, '/page');

res.writeHead(200, {
ok: 'yes'
});
setTimeout(function() {
res.writeHead(200, {
ok: 'yes'
});

var push = res.push('/push', {
request: {
yes: 'push'
}
});
push.end('push');
var push = res.push('/push', {
request: {
yes: 'push'
}
});
push.end('push');

res.end('response');
res.end('response');
}, 10);

fixtures.expectData(req, 'request', next);
});
Expand Down