Skip to content
This repository has been archived by the owner. It is now read-only.
Permalink
Browse files

http: fix ServerResponse does not emit 'close'

Refs #2453.
  • Loading branch information
koichik committed Jan 5, 2012
1 parent 78dbb4b commit dd9593ccc4dd1de21faa1996c2577c7a6ac3d8ae
Showing with 37 additions and 20 deletions.
  1. +7 −0 doc/api/http.markdown
  2. +20 −16 lib/http.js
  3. +10 −4 test/simple/test-http-response-close.js
@@ -247,6 +247,13 @@ authentication details.
This object is created internally by a HTTP server--not by the user. It is
passed as the second parameter to the `'request'` event. It is a `Writable Stream`.

### Event: 'close'

`function () { }`

Indicates that the underlaying connection was terminated before
`response.end()` was called or able to flush.

### response.writeContinue()

Sends a HTTP/1.1 100 Continue message to the client, indicating that
@@ -343,22 +343,6 @@ util.inherits(OutgoingMessage, stream.Stream);
exports.OutgoingMessage = OutgoingMessage;


OutgoingMessage.prototype.assignSocket = function(socket) {
assert(!socket._httpMessage);
socket._httpMessage = this;
this.socket = socket;
this.connection = socket;
this._flush();
};


OutgoingMessage.prototype.detachSocket = function(socket) {
assert(socket._httpMessage == this);
socket._httpMessage = null;
this.socket = this.connection = null;
};


OutgoingMessage.prototype.destroy = function(error) {
this.socket.destroy(error);
};
@@ -792,6 +776,26 @@ exports.ServerResponse = ServerResponse;

ServerResponse.prototype.statusCode = 200;

function onServerResponseClose() {
this._httpMessage.emit('close');
}

ServerResponse.prototype.assignSocket = function(socket) {
assert(!socket._httpMessage);
socket._httpMessage = this;
socket.on('close', onServerResponseClose);
this.socket = socket;
this.connection = socket;
this._flush();
};

ServerResponse.prototype.detachSocket = function(socket) {
assert(socket._httpMessage == this);
socket.removeListener('close', onServerResponseClose);
socket._httpMessage = null;
this.socket = this.connection = null;
};

ServerResponse.prototype.writeContinue = function() {
this._writeRaw('HTTP/1.1 100 Continue' + CRLF + CRLF, 'ascii');
this._sent100 = true;
@@ -23,15 +23,20 @@ var common = require('../common');
var assert = require('assert');
var http = require('http');

var gotEnd = false;
var requestGotEnd = false;
var responseGotEnd = false;

var server = http.createServer(function(req, res) {
res.writeHead(200);
res.write('a');

req.on('close', function() {
console.error('aborted');
gotEnd = true;
console.error('request aborted');
requestGotEnd = true;
});
res.on('close', function() {
console.error('response aborted');
responseGotEnd = true;
});
});
server.listen(common.PORT);
@@ -51,5 +56,6 @@ server.on('listening', function() {
});

process.on('exit', function() {
assert.ok(gotEnd);
assert.ok(requestGotEnd);
assert.ok(responseGotEnd);
});

6 comments on commit dd9593c

@vulkanr

This comment has been minimized.

Copy link

vulkanr replied Jan 23, 2012

i think "assignSocket" is called every time on keep-alive connections. The "close" event listeners will just keep ramping up until hitting the listener limit.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis replied Jan 23, 2012

Do you have a test case I can try? The logic seems okay to me and I see a removal for every added 'close' listener.

@vulkanr

This comment has been minimized.

Copy link

vulkanr replied Jan 23, 2012

plain vanilla handler:

app.get('/test', function(req,res) {
res.end();
});

  1. every "curl" (using localhost) will result in an exception (actually showing a bigger problem than i reported):
    curl -v http://localhost:8888/test

Uncaught TypeError: Cannot call method 'emit' of null
at Socket.onServerResponseClose (http.js:780:21)
at Socket.emit (events.js:88:20)
at Array. (net.js:320:10)
at EventEmitter._tickCallback (node.js:192:40)

  1. to show the problem i reported - simply issue that "curl" with multiple urls (default in curl is keep-alive). example:
    curl -v http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test

server warning/exception:
(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace:
at Socket. (events.js:139:15)
at ServerResponse.assignSocket (http.js:786:10)
at HTTPParser.onIncoming (http.js:1454:11)
at HTTPParser.onHeadersComplete (http.js:102:31)
at Socket.ondata (http.js:1387:22)
at TCP.onread (net.js:354:27)

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis replied Jan 23, 2012

That's probably a bug in whatever library you're using. Below is a plain Node example that works fine with curl or ab -k:

require('http').createServer(function(req, res) {
  res.end();
}).listen(8000);
@vulkanr

This comment has been minimized.

Copy link

vulkanr replied Jan 23, 2012

you are right. sorry for the trouble. it was an old piece of code which got included in my test. upgrade to 0.6.8 kicked the error in.

@kelnishi

This comment has been minimized.

Copy link

kelnishi replied Feb 23, 2012

How did you resolve this? I've got the exact same error, but I'm not sure what old code would cause this.
edit: found the cause. It was the long-stack-traces module.

Please sign in to comment.
You can’t perform that action at this time.