Test Got

README.md

@@ -59,10 +59,7 @@ any node.js http request library. Circuit-b is currently tested to be working wi
  * [request.js](https://github.com/request/request)
  * [node-fetch](https://github.com/bitinn/node-fetch)
  * [axios](https://github.com/axios/axios)
-
-The following http request libraries is to be included in these tests (PRs welcome):
-
- * [got](https://github.com/sindresorhus/got)
+ * [got](https://github.com/sindresorhus/got) - [[note](https://github.com/trygve-lie/circuit-b#clients-with-retries)]
 
 
 ## Constructor
@@ -316,6 +313,37 @@ request('http://api.somewhere.com', (error, response, body) => {
 ```
 
 
+## Clients with retries
+
+There are http clients out there which will retry http requests under the hood if
+it encounters certain errors (network errors, server errors etc).
+
+This plays out as follow: one initializes a request with the http client where
+it ex is configured to retry two times if it encounters an error. When the client
+encounters an error is should retry on, it will under the hood, assuming the
+errors continue, do two extra requests in addition to the initial request before
+it surfaces the error to the user of the client. In this example; what looks like
+one request for the user of the client are in reality three requests.
+
+Since circuit-b listen on [`Net.socket`](https://nodejs.org/api/net.html#net_class_net_socket)
+events it will register the retry requests on clients with such features. In the
+above example, circuit-b will have registered three failed requests. Not one.
+
+The Circuit Breaker pattern is a pattern to reduce traffic to a upstream service
+when it errors. A retry pattern on a http client is quite the opposite of this
+pattern.
+
+Due to this; it is adviced that if the http client has retry functionallity, this
+feature should be configured to off or set to 0 retries.
+
+If one are not able to configure this it might be valuable to set `maxFailures` a
+bit higher so there is a bit higher margin before the breaker kicks in.
+
+Known http clients with default retries:
+
+ * [got](https://github.com/sindresorhus/got)
+
+
 ## node.js compabillity
 
 This module use [async hooks](https://nodejs.org/api/async_hooks.html) which was first

lib/breaker.js

@@ -117,6 +117,8 @@ const Breaker = class Breaker extends EventEmitter {
         if (this.failures === this.maxFailures) {
             this.tripped = Date.now() + this.maxAge;
         }
+
+        return true;
     }
 
     reset() {

lib/main.js

@@ -47,6 +47,7 @@ const CircuitB = class CircuitB extends EventEmitter {
             init: (asyncId, type, triggerAsyncId, resource) => {
                 if (type === 'TCPWRAP') {
                     process.nextTick(() => {
+                        let tripped = false;
                         let breaker;
                         let timer;
 
@@ -55,7 +56,6 @@ const CircuitB = class CircuitB extends EventEmitter {
                         resource.owner.once('lookup', (err, address, family, host) => {
                             if (this.registry.has(host)) {
                                 breaker = this.registry.get(host);
-
                                 if (breaker.check()) {
                                     this.log.debug('Circuit breaker is open', breaker.host, asyncId);
                                     resource.owner.destroy(new errors.CircuitBreakerOpenExceptionError(`Circuit breaker is open for host: ${breaker.host}`));
@@ -70,61 +70,90 @@ const CircuitB = class CircuitB extends EventEmitter {
                         });
 
                         resource.owner.once('error', (error) => {
+                            if (tripped) {
+                                return;
+                            }
+
+                            if (timer) {
+                                clearTimeout(timer);
+                            }
+
                             if (breaker) {
                                 this.log.debug('Circuit breaker got "error" event on resource', breaker.host, asyncId);
                                 if (error instanceof errors.CircuitBreakerOpenExceptionError) {
                                     return;
                                 }
 
-                                clearTimeout(timer);
-                                breaker.trip();
+                                tripped = breaker.trip();
                             }
                         });
 
                         resource.owner.once('timeout', () => {
+                            if (tripped) {
+                                return;
+                            }
+
+                            if (timer) {
+                                clearTimeout(timer);
+                            }
+
                             if (breaker) {
                                 this.log.debug('Circuit breaker got "timeout" event on resource', breaker.host, asyncId);
-                                breaker.trip();
-                                clearTimeout(timer);
+                                tripped = breaker.trip();
                             }
                         });
 
                         resource.owner.once('abort', () => {
+                            if (tripped) {
+                                return;
+                            }
+
                             if (breaker) {
                                 this.log.debug('Circuit breaker got "abort" event on resource', breaker.host, asyncId);
-                                breaker.trip();
-                                clearTimeout(timer);
+                                tripped = breaker.trip();
                             }
                         });
 
                         resource.owner.once('data', () => {
-                            if (breaker) {
+                            if (timer) {
                                 clearTimeout(timer);
                             }
+
+                            if (breaker) {
+                                this.log.debug('Circuit breaker got "data" event on resource', breaker.host, asyncId);
+                            }
                         });
 
                         resource.owner.once('end', () => {
+                            if (tripped) {
+                                return;
+                            }
+
+                            if (timer) {
+                                clearTimeout(timer);
+                            }
+
                             if (breaker) {
                                 // eslint-disable-next-line no-underscore-dangle
                                 const httpMsg = resource.owner._httpMessage;
 
                                 if (!httpMsg || !httpMsg.res) {
                                     this.log.debug('Circuit breaker got "end" event on resource - "http message" is missing', breaker.host, asyncId);
-                                    breaker.trip();
+                                    tripped = breaker.trip();
                                     return;
                                 }
 
                                 const code = httpMsg.res.statusCode;
 
                                 if (code >= 400 && code <= 499) {
                                     this.log.debug('Circuit breaker got "end" event on resource - http status is 4xx', breaker.host, asyncId);
-                                    breaker.trip();
+                                    tripped = breaker.trip();
                                     return;
                                 }
 
                                 if (code >= 500 && code <= 599) {
                                     this.log.debug('Circuit breaker got "end" event on resource - http status is 5xx', breaker.host, asyncId);
-                                    breaker.trip();
+                                    tripped = breaker.trip();
                                     return;
                                 }
 

package.json

@@ -31,6 +31,7 @@
     "test:fetch": "tape test/integration-fetch.js | tap-summary",
     "test:axios": "tape test/integration-axios.js | tap-summary",
     "test:http": "tape test/integration-http.js | tap-summary",
+    "test:got": "tape test/integration-got.js | tap-summary",
     "test:breaker": "tape test/breaker.js | tap-summary",
     "test:main": "tape test/main.js | tap-summary",
     "lint": "eslint .",
@@ -44,6 +45,7 @@
     "request": "^2.88.0",
     "axios": "^0.18.0",
     "node-fetch": "^2.2.0",
+    "got": "^9.2.2",
     "synclog": "^1.0.1",
     "tap-summary": "^4.0.0",
     "tape": "^4.9.1"

test/breaker.js

@@ -1,7 +1,7 @@
 'use strict';
 
 const test = require('tape');
-const { sleep, within } = require('./integration/utils');
+const { sleep, within } = require('../utils/utils');
 const Breaker = require('../lib/breaker');
 
 /**
@@ -121,6 +121,12 @@ test('.trip() - breaker is "closed" - reaches max failures - should set "tripped
     t.end();
 });
 
+test('.trip() - call method - should return "true"', (t) => {
+    const breaker = new Breaker('circuit-b.local');
+    const result = breaker.trip();
+    t.true(result);
+    t.end();
+});
 
 /**
  * .check()

test/integration-axios.js

@@ -2,7 +2,7 @@
 
 const test = require('tape');
 const axios = require('axios');
-const { before, after } = require('./integration/utils');
+const { before, after } = require('../utils/utils');
 const timeout = require('./integration/timeout');
 const http400 = require('./integration/http-status-400');
 const http500 = require('./integration/http-status-500');

test/integration-fetch.js

@@ -2,7 +2,7 @@
 
 const fetch = require('node-fetch');
 const test = require('tape');
-const { before, after } = require('./integration/utils');
+const { before, after } = require('../utils/utils');
 const timeout = require('./integration/timeout');
 const http400 = require('./integration/http-status-400');
 const http500 = require('./integration/http-status-500');