Only end upgrade socket connections if unhandled

This fixes an issue where multiple engine.io instances (on different
paths) would result in the closing of websocket connections for all of
the instances. This happened because each engine.io instance would
register an `upgrade` handler on the server. This handler would check
for a matching path and otherwise call `socket.end()` Since multiple
upgrade events would be triggered with different paths, the peer
handlers would close each other.

This patch resolves this behavior in the following way:
- When an instance upgrade handler encounters a path which it does not
  recognize it creates a timeout for `destroyUpgradeTimeout`.
- At the end of the timeout, the socket is checked for writable state
  and bytes written. If there has been not activity and the socket is
  writable, then it will be ended.

This allows for peer socket handlers to keep the socket alive by sending
some data over it. This also mimics the core node behavior of closing
sockets on upgrade when no handler is specified. We consider not
handling an upgrade request similar to no handler. However, we cannot
immediately end the socket for the reasons noted above.

fixes #143

README.md

@@ -136,6 +136,7 @@ These are exposed by `require('engine.io')`:
       - `path` (`String`): name of the path to capture (`/engine.io`).
       - `policyFile` (`Boolean`): whether to handle policy file requests (`true`)
       - `destroyUpgrade` (`Boolean`): destroy unhandled upgrade requests (`true`)
+      - `destroyUpgradeTimeout` (`Number`): milliseconds after which unhandled requests are ended (`1000`)
       - **See Server options below for additional options you can pass**
     - **Returns** `Server`
 

lib/engine.io.js

@@ -97,6 +97,9 @@ exports.attach = function (server, options) {
   var options = options || {};
   var path = (options.path || '/engine.io').replace(/\/$/, '');
 
+  var destroyUpgrade = (options.destroyUpgrade !== undefined) ? options.destroyUpgrade : true;
+  var destroyUpgradeTimeout = options.destroyUpgradeTimeout || 1000;
+
   // normalize path
   path += '/';
 
@@ -126,7 +129,15 @@ exports.attach = function (server, options) {
       if (check(req)) {
         engine.handleUpgrade(req, socket, head);
       } else if (false !== options.destroyUpgrade) {
-        socket.end();
+        // default node behavior is to disconnect when no handlers
+        // but by adding a handler, we prevent that
+        // and if no eio thing handles the upgrade
+        // then the socket needs to die!
+        setTimeout(function() {
+           if (socket.writable && socket.bytesWritten <= 0) {
+             return socket.end();
+           }
+        }, options.destroyUpgradeTimeout);
       }
     });
   }

test/engine.io.js

@@ -145,7 +145,7 @@ describe('engine', function () {
 
     it('should not destroy unhandled upgrades with destroyUpgrade:false', function (done) {
       var server = http.createServer()
-        , engine = eio.attach(server, { destroyUpgrade: false });
+        , engine = eio.attach(server, { destroyUpgrade: false, destroyUpgradeTimeout: 50 });
 
       server.listen(function () {
         var client = net.createConnection(server.address().port);
@@ -160,7 +160,7 @@ describe('engine', function () {
           var check = setTimeout(function () {
             client.removeListener('end', onEnd);
             done();
-          }, 20);
+          }, 100);
 
           function onEnd () {
             done(new Error('Client should not end'));
@@ -171,6 +171,70 @@ describe('engine', function () {
       });
     });
 
+    it('should destroy unhandled upgrades with after a timeout', function (done) {
+      var server = http.createServer()
+        , engine = eio.attach(server, { destroyUpgradeTimeout: 200 });
+
+      server.listen(function () {
+        var client = net.createConnection(server.address().port);
+        client.on('connect', function () {
+          client.setEncoding('ascii');
+          client.write([
+              'GET / HTTP/1.1'
+            , 'Upgrade: IRC/6.9'
+            , '', ''
+          ].join('\r\n'));
+
+          // send from client to server
+          // tests that socket is still alive
+          // this will not keep the socket open as the server does not handle it
+          setTimeout(function() {
+            client.write('foo');
+          }, 100);
+
+          function onEnd () {
+            done();
+          }
+
+          client.on('end', onEnd);
+        });
+      });
+    });
+
+    it('should not destroy handled upgrades with after a timeout', function (done) {
+      var server = http.createServer()
+        , engine = eio.attach(server, { destroyUpgradeTimeout: 100 });
+
+      // write to the socket to keep engine.io from closing it by writing before the timeout
+      server.on('upgrade', function(req, socket) {
+        socket.write('foo');
+        socket.on('data', function(chunk) {
+          expect(chunk.toString()).to.be('foo');
+          socket.end();
+        });
+      });
+
+      server.listen(function () {
+        var client = net.createConnection(server.address().port);
+
+        client.on('connect', function () {
+          client.setEncoding('ascii');
+          client.write([
+              'GET / HTTP/1.1'
+            , 'Upgrade: IRC/6.9'
+            , '', ''
+          ].join('\r\n'));
+
+          // test that socket is still open by writing after the timeout period
+          setTimeout(function() {
+            client.write('foo');
+          }, 200);
+
+          client.on('end', done);
+        });
+      });
+    });
+
     it('should preserve original request listeners', function (done) {
       var listeners = 0
         , server = http.createServer(function (req, res) {

test/server.js

@@ -1115,6 +1115,9 @@ describe('server', function () {
           });
         });
       });
+
+      // attach another engine to make sure it doesn't break upgrades
+      var e2 = eio.attach(engine.httpServer, { path: '/foo' });
     });
   });