Repository: brightcove/hot-shots
Pull request #97 (2019-01-09 01:44:43)

Fix error handler to only happen again on errors
As noted in https://github.com/brightcove/hot-shots/pull/96 this behavior changed recently.  Fix and add a bunch of tests in the area.


Patch:
diff --git a/lib/statsd.js b/lib/statsd.js
index 9382fe1..d738023 100644
--- a/lib/statsd.js
+++ b/lib/statsd.js
@@ -330,12 +330,14 @@ Client.prototype.sendMessage = function (message, callback) {
     }
     if (callback) {
       callback(errFormatted);
-    } else if (this.errorHandler) {
-      this.errorHandler(errFormatted);
     } else if (errFormatted) {
-      console.log(String(errFormatted));
-      // emit error ourselves on the socket for backwards compatibility
-      this.socket.emit('error', errFormatted);
+      if (this.errorHandler) {
+        this.errorHandler(errFormatted);
+      } else {
+        console.log(String(errFormatted));
+        // emit error ourselves on the socket for backwards compatibility
+        this.socket.emit('error', errFormatted);
+      }
     }
   };
 


Test patch:
diff --git a/test/errorHandling.js b/test/errorHandling.js
index 02bf9fd..ff3fec7 100644
--- a/test/errorHandling.js
+++ b/test/errorHandling.js
@@ -10,8 +10,70 @@ describe('#errorHandling', () => {
   let server;
   let statsd;
 
+  // we have some tests first outside of the normal testTypes() setup as we want to
+  // test with a broken server, which is just set up with tcp
+
+  it('should use errorHandler when server is broken and using buffers', done => {
+    // sometimes two errors show up, one with the initial connection
+    let seenError = false;
+
+    server = createServer('tcp_broken', address => {
+      statsd = createHotShotsClient({
+        host: address.address,
+        port: address.port,
+        protocol: 'tcp',
+        maxBufferSize: 1,
+        errorHandler(err) {
+          assert.ok(err);
+          if (! seenError) {
+            seenError = true;
+            done();
+          }
+        }
+      }, 'client');
+      statsd.increment('a', 42, null);
+      server.on('metrics', () => {
+        assert.ok(false);
+      });
+    });
+  });
+
   testTypes().forEach(([description, serverType, clientType]) => {
     describe(description, () => {
+      it('should not use errorHandler when there is not an error', done => {
+        server = createServer(serverType, address => {
+          statsd = createHotShotsClient({
+            host: address.address,
+            port: address.port,
+            protocol: serverType,
+            errorHandler() {
+              assert.ok(false);
+            }
+          }, clientType);
+          statsd.increment('a', 42, null);
+          server.on('metrics', () => {
+            done();
+          });
+        });
+      });
+
+      it('should not use errorHandler when there is not an error and using buffers', done => {
+        server = createServer(serverType, address => {
+          statsd = createHotShotsClient({
+            host: address.address,
+            port: address.port,
+            protocol: serverType,
+            maxBufferSize: 1,
+            errorHandler() {
+              assert.ok(false);
+            }
+          }, clientType);
+          statsd.increment('a', 42, null);
+          server.on('metrics', () => {
+            done();
+          });
+        });
+      });
 
       it('should use errorHandler for sendStat error', done => {
         server = createServer(serverType, address => {
diff --git a/test/helpers/helpers.js b/test/helpers/helpers.js
index 6d5dbdf..e7e7dfd 100644
--- a/test/helpers/helpers.js
+++ b/test/helpers/helpers.js
@@ -8,6 +8,7 @@ const CHILD_CLIENT = 'child client';
 const CHILD_CHILD_CLIENT = 'child child client';
 const TCP = 'tcp';
 const UDP = 'udp';
+const TCP_BROKEN = 'tcp_broken';
 // tcp puts a newline at the end but udp does not
 const TCP_METRIC_END = '\n';
 const UDP_METRIC_END = '';
@@ -97,6 +98,22 @@ function createServer(serverType, onListening) {
 
     server.listen(0, '127.0.0.1');
   }
+  else if (serverType === TCP_BROKEN) {
+    server = net.createServer(socket => {
+      socket.setEncoding('ascii');
+      socket.on('data', data => {
+        if (data) {
+          server.emit('metrics', data);
+        }
+      });
+      socket.destroy();
+    });
+    server.on('listening', (socket) => {
+      onListening(server.address());
+    });
+
+    server.listen(0, '127.0.0.1');
+  }
   else {
     throw new Error(`Unknown server type: ${serverType}`);
   }