Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Add automatic retrying of 429/502/503 #199

Merged
merged 2 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/http/nodeMiddleware.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import {debug, headers, retry} from 'get-it/middleware'
import {debug, headers} from 'get-it/middleware'

import {name, version} from '../../package.json'

const middleware = [
debug({verbose: true, namespace: 'sanity:client'}),
headers({'User-Agent': `${name} ${version}`}),
retry({maxRetries: 3}),
]

export default middleware
44 changes: 42 additions & 2 deletions src/http/request.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {getIt, type Middlewares} from 'get-it'
import {jsonRequest, jsonResponse, observable, progress} from 'get-it/middleware'
import {jsonRequest, jsonResponse, observable, progress, retry} from 'get-it/middleware'
import {Observable} from 'rxjs'

import type {Any, HttpRequest, RequestOptions} from '../types'
Expand Down Expand Up @@ -27,8 +27,22 @@ const printWarnings = {
}

/** @internal */
export function defineHttpRequest(envMiddleware: Middlewares): HttpRequest {
export function defineHttpRequest(
envMiddleware: Middlewares,
{
maxRetries = 5,
retryDelay,
}: {maxRetries?: number; retryDelay?: (attemptNumber: number) => number}
): HttpRequest {
const request = getIt([
maxRetries > 0
? retry({
// eslint-disable-next-line @typescript-eslint/no-explicit-any
retryDelay: retryDelay as any, // This option is typed incorrectly in get-it.
maxRetries,
shouldRetry,
})
: {},
...envMiddleware,
printWarnings,
jsonRequest(),
Expand All @@ -46,3 +60,29 @@ export function defineHttpRequest(envMiddleware: Middlewares): HttpRequest {

return httpRequest
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
function shouldRetry(err: any, attempt: number, options: any) {
// By default `retry.shouldRetry` doesn't retry on server errors so we add our own logic.

const isSafe = options.method === 'GET' || options.method === 'HEAD'
const isQuery = options.uri.startsWith('/data/query')
const isRetriableResponse =
err.response &&
(err.response.statusCode === 429 ||
err.response.statusCode === 502 ||
err.response.statusCode === 503)

// We retry the following errors:
// - 429 means that the request was rate limited. It's a bit difficult
// to know exactly how long it makes sense to wait and/or how many
// attempts we should retry, but the backoff should alleviate the
// additional load.
// - 502/503 can occur when certain components struggle to talk to their
// upstream dependencies. This is most likely a temporary problem
// and retrying makes sense.

if ((isSafe || isQuery) && isRetriableResponse) return true

return retry.shouldRetry(err, attempt, options)
}
11 changes: 9 additions & 2 deletions src/index.browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,19 @@ export * from './SanityClient'
export * from './types'

// Set the http client to use for requests, and its environment specific middleware
const httpRequest = defineHttpRequest(envMiddleware)
const httpRequest = defineHttpRequest(envMiddleware, {maxRetries: 0})
/** @public */
export const requester = httpRequest.defaultRequester

/** @public */
export const createClient = (config: ClientConfig) => new SanityClient(httpRequest, config)
export const createClient = (config: ClientConfig) =>
new SanityClient(
defineHttpRequest(envMiddleware, {
maxRetries: config.maxRetries || 5,
retryDelay: config.retryDelay,
}),
config
)

/**
* @public
Expand Down
11 changes: 9 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,19 @@ export * from './SanityClient'
export * from './types'

// Set the http client to use for requests, and its environment specific middleware
const httpRequest = defineHttpRequest(envMiddleware)
const httpRequest = defineHttpRequest(envMiddleware, {})
/** @public */
export const requester = httpRequest.defaultRequester
Comment on lines +14 to 16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope for this PR, but it does seem like we should export defineHttpRequest instead of requester. I'm not sure what use there is for requester anymore. What do you think @rexxars?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few usages internally, but I'm sure they could be done differently: https://github.com/search?q=org%3Asanity-io+requester&type=code


/** @public */
export const createClient = (config: ClientConfig) => new SanityClient(httpRequest, config)
export const createClient = (config: ClientConfig) =>
new SanityClient(
defineHttpRequest(envMiddleware, {
maxRetries: config.maxRetries,
retryDelay: config.retryDelay,
}),
config
)

/**
* @public
Expand Down
13 changes: 13 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ export interface ClientConfig {
allowReconfigure?: boolean
timeout?: number

/** Number of retries for requests. Defaults to 5. */
maxRetries?: number

/**
* The amount of time, in milliseconds, to wait before retrying, given an attemptNumber (starting at 0).
*
* Defaults to exponential back-off, starting at 100ms, doubling for each attempt, together with random
* jitter between 0 and 100 milliseconds. More specifically the following algorithm is used:
*
* Delay = 100 * 2^attemptNumber + randomNumberBetween0and100
*/
retryDelay?: (attemptNumber: number) => number

/**
* @deprecated Don't use
*/
Expand Down
82 changes: 82 additions & 0 deletions test/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,57 @@ describe('client', async () => {
const project = await client.projects.getById('n1f7y')
expect(project).toEqual(doc)
})

test.each([429, 502, 503])('automatically retries %d', async (code) => {
const doc: Partial<SanityProject> = {
id: 'n1f7y',
displayName: 'Movies Unlimited',
studioHost: 'movies',
members: [
{
id: 'someuserid',
role: 'administrator',
isCurrentUser: true,
isRobot: false,
},
],
}
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(code, {})
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(200, doc)
const client = createClient({useProjectHostname: false, apiHost: `https://${apiHost}`})
const project = await client.projects.getById('n1f7y')
expect(project).toEqual(doc)
})

test.each([429, 502, 503])('can be configured to not retry %d', async (code) => {
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(code, {})
const client = createClient({
useProjectHostname: false,
apiHost: `https://${apiHost}`,
maxRetries: 0,
})

expect(client.projects.getById('n1f7y')).rejects.toBeDefined()
})

test.each([429, 502, 503])('eventually gives up on retrying %d', async (code) => {
for (let i = 0; i < 5; i++) {
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(code, {})
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(code, {})
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(code, {})
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(code, {})
nock(`https://${apiHost}`).get('/v1/projects/n1f7y').reply(code, {})
}

const client = createClient({
useProjectHostname: false,
apiHost: `https://${apiHost}`,
retryDelay() {
return 100
},
})
expect(client.projects.getById('n1f7y')).rejects.toBeDefined()
})
})

describe('DATASETS', () => {
Expand Down Expand Up @@ -1079,6 +1130,37 @@ describe('client', async () => {
expect(res[0].rating, 'data should match').toEqual(5)
})

test.skipIf(isEdge).each([429, 502, 503])('retries %d even if they are POST', async (code) => {
// Please dont ever do this. Just... don't.
const clause: string[] = []
const params: Record<string, string> = {}
for (let i = 1766; i <= 2016; i++) {
clause.push(`title == $beerName${i}`)
params[`beerName${i}`] = `some beer ${i}`
}

// Again, just... don't do this.
const query = `*[_type == "beer" && (${clause.join(' || ')})]`

nock(projectHost())
.filteringRequestBody(/.*/, '*')
.post('/v1/data/query/foo', '*')
.reply(code, {})

nock(projectHost())
.filteringRequestBody(/.*/, '*')
.post('/v1/data/query/foo', '*')
.reply(200, {
ms: 123,
q: query,
result: [{_id: 'njgNkngskjg', rating: 5}],
})

const res = await getClient().fetch(query, params)
expect(res.length, 'length should match').toEqual(1)
expect(res[0].rating, 'data should match').toEqual(5)
})

test.skipIf(isEdge)(
'uses POST for long queries, but puts request tag as query param',
async () => {
Expand Down