fix: Stop retrying by default after a request timeout (#100)

* fix: turn off retry-on-timeout by default

* chore: upgrade tap

.gitignore

@@ -108,3 +108,4 @@ dist
 package-lock.json
 
 lib
+.tap

package.json

@@ -6,11 +6,11 @@
   "types": "index.d.ts",
   "scripts": {
     "test": "npm run build && npm run lint && tap test/{unit,acceptance}/{*,**/*}.test.ts",
-    "test:unit": "npm run build && tap test/unit/{*,**/*}.test.ts",
-    "test:acceptance": "npm run build && tap test/acceptance/*.test.ts",
-    "test:coverage-100": "npm run build && tap test/{unit,acceptance}/{*,**/*}.test.ts --coverage --100",
-    "test:coverage-report": "npm run build && tap test/{unit,acceptance}/{*,**/*}.test.ts --coverage && nyc report --reporter=text-lcov > coverage.lcov",
-    "test:coverage-ui": "npm run build && tap test/{unit,acceptance}/{*,**/*}.test.ts --coverage --coverage-report=html",
+    "test:unit": "npm run build && tap test/unit/{*,**/*}.test.ts --disable-coverage",
+    "test:acceptance": "npm run build && tap test/acceptance/*.test.ts --disable-coverage",
+    "test:coverage-100": "npm run build && tap test/{unit,acceptance}/{*,**/*}.test.ts --show-full-coverage",
+    "test:coverage-report": "npm test && tap report --coverage-report=lcov",
+    "test:coverage-ui": "npm run build && tap test/{unit,acceptance}/{*,**/*}.test.ts --coverage-report=html",
     "lint": "ts-standard src",
     "lint:fix": "ts-standard --fix src",
     "license-checker": "license-checker --production --onlyAllow='MIT;Apache-2.0;Apache1.1;ISC;BSD-3-Clause;BSD-2-Clause;0BSD'",
@@ -50,7 +50,7 @@
     "proxy": "^1.0.2",
     "rimraf": "^3.0.2",
     "stoppable": "^1.1.0",
-    "tap": "^16.1.0",
+    "tap": "^19.0.0",
     "ts-node": "^10.7.0",
     "ts-standard": "^11.0.0",
     "typescript": "^4.6.4",
@@ -65,10 +65,6 @@
     "undici": "^6.12.0"
   },
   "tap": {
-    "ts": true,
-    "jsx": false,
-    "flow": false,
-    "coverage": false,
-    "check-coverage": false
+    "allow-incomplete-coverage": true
   }
 }

src/Transport.ts

@@ -56,6 +56,7 @@ import {
   kSniffOnConnectionFault,
   kSniffEndpoint,
   kRequestTimeout,
+  kRetryOnTimeout,
   kCompression,
   kMaxRetries,
   kName,
@@ -91,6 +92,7 @@ export interface TransportOptions {
   serializer?: Serializer
   maxRetries?: number
   requestTimeout?: number | string
+  retryOnTimeout?: boolean
   compression?: boolean
   sniffInterval?: number | boolean
   sniffOnConnectionFault?: boolean
@@ -125,6 +127,7 @@ export interface TransportRequestParams {
 export interface TransportRequestOptions {
   ignore?: number[]
   requestTimeout?: number | string
+  retryOnTimeout?: boolean
   maxRetries?: number
   asStream?: boolean
   headers?: http.IncomingHttpHeaders
@@ -199,6 +202,7 @@ export default class Transport {
   [kMaxRetries]: number
   [kCompression]: boolean
   [kRequestTimeout]: number
+  [kRetryOnTimeout]: boolean
   [kSniffEnabled]: boolean
   [kNextSniff]: number
   [kIsSniffing]: boolean
@@ -259,6 +263,7 @@ export default class Transport {
     this[kMaxRetries] = typeof opts.maxRetries === 'number' ? opts.maxRetries : 3
     this[kCompression] = opts.compression === true
     this[kRequestTimeout] = opts.requestTimeout != null ? toMs(opts.requestTimeout) : 30000
+    this[kRetryOnTimeout] = opts.retryOnTimeout != null ? opts.retryOnTimeout : false
     this[kSniffInterval] = opts.sniffInterval ?? false
     this[kSniffEnabled] = typeof this[kSniffInterval] === 'number'
     this[kNextSniff] = this[kSniffEnabled] ? (Date.now() + (this[kSniffInterval] as number)) : 0
@@ -575,8 +580,16 @@ export default class Transport {
             this[kDiagnostic].emit('response', wrappedError, result)
             throw wrappedError
           }
-          // should retry
+          // should maybe retry
+          // @ts-expect-error `case` fallthrough is intentional: should retry if retryOnTimeout is true
           case 'TimeoutError':
+            if (!this[kRetryOnTimeout]) {
+              const wrappedError = new TimeoutError(error.message, result, errorOptions)
+              this[kDiagnostic].emit('response', wrappedError, result)
+              throw wrappedError
+            }
+          // should retry
+          // eslint-disable-next-line no-fallthrough
           case 'ConnectionError': {
             // if there is an error in the connection
             // let's mark the connection as dead

src/symbols.ts

@@ -24,6 +24,7 @@ export const kSniffInterval = Symbol('sniff interval')
 export const kSniffOnConnectionFault = Symbol('sniff on connection fault')
 export const kSniffEndpoint = Symbol('sniff endpoint')
 export const kRequestTimeout = Symbol('request timeout')
+export const kRetryOnTimeout = Symbol('retry on timeout')
 export const kCompression = Symbol('compression')
 export const kMaxRetries = Symbol('max retries')
 export const kName = Symbol('name')

test/acceptance/events-order.test.ts

@@ -97,7 +97,7 @@ function runWithConnection (name: string, Connection: typeof HttpConnection | ty
       const client = new TestClient({
         node: 'http://foo.bar',
         Connection,
-        maxRetries: 1
+        maxRetries: 1,
       })
 
       const order = [
@@ -152,7 +152,8 @@ function runWithConnection (name: string, Connection: typeof HttpConnection | ty
         node: `http://localhost:${port}`,
         Connection,
         maxRetries: 1,
-        requestTimeout: 50
+        requestTimeout: 50,
+        retryOnTimeout: true,
       })
 
       const order = [
@@ -208,7 +209,7 @@ function runWithConnection (name: string, Connection: typeof HttpConnection | ty
         node: `http://localhost:${port}`,
         Connection,
         maxRetries: 1,
-        requestTimeout: 50
+        requestTimeout: 50,
       })
 
       const order = [

test/acceptance/observability.test.ts

@@ -115,7 +115,8 @@ test('Request id', t => {
         nodes: ['http://localhost:9200', 'http://localhost:9201'],
         Connection: MockConnectionSniff,
         sniffOnConnectionFault: true,
-        maxRetries: 0
+        maxRetries: 0,
+        retryOnTimeout: true
       })
 
       client.diagnostic.on(events.REQUEST, (e, event) => {
@@ -349,7 +350,8 @@ test('Client name', t => {
         nodes: ['http://localhost:9200', 'http://localhost:9201'],
         Connection: MockConnectionSniff,
         sniffOnConnectionFault: true,
-        maxRetries: 0
+        maxRetries: 0,
+        retryOnTimeout: true
       })
 
       client.diagnostic.on(events.REQUEST, (e, event) => {

test/unit/transport.test.ts

@@ -108,7 +108,7 @@ test('Basic error (TimeoutError)', async t => {
   const pool = new MyPool({ Connection: MockConnectionTimeout })
   pool.addConnection('http://localhost:9200')
 
-  const transport = new Transport({ connectionPool: pool, maxRetries: 0 })
+  const transport = new Transport({ connectionPool: pool, maxRetries: 0, retryOnTimeout: true })
 
   try {
     await transport.request({
@@ -620,6 +620,25 @@ test('Should not retry if the bulkBody is a stream', async t => {
   }
 })
 
+test('Should not retry on timeout error by default (retryOnTimeout is false)', async t => {
+  t.plan(2)
+
+  const pool = new WeightedConnectionPool({ Connection: MockConnectionTimeout })
+  pool.addConnection('http://localhost:9200')
+
+  const transport = new Transport({ connectionPool: pool })
+
+  try {
+    await transport.request({
+      method: 'GET',
+      path: '/hello'
+    })
+  } catch (err: any) {
+    t.ok(err instanceof TimeoutError)
+    t.equal(err.meta.meta.attempts, 0)
+  }
+})
+
 test('Disable maxRetries locally', async t => {
   let count = 0
   const Conn = buildMockConnection({
@@ -703,13 +722,13 @@ test('Retry on connection error', async t => {
   }
 })
 
-test('Retry on timeout error', async t => {
+test('Retry on timeout error if retryOnTimeout is true', async t => {
   t.plan(2)
 
   const pool = new WeightedConnectionPool({ Connection: MockConnectionTimeout })
   pool.addConnection('http://localhost:9200')
 
-  const transport = new Transport({ connectionPool: pool })
+  const transport = new Transport({ connectionPool: pool, retryOnTimeout: true })
 
   try {
     await transport.request({
@@ -1511,7 +1530,7 @@ test('Calls the sniff method on connection error', async t => {
   }
 })
 
-test('Calls the sniff method on timeout error', async t => {
+test('Calls the sniff method on timeout error if retryOnTimeout is true', async t => {
   t.plan(6)
 
   class MyTransport extends Transport {
@@ -1524,7 +1543,8 @@ test('Calls the sniff method on timeout error', async t => {
 
   const transport = new MyTransport({
     connectionPool: pool,
-    sniffOnConnectionFault: true
+    sniffOnConnectionFault: true,
+    retryOnTimeout: true
   })
 
   try {

test/utils/TestClient.ts

@@ -80,6 +80,7 @@ export default class TestClient {
       ConnectionPool: opts.cloud ? CloudConnectionPool : ClusterConnectionPool,
       maxRetries: 3,
       requestTimeout: 30000,
+      retryOnTimeout: false,
       pingTimeout: 3000,
       sniffInterval: false,
       sniffOnStart: false,
@@ -98,7 +99,7 @@ export default class TestClient {
       opaqueIdPrefix: null,
       context: null,
       proxy: null,
-      enableMetaHeader: true
+      enableMetaHeader: true,
     }, opts)
 
     this.name = options.name
@@ -124,6 +125,7 @@ export default class TestClient {
       serializer: this.serializer,
       maxRetries: options.maxRetries,
       requestTimeout: options.requestTimeout,
+      retryOnTimeout: options.retryOnTimeout,
       sniffInterval: options.sniffInterval,
       sniffOnStart: options.sniffOnStart,
       sniffOnConnectionFault: options.sniffOnConnectionFault,