Merge pull request #19 from oortcloud/error-notopened

Add some test and fix a rare network condition that caused a crash

Author: sarfata

CHANGELOG.md

@@ -1,3 +1,7 @@
+0.3.3 - 2013-05-29
+ - fixed bug where an exception could be thrown when sending a message on a socket that is not opened anymore (issue #18)
+ - added some tests (work in progress)
+
 0.3.2 - 2013-04-08
   - fixed bug where client would reconnect when closing (@tarangp)
 

lib/ddp-client.js

@@ -7,6 +7,7 @@ DDPClient = function(opts) {
   var self = this;
 
   // default arguments
+  opts = opts || {};
   self.host = opts.host || 'localhost';
   self.port = opts.port || 3000;
   self.path = opts.path || 'websocket';
@@ -51,16 +52,12 @@ DDPClient.prototype._prepareHandlers = function() {
 
   self.socket.on('error', function(error) {
     self.emit('socket-error', error);
-    if (self.auto_reconnect) {
-      setTimeout(function() { self.connect(); }, self.auto_reconnect_timer);
-    }
+    self._recoverNetworkError();
   });
 
   self.socket.on('close', function(code, message) {
     self.emit('socket-close', code, message);
-    if (self.auto_reconnect && ! self._connectionFailed && ! self._isClosing) {
-      setTimeout(function() { self.connect(); }, self.auto_reconnect_timer);
-    }
+    self._recoverNetworkError();
   });
 
   self.socket.on('message', function(data, flags) {
@@ -70,11 +67,20 @@ DDPClient.prototype._prepareHandlers = function() {
   });
 };
 
+DDPClient.prototype._recoverNetworkError = function() {
+  var self = this;
+  if (self.auto_reconnect && ! self._connectionFailed && ! self._isClosing) {
+    setTimeout(function() { self.connect(); }, self.auto_reconnect_timer);
+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////
 // RAW, low level functions
 DDPClient.prototype._send = function(data) {
-
-  this.socket.send(JSON.stringify(data));
+  this.socket.send(JSON.stringify(data), function(error) {
+    // This callback to avoid an exception being thrown.
+    // if an error happened, the socket will throw an event and we will recover.
+  });
 };
 
 // handle a message from the server

package.json

@@ -1,6 +1,6 @@
 {
     "name": "ddp",
-    "version": "0.3.2",
+    "version": "0.3.3",
     "description": "Node.js module to connect to servers using DDP protocol.",
     "author": "Tom Coleman <[email protected]> (http://tom.thesnail.org), Mike Bannister <[email protected]> (http://po.ssibiliti.es)",
     "main": "lib/ddp-client",
@@ -13,5 +13,13 @@
         "ws": ">=0.4.23",
         "underscore": ">=1.3.3"
     },
+    "devDependencies": {
+        "mocha": "1.9.x",
+        "sinon": "1.7.x",
+        "rewire": "1.1.x"
+    },
+    "scripts": {
+      "test": "./node_modules/mocha/bin/mocha test"
+    },
     "engines": { "node": "*" }
-}
+}

test/ddp-client.js

@@ -0,0 +1,108 @@
+var assert = require('assert'),
+    sinon = require('sinon'),
+    rewire = require('rewire'),
+    events = require('events');
+
+var DDPClient = rewire("../lib/ddp-client");
+
+var wsConstructor, wsMock;
+
+function prepareMocks() {
+  wsMock = new events.EventEmitter();
+
+  wsConstructor = sinon.stub();
+  wsConstructor.returns(wsMock);
+  DDPClient.__set__('WebSocket', wsConstructor);
+}
+
+
+describe("Connect to remote server", function() {
+  beforeEach(function() {
+    prepareMocks();
+  });
+
+  it('should connect to localhost by default', function() {
+    new DDPClient().connect();
+
+    assert(wsConstructor.calledOnce);
+    assert(wsConstructor.calledWithNew());
+    assert(wsConstructor.call)
+    assert.deepEqual(wsConstructor.args, [['ws://localhost:3000/websocket']]);
+  });
+  it('should connect to the provided host', function() {
+    new DDPClient({'host': 'myserver.com'}).connect();
+    assert.deepEqual(wsConstructor.args, [['ws://myserver.com:3000/websocket']]);
+  });
+  it('should connect to the provided host and port', function() {
+    new DDPClient({'host': 'myserver.com', 'port': 42}).connect();
+    assert.deepEqual(wsConstructor.args, [['ws://myserver.com:42/websocket']]);
+  });
+  it('should use ssl if the port is 443', function() {
+    new DDPClient({'host': 'myserver.com', 'port': 443}).connect();
+    assert.deepEqual(wsConstructor.args, [['wss://myserver.com:443/websocket']]);
+  });
+});
+
+describe('Automatic reconnection', function() {
+  beforeEach(function() {
+    prepareMocks();
+  });
+
+
+  /* We should be able to get this test to work with clock.tick() but for some weird 
+     reasons it does not work. See: https://github.com/cjohansen/Sinon.JS/issues/283
+   */
+  it('should reconnect when the connection fails', function(done) {
+    var ddpclient = new DDPClient({ auto_reconnect_timer: 10 });
+
+    ddpclient.connect();
+    wsMock.emit('error', {});
+
+    // At this point, the constructor should have been called only once.
+    assert(wsConstructor.calledOnce);
+
+    setTimeout(function() {
+      // Now the constructor should have been called twice
+      assert(wsConstructor.calledTwice);
+      done();
+    }, 15);
+  });
+});
+
+describe("Network errors", function() {
+  beforeEach(function() {
+    prepareMocks();
+  })
+
+  // For some weird reasons (hard to reproduce) it happens that we try to send a message and
+  // get an exception throws at us because the connection is not opened anymore.
+  it ('should not crash when WebSocket throws a "not-opened" error', function() {
+    // Simulates the error generated by WebSocket if the connection is not opened:
+    // WebSocket.send (ws/lib/WebSocket.js:175:16)
+    wsMock.send = function(data, options, cb) {
+      if (typeof options == 'function') {
+        cb = options;
+        options = {};
+      }
+      // if (this.readyState != WebSocket.OPEN)
+      if (typeof cb == 'function') cb(new Error('not opened'));
+      else throw new Error('not opened');
+    };
+
+    var ddpclient = new DDPClient();
+    ddpclient.connect();
+
+    var errorCB = sinon.spy();
+    ddpclient.on('socket-error', errorCB);
+    var callCB = sinon.spy();
+
+    ddpclient.call('aServerMethod', [42], callCB);
+
+    // First of all, the previous call should not throw anything.
+    // The method callback should not be called
+    // The socket-error event should not be triggered now
+    // (but it will be triggered later by the socket)
+    assert(!callCB.calledOnce);
+    assert(!errorCB.calledOnce);
+  });
+});