Exponential connection resurrectTimeout becomes too long after two deaths

[ukcrpb6]

🐛 Bug Report

Resurrection formula means that resurrection attempts will only occur twice (effectively).

    // resurrectTimeout formula:
    // `Math.pow(resurrectTimeout * 2, deadCount -1)`
    // we don't need to multiply the resurrectTimeout by 2
    // every time, it is cached during the initialization
    connection.resurrectTimeout = Date.now() + Math.pow(
      this.resurrectTimeout,
      Math.min(
        connection.deadCount - 1,
        this.resurrectTimeoutCutoff
      )
    )

applying these defaults (constructor)

    // the resurrect timeout is 60s
    // we multiply it by 2 because the resurrect formula is
    // `Math.pow(resurrectTimeout * 2, deadCount -1)`
    // and we don't need to multiply by 2
    // the resurrectTimeout every time
    this.resurrectTimeout = 1000 * 60 * 2
    // number of consecutive failures after which
    // the timeout doesn't increase
    this.resurrectTimeoutCutoff = 5    

yields only 2 attempts within a reasonable timeframe

deadCount - 1	MS	M	H	D	Y
0	1	-	-	-	-
1	120000	2	-	-	-
2	14400000000	240000	4000	166.6666667	-
3	1.728E+15	28800000000	480000000	20000000	54794.52055
4	2.0736E+20	3.456E+15	57600000000000	2400000000000	6575342466
5 (cutoff)	2.48832E+25	4.1472E+20	6.912E+18	2.88E+17	789041095890411

To Reproduce

Steps to reproduce the behavior:

const { Client, Connection } = require('@elastic/elasticsearch')

class FakeConnection extends Connection {
  set resurrectTimeout(timeout) {
    console.log(`deadCount is ${this.deadCount} next resurrect timeout is at ${new Date(timeout).toISOString()}`);
    this._resurrectTimeout = timeout;
  }

  get resurrectTimeout() {
    return this._resurrectTimeout;
  }

  request(_, cb) {
    cb(new Error('so long and thanks for all the fish'), null);
  }
}

const client = new Client({
  Connection: FakeConnection,
  nodes: ['http://localhost:12345', 'http://localhost:12346'], // multiple nodes needed or else markDead is ignored
});


(async () => {
  const connection = client.connectionPool.getConnection();
  client.connectionPool.markDead(connection);
  for (let index = 0; index < 10; index++) {
    await client.connectionPool.resurrect(connection.resurrectTimeout);
  }
}
)();

// deadCount is 0 next resurrect timeout is at 1970-01-01T00:00:00.000Z
// deadCount is 0 next resurrect timeout is at 1970-01-01T00:00:00.000Z
// deadCount is 1 next resurrect timeout is at 2019-04-24T12:21:43.669Z
// deadCount is 2 next resurrect timeout is at 2019-04-24T12:23:43.668Z
// deadCount is 3 next resurrect timeout is at 2019-10-08T04:21:43.669Z
// deadCount is 4 next resurrect timeout is at +056777-06-14T12:21:43.669Z
// (node:24034) UnhandledPromiseRejectionWarning: RangeError: Invalid time value       << deadCount is 5

Expected behavior

Resurrect timeout should be within normal expected range for exponential backoff.

Your Environment

• node version: 10
• @elastic/elasticsearch version: >=6.7.0-rc2
• os: Mac

[ukcrpb6]

I suspect this should be something like

    connection.resurrectTimeout = Date.now() + this.resurrectTimeout * Math.pow(
      2,
      Math.min(
        connection.deadCount - 1,
        this.resurrectTimeoutCutoff
      )
    );

and the initial value should be this.resurrectTimeout = 1000 * 60 giving delays of

deadCount - 1	MS	M
0	60000	1
1	120000	2
2	240000	4
3	480000	8
4	960000	16
5 (cutoff)	1920000	32

[delvedor]

Hello, good catch!!
Do you want to open a pull request? :)