Skip to content

Commit

Permalink
feat: catch more http errors (#5)
Browse files Browse the repository at this point in the history 
BREAKING CHANGE: Now retrying for 4xx & 5xx except 400, 401, 403 and 404
  • Loading branch information
@SGrondin @gr2m
SGrondin authored and gr2m committed Jan 1, 2019
1 parent 14352e3 commit 0b0360b
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 42 deletions.
14 changes: 12 additions & 2 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 => {
Expand Down
13 changes: 5 additions & 8 deletions lib/error-request.js
View file
Original file line number Diff line number Diff line change
@@ -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
}
13 changes: 7 additions & 6 deletions lib/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,21 @@ 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
return error
}
}

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))
}
2 changes: 2 additions & 0 deletions lib/wrap-request.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
})
Expand Down
68 changes: 63 additions & 5 deletions test/backend-errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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)
})
})
39 changes: 19 additions & 20 deletions test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand All @@ -34,28 +34,27 @@ 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([
'START GET /route',
'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: {
Expand All @@ -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')
Expand All @@ -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,
Expand All @@ -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)
})
})
2 changes: 1 addition & 1 deletion test/octokit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 0b0360b

Please sign in to comment.