Repository: node-influx/node-influx (ts)

Issue:
Don't disable host if backoff delay is zero
## Expected Behavior

When a request fails, if some conditions are met, host is disabled during an specific delay, acting as a circuit breaker. This delay can be configured through custom `IBackoffStrategy` classes. If this delay is zero, host should not be disabled.

## Actual Behavior

What is happening is that host is disabled for zero milliseconds. Host re-enable is done in a `setTimeout` callback, so it's effectively disabled until event loop triggers setTimeout execution.

## Steps/Code to Reproduce the Problem

1. Create a custom backoff strategy.

```typescript
import { IBackoffStrategy } from "influx/backoff/backoff";

export class ZeroBackoff implements IBackoffStrategy {

  public getDelay(): number {
    return 0;
  }

  public next(): IBackoffStrategy {
    return this;
  }

  public reset(): IBackoffStrategy {
    return this;
  }
}
```

2. Execute any request that fails with a 500 error (you can use a proxy like nginx to simulate that error). Check that even when backoff is zero, hosts are being disabled:

```typescript
import { InfluxDB } from "influx";
import { Pool } from "influx/pool";

const influx = new InfluxDB({
  database: "test",
  pool: {
    backoff: new ZeroBackoff(),
  }
});

influx.writePoints([
  {
    timestamp: new Date("2021-01-29T00:00:00Z"),
    measurement: "test",
    fields: {
      "test": 1
    }
  }
]).catch(() => {
  console.log(((influx as any)._pool as Pool).getHostsDisabled().length);
});
```

## Specifications

- Version: 5.6.3
- Platform:
- Subsystem:


Pull request:
fix: don't disable pool host if backoff is zero
Fixes #544 

## Proposed Changes

  - Check if host should be disabled. It's keep enabled if `host.fail()` (which calls the backoff getter) returns zero. It will only disable the host if this value is greater than zero.
  - Add a constant backoff strategy as an alternative to the default exponential backoff. This was required to simplify test creation, but it also adds value for users that don't need exponential strategies.
  - Create tests for ConstantBackoff class and add two tests for the Pool class to show the changed behaviour.

---

## Checklist

- [x] A test has been added if appropriate
- [x] `npm test` completes successfully
- [x] Commit messages are in [semantic format](https://seesparkbox.com/foundry/semantic_commit_messages)


Patch:
diff --git a/src/backoff/constant.ts b/src/backoff/constant.ts
new file mode 100644
index 0000000..c4e5a1c
--- /dev/null
+++ b/src/backoff/constant.ts
@@ -0,0 +1,66 @@
+import { IBackoffStrategy } from "./backoff";
+
+/**
+ * IConstantOptions are passed into the ConstantBackoff constructor. The
+ * backoff equation is, the simplest possible, a constant delay with an
+ * optional jitter, delay*(1-random_jitter) and delay*(1+random_jitter).
+ *
+ */
+export interface IConstantOptions {
+  /**
+   * The constant delay passed to the equation.
+   */
+  delay: number;
+
+  /**
+   * Random factor between 0 and 1 to avoid the thundering herd problem.
+   */
+  jitter?: number;
+}
+
+/**
+ * Constant Backoff
+ */
+export class ConstantBackoff implements IBackoffStrategy {
+  protected options: IConstantOptions;
+
+  /**
+   * Creates a new constant backoff strategy.
+   * @param options
+   */
+  constructor(options: IConstantOptions) {
+    this.options = {
+      delay: Math.max(options.delay, 0),
+      jitter: Math.min(Math.max(options.jitter || 0, 0), 1),
+    };
+  }
+
+  /**
+   * @inheritDoc
+   */
+  public getDelay(): number {
+    let delay: number = this.options.delay;
+
+    if (this.options.jitter > 0) {
+      const min = delay * (1 - this.options.jitter);
+      const max = delay * (1 + this.options.jitter);
+      delay = Math.random() * (max - min) + min;
+    }
+
+    return delay;
+  }
+
+  /**
+   * @inheritDoc
+   */
+  public next(): IBackoffStrategy {
+    return this;
+  }
+
+  /**
+   * @inheritDoc
+   */
+  public reset(): IBackoffStrategy {
+    return this;
+  }
+}
diff --git a/src/pool.ts b/src/pool.ts
index e562fdc..78b1ad9 100644
--- a/src/pool.ts
+++ b/src/pool.ts
@@ -466,11 +466,15 @@ export class Pool {
    * re-enabled after a backoff interval
    */
   private _disableHost(host: Host): void {
-    this._hostsAvailable.delete(host);
-    this._hostsDisabled.add(host);
-    this._index %= Math.max(1, this._hostsAvailable.size);
+    const delay = host.fail();
 
-    setTimeout(() => this._enableHost(host), host.fail());
+    if (delay > 0) {
+      this._hostsAvailable.delete(host);
+      this._hostsDisabled.add(host);
+      this._index %= Math.max(1, this._hostsAvailable.size);
+
+      setTimeout(() => this._enableHost(host), delay);
+    }
   }
 
   private _handleRequestError(


Test patch:
diff --git a/test/unit/backoff.test.ts b/test/unit/backoff.test.ts
index e209ca3..dbe07ec 100644
--- a/test/unit/backoff.test.ts
+++ b/test/unit/backoff.test.ts
@@ -3,9 +3,41 @@
 import { expect } from "chai";
 
 import { IBackoffStrategy } from "../../src/backoff/backoff";
+import { ConstantBackoff } from "../../src/backoff/constant";
 import { ExponentialBackoff } from "../../src/backoff/exponential";
 
 describe("backoff strategies", () => {
+  describe("constant strategy", () => {
+    it("appears to work", () => {
+      let exp: IBackoffStrategy = new ConstantBackoff({
+        delay: 500,
+        jitter: 0.5,
+      });
+
+      function next(): number {
+        const value = exp.getDelay();
+        exp = exp.next();
+        return value;
+      }
+
+      const checkSequence = (): void => {
+        expect(next()).to.be.within(500 * (1 - 0.5), 500 * (1 + 0.5));
+        expect(next()).to.be.within(500 * (1 - 0.5), 500 * (1 + 0.5));
+        expect(next()).to.be.within(500 * (1 - 0.5), 500 * (1 + 0.5));
+        expect(next()).to.be.within(500 * (1 - 0.5), 500 * (1 + 0.5));
+        expect(next()).to.be.within(500 * (1 - 0.5), 500 * (1 + 0.5));
+        expect(next()).to.be.within(500 * (1 - 0.5), 500 * (1 + 0.5));
+      };
+
+      checkSequence();
+      exp = exp.reset();
+      const dupe = exp.reset();
+      checkSequence();
+      exp = dupe;
+      checkSequence();
+    });
+  });
+
   describe("exponential strategy", () => {
     it("appears to work", () => {
       let exp: IBackoffStrategy = new ExponentialBackoff({
diff --git a/test/unit/pool.test.ts b/test/unit/pool.test.ts
index 9dc071e..5f872ad 100644
--- a/test/unit/pool.test.ts
+++ b/test/unit/pool.test.ts
@@ -8,6 +8,8 @@ import * as sinon from "sinon";
 import * as https from "https";
 
 import { ExponentialBackoff } from "../../src/backoff/exponential";
+import { IBackoffStrategy } from "../../src/backoff/backoff";
+import { ConstantBackoff, IConstantOptions } from "../../src/backoff/constant";
 import { Pool, RequestError, ServiceNotAvailableError } from "../../src/pool";
 
 const hosts = 2;
@@ -18,13 +20,15 @@ describe("pool", () => {
   let server: http.Server;
   let sid: string; // Random string to avoid conflicts with other running tests
 
-  const createPool = (): Pool => {
+  const createPool = (backoff?: IBackoffStrategy): Pool => {
     return new Pool({
-      backoff: new ExponentialBackoff({
-        initial: 300,
-        random: 0,
-        max: 10 * 1000,
-      }),
+      backoff:
+        backoff ||
+        new ExponentialBackoff({
+          initial: 300,
+          random: 0,
+          max: 10 * 1000,
+        }),
     });
   };
 
@@ -207,69 +211,117 @@ describe("pool", () => {
   });
 
   describe("backoff", () => {
-    beforeEach(() => {
-      clock = sinon.useFakeTimers();
-      return pool.discard({ method: "GET", path: "/pool/502" }).catch(() => {
-        /* ignore */
+    describe("exponential", () => {
+      beforeEach(() => {
+        clock = sinon.useFakeTimers();
+        return pool.discard({ method: "GET", path: "/pool/502" }).catch(() => {
+          /* ignore */
+        });
       });
-    });
 
-    it("should error if there are no available hosts", () => {
-      return pool
-        .discard({ method: "GET", path: "/pool/json" })
-        .then(() => {
-          throw new Error("Expected to have thrown");
-        })
-        .catch((err) => {
-          expect(err).to.be.an.instanceof(ServiceNotAvailableError);
-          expect(err.message).to.equal("No host available");
-        });
-    });
+      it("should error if there are no available hosts", () => {
+        return pool
+          .discard({ method: "GET", path: "/pool/json" })
+          .then(() => {
+            throw new Error("Expected to have thrown");
+          })
+          .catch((err) => {
+            expect(err).to.be.an.instanceof(ServiceNotAvailableError);
+            expect(err.message).to.equal("No host available");
+          });
+      });
 
-    it("should reenable hosts after the backoff expires", () => {
-      expect(pool.hostIsAvailable()).to.be.false;
-      clock.tick(300);
-      expect(pool.hostIsAvailable()).to.be.true;
-    });
+      it("should reenable hosts after the backoff expires", () => {
+        expect(pool.hostIsAvailable()).to.be.false;
+        clock.tick(300);
+        expect(pool.hostIsAvailable()).to.be.true;
+      });
 
-    it("should back off if failures continue", () => {
-      clock.tick(300);
-      expect(pool.hostIsAvailable()).to.be.true;
+      it("should back off if failures continue", () => {
+        clock.tick(300);
+        expect(pool.hostIsAvailable()).to.be.true;
 
-      return pool
-        .discard({ method: "GET", path: "/pool/502" })
-        .then(() => {
-          throw new Error("Expected to have thrown");
-        })
-        .catch((err) => {
-          expect(err).to.be.an.instanceof(ServiceNotAvailableError);
-          expect(pool.hostIsAvailable()).to.be.false;
-
-          clock.tick(300);
-          expect(pool.hostIsAvailable()).to.be.false;
-          clock.tick(300);
-          expect(pool.hostIsAvailable()).to.be.true;
-        });
+        return pool
+          .discard({ method: "GET", path: "/pool/502" })
+          .then(() => {
+            throw new Error("Expected to have thrown");
+          })
+          .catch((err) => {
+            expect(err).to.be.an.instanceof(ServiceNotAvailableError);
+            expect(pool.hostIsAvailable()).to.be.false;
+
+            clock.tick(300);
+            expect(pool.hostIsAvailable()).to.be.false;
+            clock.tick(300);
+            expect(pool.hostIsAvailable()).to.be.true;
+          });
+      });
+
+      it("should reset backoff after success", () => {
+        clock.tick(300);
+        expect(pool.hostIsAvailable()).to.be.true;
+
+        return pool
+          .discard({ method: "GET", path: "/pool/204" })
+          .then(() => {
+            return pool.discard({ method: "GET", path: "/pool/502" });
+          })
+          .then(() => {
+            throw new Error("Expected to have thrown");
+          })
+          .catch((err) => {
+            expect(err).not.to.be.undefined;
+            expect(pool.hostIsAvailable()).to.be.false;
+            clock.tick(300);
+            expect(pool.hostIsAvailable()).to.be.true;
+          });
+      });
     });
 
-    it("should reset backoff after success", () => {
-      clock.tick(300);
-      expect(pool.hostIsAvailable()).to.be.true;
+    describe("constant", () => {
+      const createConstantBackoffPool = (options: IConstantOptions): Pool => {
+        const p = createPool(new ConstantBackoff(options));
 
-      return pool
-        .discard({ method: "GET", path: "/pool/204" })
-        .then(() => {
-          return pool.discard({ method: "GET", path: "/pool/502" });
-        })
-        .then(() => {
-          throw new Error("Expected to have thrown");
-        })
-        .catch((err) => {
-          expect(err).not.to.be.undefined;
-          expect(pool.hostIsAvailable()).to.be.false;
-          clock.tick(300);
-          expect(pool.hostIsAvailable()).to.be.true;
-        });
+        for (let i = 0; i < hosts; i += 1) {
+          p.addHost(
+            process.env.WEBPACK ? location.origin : `http://127.0.0.1:${3005}`
+          );
+        }
+
+        return p;
+      };
+
+      it("should disable hosts if backoff delay is greater than zero", () => {
+        const p = createConstantBackoffPool({ delay: 300, jitter: 0 });
+
+        return p
+          .discard({ method: "GET", path: "/pool/502" })
+          .then(() => {
+            throw new Error("Expected to have thrown");
+          })
+          .catch((err) => {
+            expect(err).to.be.an.instanceof(ServiceNotAvailableError);
+            expect(err.message).to.equal("Bad Gateway");
+
+            expect(p.getHostsDisabled().length).to.be.greaterThan(0);
+          });
+      });
+
+      it("should not disable hosts if backoff delay is zero", () => {
+        const p = createConstantBackoffPool({ delay: 0, jitter: 0 });
+
+        return p
+          .discard({ method: "GET", path: "/pool/502" })
+          .then(() => {
+            throw new Error("Expected to have thrown");
+          })
+          .catch((err) => {
+            expect(err).to.be.an.instanceof(ServiceNotAvailableError);
+            expect(err.message).to.equal("Bad Gateway");
+
+            expect(p.getHostsDisabled().length).to.be.equal(0);
+          });
+      });
     });
   });
 });