Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

http: Upgrade/CONNECT request should detach its socket earlier #2510

Closed
wants to merge 1 commit into from

2 participants

@koichik
Owner

With Upgrade or CONNECT request, http.ClientRequest emits 'close' event after its socket is closed. However, after receiving a response, the socket is not under management by the request.

http.ClientRequest should detach the socket before 'upgrade'/'connect' event is emitted to pass the socket to a user. After that, it should emit 'close' event immediately without waiting for closing of the socket.

Please review.

@koichik koichik http: Upgrade/CONNECT request should detach its socket earlier
With Upgrade or CONNECT request, http.ClientRequest emits 'close' event
after its socket is closed. However, after receiving a response, the socket
is not under management by the request.

http.ClientRequest should detach the socket before 'upgrade'/'connect'
event is emitted to pass the socket to a user. After that, it should
emit 'close' event immediately without waiting for closing of the socket.
97b46ab
@bnoordhuis

LGTM, Koichi.

@koichik
Owner

Thanks Ben, merging.

@koichik koichik closed this pull request from a commit
@koichik koichik http: Upgrade/CONNECT request should detach its socket earlier
With Upgrade or CONNECT request, http.ClientRequest emits 'close' event
after its socket is closed. However, after receiving a response, the socket
is not under management by the request.

http.ClientRequest should detach the socket before 'upgrade'/'connect'
event is emitted to pass the socket to a user. After that, it should
emit 'close' event immediately without waiting for closing of the socket.

Fixes #2510.
7dffbaf
@koichik koichik closed this in 7dffbaf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 11, 2012
  1. @koichik

    http: Upgrade/CONNECT request should detach its socket earlier

    koichik authored
    With Upgrade or CONNECT request, http.ClientRequest emits 'close' event
    after its socket is closed. However, after receiving a response, the socket
    is not under management by the request.
    
    http.ClientRequest should detach the socket before 'upgrade'/'connect'
    event is emitted to pass the socket to a user. After that, it should
    emit 'close' event immediately without waiting for closing of the socket.
This page is out of date. Refresh to see the latest.
View
10 lib/http.js
@@ -1195,8 +1195,14 @@ ClientRequest.prototype.onSocket = function(socket) {
var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade';
if (req.listeners(eventName).length) {
req.upgradeOrConnect = true;
- req.emit(eventName, res, socket, bodyHead);
+
+ // detach the socket
socket.emit('agentRemove');
+ socket.removeListener('close', closeListener);
+ socket.removeListener('error', errorListener);
+
+ req.emit(eventName, res, socket, bodyHead);
+ req.emit('close');
} else {
// Got Upgrade header or CONNECT method, but have no handler.
socket.destroy();
@@ -1316,7 +1322,7 @@ ClientRequest.prototype._deferToConnect = function(method, arguments_, cb) {
}
if (cb) { cb(); }
} else {
- self.socket.on('connect', function() {
+ self.socket.once('connect', function() {
if (method) {
self.socket[method].apply(self.socket, arguments_);
}
View
28 test/simple/test-http-connect.js
@@ -53,16 +53,39 @@ server.listen(common.PORT, function() {
}, function(res) {
assert(false);
});
+
+ var clientRequestClosed = false;
+ req.on('close', function() {
+ clientRequestClosed = true;
+ });
+
req.on('connect', function(res, socket, firstBodyChunk) {
common.debug('Client got CONNECT request');
clientGotConnect = true;
+ // Make sure this request got removed from the pool.
+ var name = 'localhost:' + common.PORT;
+ assert(!http.globalAgent.sockets.hasOwnProperty(name));
+ assert(!http.globalAgent.requests.hasOwnProperty(name));
+
+ // Make sure this socket has detached.
+ assert(!socket.ondata);
+ assert(!socket.onend);
+ assert.equal(socket.listeners('connect').length, 0);
+ assert.equal(socket.listeners('data').length, 0);
+ assert.equal(socket.listeners('end').length, 0);
+ assert.equal(socket.listeners('free').length, 0);
+ assert.equal(socket.listeners('close').length, 0);
+ assert.equal(socket.listeners('error').length, 0);
+ assert.equal(socket.listeners('agentRemove').length, 0);
+
var data = firstBodyChunk.toString();
socket.on('data', function(buf) {
data += buf.toString();
});
socket.on('end', function() {
assert.equal(data, 'HeadBody');
+ assert(clientRequestClosed);
server.close();
});
socket.write('Body');
@@ -79,9 +102,4 @@ server.listen(common.PORT, function() {
process.on('exit', function() {
assert.ok(serverGotConnect);
assert.ok(clientGotConnect);
-
- // Make sure this request got removed from the pool.
- var name = 'localhost:' + common.PORT;
- assert(!http.globalAgent.sockets.hasOwnProperty(name));
- assert(!http.globalAgent.requests.hasOwnProperty(name));
});
View
8 test/simple/test-http-upgrade-agent.js
@@ -74,11 +74,11 @@ srv.listen(common.PORT, '127.0.0.1', function() {
'connection': 'upgrade',
'upgrade': 'websocket' };
assert.deepEqual(expectedHeaders, res.headers);
- assert.equal(http.globalAgent.sockets[name].length, 1);
- process.nextTick(function() {
- // Make sure this request got removed from the pool.
- assert(!http.globalAgent.sockets.hasOwnProperty(name));
+ // Make sure this request got removed from the pool.
+ assert(!http.globalAgent.sockets.hasOwnProperty(name));
+
+ req.on('close', function() {
socket.end();
srv.close();
Something went wrong with that request. Please try again.