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.

diff --git a/lib/http.js b/lib/http.js
index b035e79..0598ac0 100644 (file)
--- a/lib/http.js
+++ b/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_);
         }

diff --git a/test/simple/test-http-connect.js b/test/simple/test-http-connect.js
index 64b9714..668dda7 100644 (file)
--- a/test/simple/test-http-connect.js
+++ b/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));
 });

diff --git a/test/simple/test-http-upgrade-agent.js b/test/simple/test-http-upgrade-agent.js
index a87d5e8..1077a98 100644 (file)
--- a/test/simple/test-http-upgrade-agent.js
+++ b/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();