fix: promise should reject when action throws

When an action throws an exception, that should be caught by the circuit, recorded as a failure, and the returned promise should be rejected.
nodeshift · Mar 7, 2017 · 58dab98 · 58dab98
1 parent f2594d8
commit 58dab98
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 27 deletions.
diff --git a/lib/circuit.js b/lib/circuit.js
@@ -185,20 +185,20 @@ class CircuitBreaker extends EventEmitter {
 
     let timeout;
     let timeoutError = false;
-    try {
-      return new this.Promise((resolve, reject) => {
-        timeout = setTimeout(
-          () => {
-            timeoutError = true;
-            const error = new Error(`Timed out after ${this.options.timeout}ms`);
-            /**
-             * Emitted when the circuit breaker action takes longer than `options.timeout`
-             * @event CircuitBreaker#timeout
-             */
-            this.emit('timeout', error);
-            resolve(fallback(this, error, args) || fail(this, error, args));
-          }, this.options.timeout);
+    return new this.Promise((resolve, reject) => {
+      timeout = setTimeout(
+        () => {
+          timeoutError = true;
+          const error = new Error(`Timed out after ${this.options.timeout}ms`);
+          /**
+           * Emitted when the circuit breaker action takes longer than `options.timeout`
+           * @event CircuitBreaker#timeout
+           */
+          this.emit('timeout', error);
+          resolve(fallback(this, error, args) || fail(this, error, args));
+        }, this.options.timeout);
 
+      try {
         const result = this.action.apply(this.action, args);
         const promise = (typeof result.then === 'function')
           ? result
@@ -216,22 +216,23 @@ class CircuitBreaker extends EventEmitter {
               clearTimeout(timeout);
             }
           })
-          .catch((error) => {
-            clearTimeout(timeout);
-            fail(this, error, args);
-            const fb = fallback(this, error, args);
-            if (fb) return resolve(fb);
-            else reject(error);
-          });
-      });
-    } catch (error) {
-      clearTimeout(timeout);
-      fail(this, error, args);
-      return fallback(this, error, args);
-    }
+          .catch((error) =>
+            handleError(error, this, timeout, args, resolve, reject));
+      } catch (error) {
+        handleError(error, this, timeout, args, resolve, reject);
+      }
+    });
   }
 }
 
+function handleError (error, circuit, timeout, args, resolve, reject) {
+  clearTimeout(timeout);
+  fail(circuit, error, args);
+  const fb = fallback(circuit, error, args);
+  if (fb) resolve(fb);
+  else reject(error);
+}
+
 function fallback (circuit, err, args) {
   if (circuit[FALLBACK_FUNCTION]) {
     return new circuit.Promise((resolve, reject) => {

diff --git a/test/test.js b/test/test.js
@@ -201,12 +201,24 @@ test('Returns self from fallback()', (t) => {
     .catch(t.fail);
 });
 
+test('CircuitBreaker emits failure when action throws', (t) => {
+  t.plan(2);
+  const breaker = cb(() => { throw new Error('E_TOOMANYCHICKENTACOS'); });
+  breaker.fire()
+    .then(t.fail)
+    .catch((e) => {
+      t.equals(breaker.status.failures, 1, 'expected failure status');
+      t.equals(e.message, 'E_TOOMANYCHICKENTACOS', 'expected error message');
+      t.end();
+    });
+});
+
 test('CircuitBreaker emits failure when falling back', (t) => {
   t.plan(2);
   const breaker = cb(passFail).fallback(() => 'fallback value');
 
   breaker.on('failure', (err) => {
-    t.equals(err, 'Error: -1 is < 0', 'Unexpected error');
+    t.equals('Error: -1 is < 0', err, 'Expected failure');
   });
 
   breaker.fire(-1).then((result) => {