From 0b0360b8b6a67ed74a02d872d91bcf12f84d435f Mon Sep 17 00:00:00 2001 From: Simon Grondin Date: Tue, 1 Jan 2019 12:06:32 -0600 Subject: [PATCH] feat: catch more http errors (#5) BREAKING CHANGE: Now retrying for 4xx & 5xx except 400, 401, 403 and 404 --- README.md | 14 +++++++-- lib/error-request.js | 13 ++++---- lib/index.js | 13 ++++---- lib/wrap-request.js | 2 ++ test/backend-errors.js | 68 ++++++++++++++++++++++++++++++++++++++---- test/index.js | 39 ++++++++++++------------ test/octokit.js | 2 +- 7 files changed, 109 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 576395dd..4219dfd0 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ [![Coverage Status](https://img.shields.io/coveralls/github/octokit/plugin-retry.js.svg)](https://coveralls.io/github/octokit/plugin-retry.js) [![Greenkeeper](https://badges.greenkeeper.io/octokit/plugin-retry.js.svg)](https://greenkeeper.io/) -Implements request retries for server error responses. +Implements request retries for server 4xx/5xx responses except `400`, `401`, `403` and `404`. ## Usage @@ -25,7 +25,17 @@ octokit.request('/').catch(error => { }) ``` -You can ask for retries for any request by passing `{ request: { retries: numRetries, retryAfter: delayInSeconds }}` +To override the default `doNotRetry` list: + +```js +const octokit = new Octokit({ + retry: { + doNotRetry: [ /* List of HTTP 4xx/5xx status codes */ ] + } +}) +``` + +You can manually ask for retries for any request by passing `{ request: { retries: numRetries, retryAfter: delayInSeconds }}` ```js octokit.request('/', { request: { retries: 1, retryAfter: 1 } }).catch(error => { diff --git a/lib/error-request.js b/lib/error-request.js index 7a8b1bed..6af33cd1 100644 --- a/lib/error-request.js +++ b/lib/error-request.js @@ -1,17 +1,14 @@ module.exports = errorRequest -async function errorRequest (octokit, error, options) { - if (error.status === 500) { - const retries = 3 +async function errorRequest (octokit, state, error, options) { + // retry all >= 400 && not doNotRetry + if (error.status >= 400 && !state.doNotRetry.includes(error.status)) { + const retries = options.request.retries != null ? options.request.retries : state.retries const retryAfter = Math.pow((options.request.retryCount || 0) + 1, 2) throw octokit.retry.retryRequest(error, retries, retryAfter) } - /* - TODO: - Add more cases here. - Use the 500 error above as example. - */ + // Maybe eventually there will be more cases here throw error } diff --git a/lib/index.js b/lib/index.js index b6c94760..560221b7 100644 --- a/lib/index.js +++ b/lib/index.js @@ -3,13 +3,14 @@ module.exports = retryPlugin const wrapRequest = require('./wrap-request') const errorRequest = require('./error-request') -function retryPlugin (octokit) { - const state = { - retryAfterBaseValue: 1000 - } +function retryPlugin (octokit, octokitOptions = {}) { + const state = Object.assign({ + retryAfterBaseValue: 1000, + doNotRetry: [ 400, 401, 403, 404 ], + retries: 3 + }, octokitOptions.retry) octokit.retry = { - _options: (options = {}) => Object.assign(state, options), retryRequest: (error, retries, retryAfter) => { error.request.request.retries = retries error.request.request.retryAfter = retryAfter @@ -17,6 +18,6 @@ function retryPlugin (octokit) { } } - octokit.hook.error('request', errorRequest.bind(null, octokit)) + octokit.hook.error('request', errorRequest.bind(null, octokit, state)) octokit.hook.wrap('request', wrapRequest.bind(null, state)) } diff --git a/lib/wrap-request.js b/lib/wrap-request.js index 5c62451c..aefa773f 100644 --- a/lib/wrap-request.js +++ b/lib/wrap-request.js @@ -11,6 +11,8 @@ async function wrapRequest (state, request, options) { options.request.retryCount = info.retryCount + 1 if (maxRetries > info.retryCount) { + // Returning a number instructs the limiter to retry + // the request after that number of milliseconds have passed return after * state.retryAfterBaseValue } }) diff --git a/test/backend-errors.js b/test/backend-errors.js index 86d5cc8d..0f319f19 100644 --- a/test/backend-errors.js +++ b/test/backend-errors.js @@ -3,8 +3,7 @@ const Octokit = require('./octokit') describe('Trigger Retries', function () { it('Should trigger exponential retries on HTTP 500 errors', async function () { - const octokit = new Octokit() - octokit.retry._options({ retryAfterBaseValue: 50 }) + const octokit = new Octokit({ retry: { retryAfterBaseValue: 50 } }) const res = await octokit.request('GET /route', { request: { @@ -26,8 +25,67 @@ describe('Trigger Retries', function () { 'START GET /route', 'END GET /route' ]) - expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(50, 15) - expect(octokit.__requestTimings[2] - octokit.__requestTimings[1]).to.be.closeTo(200, 15) - expect(octokit.__requestTimings[3] - octokit.__requestTimings[2]).to.be.closeTo(450, 15) + expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(50, 20) + expect(octokit.__requestTimings[2] - octokit.__requestTimings[1]).to.be.closeTo(200, 20) + expect(octokit.__requestTimings[3] - octokit.__requestTimings[2]).to.be.closeTo(450, 20) + }) + + it('Should not retry 3xx/400/401/403 errors', async function () { + const octokit = new Octokit({ retry: { retryAfterBaseValue: 50 } }) + let caught = 0 + const testStatuses = [ 304, 400, 401, 403, 404 ] + + for (const status of testStatuses) { + try { + await octokit.request('GET /route', { + request: { + responses: [ + { status, headers: {}, data: { message: `Error ${status}` } }, + { status: 500, headers: {}, data: { message: 'Error 500' } } + ] + } + }) + } catch (error) { + expect(error.message).to.equal(`Error ${status}`) + caught++ + } + } + + expect(caught).to.equal(testStatuses.length) + expect(octokit.__requestLog).to.deep.equal(testStatuses.map(x => 'START GET /route')) + }) + + it('Should allow to override the doNotRetry list', async function () { + const octokit = new Octokit({ + retry: { + doNotRetry: [ 400 ], + retries: 1, + retryAfterBaseValue: 50 + } + }) + let caught = 0 + const testStatuses = [ 304, 400, 401, 403, 404 ] + + for (const status of testStatuses) { + try { + await octokit.request('GET /route', { + request: { + responses: [ + { status, headers: {}, data: { message: `Error ${status}` } }, + { status: 500, headers: {}, data: { message: 'Error 500' } } + ] + } + }) + } catch (error) { + if (status === 400 || status < 400) { + expect(error.message).to.equal(`Error ${status}`) + } else { + expect(error.message).to.equal(`Error 500`) + } + caught++ + } + } + + expect(caught).to.equal(testStatuses.length) }) }) diff --git a/test/index.js b/test/index.js index 797a7b27..5a7e52b3 100644 --- a/test/index.js +++ b/test/index.js @@ -22,7 +22,7 @@ describe('Automatic Retries', function () { 'START GET /route', 'END GET /route' ]) - expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(0, 15) + expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(0, 20) }) it('Should retry twice and fail', async function () { @@ -34,14 +34,14 @@ describe('Automatic Retries', function () { responses: [ { status: 403, headers: {}, data: { message: 'ONE' } }, { status: 403, headers: {}, data: { message: 'TWO' } }, - { status: 404, headers: {}, data: { message: 'THREE' } } + { status: 401, headers: {}, data: { message: 'THREE' } } ], retries: 2 } }) throw new Error('Should not reach this point') } catch (error) { - expect(error.status).to.equal(404) + expect(error.status).to.equal(401) expect(error.message).to.equal('THREE') } expect(octokit.__requestLog).to.deep.equal([ @@ -49,13 +49,12 @@ describe('Automatic Retries', function () { 'START GET /route', 'START GET /route' ]) - expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(0, 15) - expect(octokit.__requestTimings[2] - octokit.__requestTimings[1]).to.be.closeTo(0, 15) + expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(0, 20) + expect(octokit.__requestTimings[2] - octokit.__requestTimings[1]).to.be.closeTo(0, 20) }) it('Should retry after 2000ms', async function () { - const octokit = new Octokit() - octokit.retry._options({ retryAfterBaseValue: 50 }) + const octokit = new Octokit({ retry: { retryAfterBaseValue: 50 } }) const res = await octokit.request('GET /route', { request: { @@ -76,22 +75,22 @@ describe('Automatic Retries', function () { 'END GET /route' ]) // 50ms * 2 === 100ms - expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(100, 15) + expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(100, 20) }) it('Should allow end users to see the number of retries after a failure', async function () { - const octokit = new Octokit() - octokit.retry._options({ retryAfterBaseValue: 25 }) + const octokit = new Octokit({ retry: { retryAfterBaseValue: 25 } }) try { await octokit.request('GET /route', { request: { responses: [ - { status: 500, headers: {}, data: { message: 'Failed, one' } }, - { status: 500, headers: {}, data: { message: 'Failed, two' } }, - { status: 500, headers: {}, data: { message: 'Failed, three' } }, - { status: 500, headers: {}, data: { message: 'Failed, four' } } - ] + { status: 403, headers: {}, data: { message: 'Failed, one' } }, + { status: 403, headers: {}, data: { message: 'Failed, two' } }, + { status: 403, headers: {}, data: { message: 'Failed, three' } }, + { status: 403, headers: {}, data: { message: 'Failed, four' } } + ], + retries: 3 } }) throw new Error('Should not reach this point') @@ -100,17 +99,17 @@ describe('Automatic Retries', function () { expect(error.request.request.retryCount).to.equal(4) } - expect(octokit.__requestTimings[3] - octokit.__requestTimings[2]).to.be.closeTo(225, 15) + // null (0) retryAfter + expect(octokit.__requestTimings[3] - octokit.__requestTimings[2]).to.be.closeTo(0, 20) }) it('Should allow end users to request retries', async function () { - const octokit = new Octokit() - octokit.retry._options({ retryAfterBaseValue: 25 }) + const octokit = new Octokit({ retry: { retryAfterBaseValue: 25 } }) const res = await octokit.request('GET /route', { request: { responses: [ - { status: 400, headers: {}, data: { message: 'Did not retry' } }, + { status: 403, headers: {}, data: { message: 'Did not retry' } }, { status: 202, headers: {}, data: { message: 'Yay!' } } ], retries: 1, @@ -125,6 +124,6 @@ describe('Automatic Retries', function () { 'START GET /route', 'END GET /route' ]) - expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(25, 15) + expect(octokit.__requestTimings[1] - octokit.__requestTimings[0]).to.be.closeTo(25, 20) }) }) diff --git a/test/octokit.js b/test/octokit.js index 680ca5b8..5b66e14f 100644 --- a/test/octokit.js +++ b/test/octokit.js @@ -14,7 +14,7 @@ module.exports = Octokit await new Promise(resolve => setTimeout(resolve, 0)) const res = options.request.responses.shift() - if (res.status >= 400) { + if (res.status >= 300) { const message = res.data.message != null ? res.data.message : `Test failed request (${res.status})` const error = new HttpError(message, res.status, res.headers, options) throw error