Do not retry on timeout by default (#2293)

* Do not retry on timeout by default * Stop testing on Node.js 12 No longer available on actions/setup-node * Node.js 14.x is no longer available for testing either * Fix linter issue * Update acceptance tests * Add retryOnTimeout to type defs * Linter cleanup * Drop code coverage step from Github action
elastic · Jun 28, 2024 · d614d95 · d614d95
1 parent af385f0
commit d614d95
Show file tree

Hide file tree

Showing 6 changed files with 132 additions and 146 deletions.
diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml
@@ -9,170 +9,136 @@ jobs:
 
     strategy:
       matrix:
-        node-version: [12.x, 14.x, 16.x]
+        node-version: [16.x, 18.x, 20.x]
         os: [ubuntu-latest, windows-latest, macOS-latest]
 
     steps:
-    - uses: actions/checkout@v2
+      - uses: actions/checkout@v2
 
-    - name: Use Node.js ${{ matrix.node-version }}
-      uses: actions/setup-node@v1
-      with:
-        node-version: ${{ matrix.node-version }}
+      - name: Use Node.js ${{ matrix.node-version }}
+        uses: actions/setup-node@v1
+        with:
+          node-version: ${{ matrix.node-version }}
 
-    - name: Install
-      run: |
-        npm install
+      - name: Install
+        run: |
+          npm install
 
-    - name: Lint
-      run: |
-        npm run lint
+      - name: Lint
+        run: |
+          npm run lint
 
-    - name: Unit test
-      run: |
-        npm run test:unit
+      - name: Unit test
+        run: |
+          npm run test:unit
 
-    - name: Acceptance test
-      run: |
-        npm run test:acceptance
+      - name: Acceptance test
+        run: |
+          npm run test:acceptance
 
-    - name: Type Definitions
-      run: |
-        npm run test:types
+      - name: Type Definitions
+        run: |
+          npm run test:types
 
   helpers-integration-test:
     name: Helpers integration test
     runs-on: ubuntu-latest
 
     strategy:
       matrix:
-        node-version: [12.x, 14.x, 16.x]
+        node-version: [16.x, 18.x, 20.x]
 
     steps:
-    - uses: actions/checkout@v2
+      - uses: actions/checkout@v2
 
-    - name: Configure sysctl limits
-      run: |
-        sudo swapoff -a
-        sudo sysctl -w vm.swappiness=1
-        sudo sysctl -w fs.file-max=262144
-        sudo sysctl -w vm.max_map_count=262144
+      - name: Configure sysctl limits
+        run: |
+          sudo swapoff -a
+          sudo sysctl -w vm.swappiness=1
+          sudo sysctl -w fs.file-max=262144
+          sudo sysctl -w vm.max_map_count=262144
 
-    - name: Runs Elasticsearch
-      uses: elastic/elastic-github-actions/elasticsearch@master
-      with:
-        stack-version: 7.16-SNAPSHOT
+      - name: Runs Elasticsearch
+        uses: elastic/elastic-github-actions/elasticsearch@master
+        with:
+          stack-version: 7.16-SNAPSHOT
 
-    - name: Use Node.js ${{ matrix.node-version }}
-      uses: actions/setup-node@v1
-      with:
-        node-version: ${{ matrix.node-version }}
+      - name: Use Node.js ${{ matrix.node-version }}
+        uses: actions/setup-node@v1
+        with:
+          node-version: ${{ matrix.node-version }}
 
-    - name: Install
-      run: |
-        npm install
+      - name: Install
+        run: |
+          npm install
 
-    - name: Integration test
-      run: |
-        npm run test:integration:helpers
+      - name: Integration test
+        run: |
+          npm run test:integration:helpers
 
   bundler-support:
     name: Bundler support
     runs-on: ubuntu-latest
 
     steps:
-    - uses: actions/checkout@v2
-
-    - name: Configure sysctl limits
-      run: |
-        sudo swapoff -a
-        sudo sysctl -w vm.swappiness=1
-        sudo sysctl -w fs.file-max=262144
-        sudo sysctl -w vm.max_map_count=262144
-
-    - name: Runs Elasticsearch
-      uses: elastic/elastic-github-actions/elasticsearch@master
-      with:
-        stack-version: 7.16-SNAPSHOT
-
-    - name: Use Node.js 14.x
-      uses: actions/setup-node@v1
-      with:
-        node-version: 14.x
-
-    - name: Install
-      run: |
-        npm install
-        npm install --prefix test/bundlers/parcel-test
-        npm install --prefix test/bundlers/rollup-test
-        npm install --prefix test/bundlers/webpack-test
-
-    - name: Build
-      run: |
-        npm run build --prefix test/bundlers/parcel-test
-        npm run build --prefix test/bundlers/rollup-test
-        npm run build --prefix test/bundlers/webpack-test
-
-    - name: Run bundle
-      run: |
-        npm start --prefix test/bundlers/parcel-test
-        npm start --prefix test/bundlers/rollup-test
-        npm start --prefix test/bundlers/webpack-test
+      - uses: actions/checkout@v2
+
+      - name: Configure sysctl limits
+        run: |
+          sudo swapoff -a
+          sudo sysctl -w vm.swappiness=1
+          sudo sysctl -w fs.file-max=262144
+          sudo sysctl -w vm.max_map_count=262144
+
+      - name: Runs Elasticsearch
+        uses: elastic/elastic-github-actions/elasticsearch@master
+        with:
+          stack-version: 7.16-SNAPSHOT
+
+      - name: Use Node.js 14.x
+        uses: actions/setup-node@v1
+        with:
+          node-version: 14.x
+
+      - name: Install
+        run: |
+          npm install
+          npm install --prefix test/bundlers/parcel-test
+          npm install --prefix test/bundlers/rollup-test
+          npm install --prefix test/bundlers/webpack-test
+
+      - name: Build
+        run: |
+          npm run build --prefix test/bundlers/parcel-test
+          npm run build --prefix test/bundlers/rollup-test
+          npm run build --prefix test/bundlers/webpack-test
+
+      - name: Run bundle
+        run: |
+          npm start --prefix test/bundlers/parcel-test
+          npm start --prefix test/bundlers/rollup-test
+          npm start --prefix test/bundlers/webpack-test
 
   mock-support:
     name: Mock support
     runs-on: ubuntu-latest
 
     steps:
-    - uses: actions/checkout@v2
+      - uses: actions/checkout@v2
 
-    - name: Use Node.js 14.x
-      uses: actions/setup-node@v1
-      with:
-        node-version: 14.x
+      - name: Use Node.js 14.x
+        uses: actions/setup-node@v1
+        with:
+          node-version: 14.x
 
-    - name: Install
-      run: |
-        npm install
-        npm install --prefix test/mock
+      - name: Install
+        run: |
+          npm install
+          npm install --prefix test/mock
 
-    - name: Run test
-      run: |
-        npm test --prefix test/mock
-
-  code-coverage:
-    name: Code coverage
-    runs-on: ubuntu-latest
-
-    strategy:
-      matrix:
-        node-version: [14.x]
-
-    steps:
-    - uses: actions/checkout@v2
-
-    - name: Use Node.js ${{ matrix.node-version }}
-      uses: actions/setup-node@v1
-      with:
-        node-version: ${{ matrix.node-version }}
-
-    - name: Install
-      run: |
-        npm install
-
-    - name: Code coverage report
-      run: |
-        npm run test:coverage-report
-
-    - name: Upload coverage to Codecov
-      uses: codecov/codecov-action@v1
-      with:
-        file: ./coverage.lcov
-        fail_ci_if_error: true
-
-    - name: Code coverage 100%
-      run: |
-        npm run test:coverage-100
+      - name: Run test
+        run: |
+          npm test --prefix test/mock
 
   license:
     name: License check
@@ -183,17 +149,17 @@ jobs:
         node-version: [14.x]
 
     steps:
-    - uses: actions/checkout@v2
+      - uses: actions/checkout@v2
 
-    - name: Use Node.js ${{ matrix.node-version }}
-      uses: actions/setup-node@v1
-      with:
-        node-version: ${{ matrix.node-version }}
+      - name: Use Node.js ${{ matrix.node-version }}
+        uses: actions/setup-node@v1
+        with:
+          node-version: ${{ matrix.node-version }}
 
-    - name: Install
-      run: |
-        npm install
+      - name: Install
+        run: |
+          npm install
 
-    - name: License checker
-      run: |
-        npm run license-checker
+      - name: License checker
+        run: |
+          npm run license-checker
diff --git a/lib/Transport.d.ts b/lib/Transport.d.ts
@@ -49,6 +49,7 @@ interface TransportOptions {
   serializer: Serializer;
   maxRetries: number;
   requestTimeout: number | string;
+  retryOnTimeout?: boolean;
   suggestCompression?: boolean;
   compression?: 'gzip';
   sniffInterval?: number;

diff --git a/lib/Transport.js b/lib/Transport.js
@@ -57,6 +57,7 @@ class Transport {
     this.serializer = opts.serializer
     this.maxRetries = opts.maxRetries
     this.requestTimeout = toMs(opts.requestTimeout)
+    this.retryOnTimeout = opts.retryOnTimeout != null ? opts.retryOnTimeout : false
     this.suggestCompression = opts.suggestCompression === true
     this.compression = opts.compression || false
     this.context = opts.context || null
@@ -220,6 +221,13 @@ class Transport {
           })
         }
 
+        // do not retry timeout errors by default
+        if (err.name === 'TimeoutError' && this.retryOnTimeout !== true) {
+          err.meta = result
+          this.emit('response', err, result)
+          return callback(err, result)
+        }
+
         // retry logic
         if (meta.attempts < maxRetries) {
           meta.attempts++

diff --git a/test/acceptance/events-order.test.js b/test/acceptance/events-order.test.js
@@ -88,7 +88,8 @@ test('Connection error', t => {
   const client = new Client({
     node: 'http://localhost:9200',
     Connection: MockConnectionError,
-    maxRetries: 1
+    maxRetries: 1,
+    retryOnTimeout: true
   })
 
   const order = [
@@ -124,18 +125,18 @@ test('Connection error', t => {
 })
 
 test('TimeoutError error', t => {
-  t.plan(10)
+  t.plan(8)
 
   const client = new Client({
     node: 'http://localhost:9200',
     Connection: MockConnectionTimeout,
-    maxRetries: 1
+    maxRetries: 1,
+    retryOnTimeout: true
   })
 
   const order = [
     events.SERIALIZATION,
     events.REQUEST,
-    events.REQUEST,
     events.RESPONSE
   ]
 
@@ -170,7 +171,8 @@ test('RequestAbortedError error', t => {
   const client = new Client({
     node: 'http://localhost:9200',
     Connection: MockConnectionTimeout,
-    maxRetries: 1
+    maxRetries: 1,
+    retryOnTimeout: true
   })
 
   const order = [
@@ -221,7 +223,8 @@ test('ResponseError error (no retry)', t => {
   const client = new Client({
     node: 'http://localhost:9200',
     Connection: MockConnection,
-    maxRetries: 1
+    maxRetries: 1,
+    retryOnTimeout: true
   })
 
   const order = [
@@ -373,7 +376,8 @@ test('Deserialization Error', t => {
   const client = new Client({
     node: 'http://localhost:9200',
     Connection: MockConnection,
-    maxRetries: 1
+    maxRetries: 1,
+    retryOnTimeout: true
   })
 
   const order = [
@@ -423,7 +427,11 @@ test('Socket destroyed while reading the body', t => {
   }
 
   buildServer(handler, ({ port }, server) => {
-    const client = new Client({ node: `http://localhost:${port}`, maxRetries: 1 })
+    const client = new Client({
+      node: `http://localhost:${port}`,
+      maxRetries: 1,
+      retryOnTimeout: true
+    })
 
     const order = [
       events.SERIALIZATION,