fix: keep all sockets in the case an old socket will survive

Before this fix, if another socket.io connection (socket) got established, Karma would forget the old one, expecting the old one will get disconnected at some point. It can however happen, that the new socket will get disconnected. This changes `Browser` to keep all socket objects.
karma-runner · Dec 3, 2013 · a5945eb · a5945eb
1 parent 1f1a8eb
commit a5945eb
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 17 deletions.
diff --git a/lib/browser.js b/lib/browser.js
@@ -26,6 +26,12 @@ var Browser = function(id, fullName, /* capturedBrowsers */ collection, emitter,
 
   var name = helper.browserFullNameToShort(fullName);
   var log = logger.create(name);
+  var activeSockets = [socket];
+  var activeSocketsIds = function() {
+    return activeSockets.map(function(s) {
+      return s.id;
+    }).join(', ');
+  };
 
   this.id = id;
   this.fullName = fullName;
@@ -110,12 +116,19 @@ var Browser = function(id, fullName, /* capturedBrowsers */ collection, emitter,
   var disconnect = function() {
     self.state = DISCONNECTED;
     self.disconnectsCount++;
-    log.warn('Disconnected');
+    log.warn('Disconnected (%d times)', self.disconnectsCount);
     collection.remove(self);
   };
 
   var pendingDisconnect;
-  this.onDisconnect = function() {
+  this.onDisconnect = function(_, disconnectedSocket) {
+    activeSockets.splice(activeSockets.indexOf(disconnectedSocket), 1);
+
+    if (activeSockets.length) {
+      log.debug('Disconnected %s, still have %s', disconnectedSocket.id, activeSocketsIds());
+      return;
+    }
+
     if (this.state === READY) {
       disconnect();
     } else if (this.state === EXECUTING) {
@@ -134,11 +147,9 @@ var Browser = function(id, fullName, /* capturedBrowsers */ collection, emitter,
   this.reconnect = function(newSocket) {
     if (this.state === EXECUTING_DISCONNECTED) {
       this.state = EXECUTING;
-      log.debug('Reconnected.');
+      log.debug('Reconnected on %s.', newSocket.id);
     } else if (this.state === EXECUTING || this.state === READY) {
-      log.debug('New connection %s, forgetting %s.', newSocket.id, socket.id);
-      // TODO(vojta): this should only remove this browser.onDisconnect listener
-      socket.removeAllListeners('disconnect');
+      log.debug('New connection %s (already have %s)', newSocket.id, activeSocketsIds());
     } else if (this.state === DISCONNECTED) {
       this.state = READY;
       log.info('Connected on socket %s with id %s', newSocket.id, this.id);
@@ -150,7 +161,7 @@ var Browser = function(id, fullName, /* capturedBrowsers */ collection, emitter,
       emitter.emit('browser_register', this);
     }
 
-    socket = newSocket;
+    activeSockets.push(newSocket);
     events.bindAll(this, newSocket);
     if (pendingDisconnect) {
       timer.clearTimeout(pendingDisconnect);
@@ -181,7 +192,10 @@ var Browser = function(id, fullName, /* capturedBrowsers */ collection, emitter,
   };
 
   this.execute = function(config) {
-    socket.emit('execute', config);
+    activeSockets.forEach(function(socket) {
+      socket.emit('execute', config);
+    });
+
     this.state = EXECUTING;
   };
 };

diff --git a/lib/events.js b/lib/events.js
@@ -8,9 +8,17 @@ var helper = require('./helper');
 var bindAllEvents = function(object, context) {
   context = context || this;
 
+  var bindMethod = function(method) {
+    context.on(helper.camelToSnake(method.substr(2)), function() {
+      var args = Array.prototype.slice.call(arguments, 0);
+      args.push(context);
+      object[method].apply(object, args);
+    });
+  }
+
   for (var method in object) {
     if (helper.isFunction(object[method]) && method.substr(0, 2) === 'on') {
-      context.on(helper.camelToSnake(method.substr(2)), object[method].bind(object));
+      bindMethod(method);
     }
   }
 };

diff --git a/test/unit/browser.spec.coffee b/test/unit/browser.spec.coffee
@@ -212,7 +212,7 @@ describe 'Browser', ->
     it 'should remove from parent collection', ->
       expect(collection.length).to.equal 1
 
-      browser.onDisconnect()
+      browser.onDisconnect 'socket.io-reason', socket
       expect(collection.length).to.equal 0
 
 
@@ -221,7 +221,7 @@ describe 'Browser', ->
       emitter.on 'browser_complete', spy
       browser.state = Browser.STATE_EXECUTING
 
-      browser.onDisconnect()
+      browser.onDisconnect 'socket.io-reason', socket
       timer.wind 20
 
       expect(browser.lastResult.disconnected).to.equal true
@@ -233,7 +233,7 @@ describe 'Browser', ->
       emitter.on 'browser_complete', spy
       browser.state = Browser.STATE_READY
 
-      browser.onDisconnect()
+      browser.onDisconnect 'socket.io-reason', socket
       expect(spy).not.to.have.been.called
 
 
@@ -249,7 +249,7 @@ describe 'Browser', ->
       browser.init()
       browser.state = Browser.STATE_EXECUTING
 
-      browser.onDisconnect()
+      browser.onDisconnect 'socket.io-reason', socket
       browser.reconnect new e.EventEmitter
 
       timer.wind 10
@@ -273,7 +273,7 @@ describe 'Browser', ->
       expect(browser.lastResult.error).to.equal true
 
       # should be ignored, keep executing
-      socket.emit 'disconnect'
+      socket.emit 'disconnect', 'socket.io reason'
       expect(browser.state).to.equal Browser.STATE_EXECUTING
 
 
@@ -385,7 +385,7 @@ describe 'Browser', ->
       browser.init()
       browser.state = Browser.STATE_EXECUTING
       socket.emit 'result', {success: true, suite: [], log: []}
-      socket.emit 'disconnect'
+      socket.emit 'disconnect', 'socket.io reason'
       expect(browser.isReady()).to.equal false
 
       newSocket = new e.EventEmitter
@@ -407,7 +407,7 @@ describe 'Browser', ->
       browser.init()
       browser.state = Browser.STATE_EXECUTING
       socket.emit 'result', {success: true, suite: [], log: []}
-      socket.emit 'disconnect'
+      socket.emit 'disconnect', 'socket.io reason'
 
       timer.wind 10
       expect(browser.lastResult.disconnected).to.equal true
@@ -424,7 +424,7 @@ describe 'Browser', ->
       socket.emit 'result', {success: true, suite: [], log: []}
       socket.emit 'result', {success: false, suite: [], log: []}
       socket.emit 'result', {skipped: true, suite: [], log: []}
-      socket.emit 'disconnect'
+      socket.emit 'disconnect', 'socket.io reason'
       timer.wind 10 # wait-for reconnecting delay
       expect(browser.state).to.equal Browser.STATE_DISCONNECTED
       expect(browser.disconnectsCount).to.equal 1
@@ -443,3 +443,24 @@ describe 'Browser', ->
       expect(browser.lastResult.success).to.equal 1
       expect(browser.lastResult.failed).to.equal 0
       expect(browser.lastResult.skipped).to.equal 0
+
+
+    it 'keeping multiple active sockets', ->
+      # If there is a new connection (socket) for an already connected browser,
+      # we need to keep the old socket, in the case that the new socket will disconnect.
+      browser = new Browser 'fake-id', 'Chrome 31.0', collection, emitter, socket, null, 10
+      browser.init()
+      browser.execute()
+
+      # A second connection...
+      newSocket = new e.EventEmitter
+      browser.reconnect newSocket
+
+      # Disconnect the second connection...
+      browser.onDisconnect 'socket.io-reason', newSocket
+      expect(browser.state).to.equal Browser.STATE_EXECUTING
+
+      # It should still be listening on the old socket.
+      socket.emit 'result', {success: true, suite: [], log: []}
+      expect(browser.lastResult.success).to.equal 1
+
diff --git a/test/unit/events.spec.coffee b/test/unit/events.spec.coffee
@@ -107,6 +107,16 @@ describe 'events', ->
 
       expect(object.onFoo).to.have.been.called
 
+
+    it 'should append "context" to event arguments', ->
+      object = sinon.stub onFoo: ->
+
+      e.bindAll object, emitter
+      emitter.emit 'foo', 'event-argument'
+
+      expect(object.onFoo).to.have.been.calledWith 'event-argument', emitter
+
+
   #============================================================================
   # events.bufferEvents
   #============================================================================