From d614d95b8d4de19faea55684cabe88791adcfe09 Mon Sep 17 00:00:00 2001 From: Josh Mock Date: Fri, 28 Jun 2024 13:28:52 -0500 Subject: [PATCH] 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 --- .github/workflows/nodejs.yml | 240 +++++++++++--------------- lib/Transport.d.ts | 1 + lib/Transport.js | 8 + test/acceptance/events-order.test.js | 24 ++- test/acceptance/product-check.test.js | 3 +- test/unit/transport.test.js | 2 + 6 files changed, 132 insertions(+), 146 deletions(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index 43181ce94..45897aec0 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -9,36 +9,36 @@ 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 @@ -46,133 +46,99 @@ jobs: 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 index 25b770fdb..6d3c06b24 100644 --- 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 index d30f6347a..f499a8783 100644 --- 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 index 335fd4ba8..c8fa20d38 100644 --- 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, diff --git a/test/acceptance/product-check.test.js b/test/acceptance/product-check.test.js index 2ed7cec88..3f788d6ea 100644 --- a/test/acceptance/product-check.test.js +++ b/test/acceptance/product-check.test.js @@ -680,7 +680,8 @@ test('TimeoutError', t => { const client = new Client({ node: 'http://localhost:9200', Connection: MockConnectionTimeout, - maxRetries: 0 + maxRetries: 0, + retryOnTimeout: true }) client.on('request', (err, event) => { diff --git a/test/unit/transport.test.js b/test/unit/transport.test.js index e1a6aaa9f..5f183a2e9 100644 --- a/test/unit/transport.test.js +++ b/test/unit/transport.test.js @@ -1025,6 +1025,7 @@ test('Retry mechanism and abort', t => { serializer: new Serializer(), maxRetries: 2, requestTimeout: 100, + retryOnTimeout: true, sniffInterval: false, sniffOnStart: false }) @@ -2203,6 +2204,7 @@ test('Compress request', t => { serializer: new Serializer(), maxRetries: 3, requestTimeout: 250, + retryOnTimeout: true, sniffInterval: false, sniffOnStart: false })