diff --git a/README.MD b/README.MD index 6e7b5f6..a1c5d07 100644 --- a/README.MD +++ b/README.MD @@ -194,6 +194,8 @@ Handlers have a signature with two items: Every error carries the request that failed as `e.request` (`{ method, url }`), and HTTP failures also carry `e.status` and the parsed response as `e.body`. Errors thrown by `fetch` itself, such as network failures or aborts, have `e.request` but no `e.status`. +If the connection drops while a successful response body is downloading, the request fails with an error that reaches the `default` handler, with the original error on `e.cause`. Invalid JSON still resolves `undefined`, as before. + ctx can be whatever you want really - it is whatever you pass in as `context(...)`. However, if the `context` object you pass in has a `fetch` function, this is used as the `fetch` for XHR requests. @@ -289,7 +291,7 @@ Api.configure({ retry: false }) // never retry Good to know: - Timeouts and HTTP error responses (4xx, 5xx) aren't retried. -- Retries only apply server-side, as browsers don't expose network error codes. +- Retries only apply server-side, as browsers don't expose network error codes. The exception is a response body lost mid-download, which is retried in both environments, but only for GET, HEAD and OPTIONS. - Each retry logs a warning, e.g. `Got ECONNRESET when calling https://…/api/v1/apps. Retrying request (1/1)`. ## Parsing error payloads diff --git a/lib/index.js b/lib/index.js index 7ba8cd9..4067906 100644 --- a/lib/index.js +++ b/lib/index.js @@ -64,6 +64,17 @@ class BadDataError extends HttpError {} /** 429 Too Many Requests error */ class TooManyRequestsError extends HttpError {} +/** Response body lost mid-download */ +class BodyReadError extends HttpError { + /** + * @param {Error} cause + */ + constructor (cause) { + super(cause.message) + this.cause = cause + } +} + /** * Map of HTTP status codes to error classes * @type {Record} @@ -96,6 +107,8 @@ function getErrorByCode (code) { return errorMapping[code] || HttpError } +const SAFE_METHODS = [ 'GET', 'HEAD', 'OPTIONS' ] + /** * @param {ApiOptions['retry']} retry * @returns {{ attempts: number, errors: string[], methods: string[] }} @@ -108,7 +121,7 @@ function retrySettings (retry) { const { attempts = 1, errors = [ 'ECONNRESET' ], - methods = [ 'GET', 'HEAD', 'OPTIONS' ] + methods = SAFE_METHODS } = retry || {} return { attempts, errors, methods } @@ -313,6 +326,9 @@ class Api { try { json = await r.json() } catch (e) { + if (e instanceof TypeError) { + throw new BodyReadError(e) + } // @ts-ignore console.error('Unable to parse response json', e.message) } @@ -336,11 +352,14 @@ class Api { const code = e.code || (e.cause && e.cause.code) const retriesCount = attempt - 1 const hasAttemptsLeft = retriesCount < retry.attempts - const isRetryableError = retry.errors.includes(code) - const isRetryableMethod = retry.methods.includes(options.method) + const isBodyReadError = e instanceof BodyReadError + const isRetryableError = isBodyReadError || retry.errors.includes(code) + const isRetryableMethod = retry.methods.includes(options.method) && + (!isBodyReadError || SAFE_METHODS.includes(options.method)) if (hasAttemptsLeft && isRetryableError && isRetryableMethod) { - console.warn(`Got ${code} when calling ${endpoint}. Retrying request (${attempt}/${retry.attempts})`) + const reason = isBodyReadError ? `Lost response body (${e.message})` : `Got ${code}` + console.warn(`${reason} when calling ${endpoint}. Retrying request (${attempt}/${retry.attempts})`) return this.#doQuery(++attempt, client, endpoint, options) } diff --git a/lib/index.spec.js b/lib/index.spec.js index 8abe621..b8102de 100644 --- a/lib/index.spec.js +++ b/lib/index.spec.js @@ -20,6 +20,10 @@ function fetchFailed (code) { return new TypeError('fetch failed', { cause }) } +function bodyFailed (error) { + return response({ json: stub().rejects(error) }) +} + function errorWithCode (code) { const error = new Error(`read ${code}`) error.code = code @@ -237,7 +241,7 @@ describe('util/api', () => { it('logs through the context logger before the default handler runs', async () => { const calls = [] - const logger = { error: (details, message) => calls.push(['log', details, message]) } + const logger = { error: (details, message) => calls.push([ 'log', details, message ]) } const api = createApi() @@ -246,7 +250,7 @@ describe('util/api', () => { await api .context({ fetch: clientStub, logger }) .endpoint('some/url') - .default(() => calls.push(['handler'])) + .default(() => calls.push([ 'handler' ])) .get() expect(calls.map(c => c[0])).to.equal([ 'log', 'handler' ]) @@ -294,6 +298,22 @@ describe('util/api', () => { }) }) + context('body read failure', function () { + beforeEach(async function () { + const clientStub = stub().resolves(bodyFailed(new TypeError('Failed to fetch'))) + + await createApi({ retry: false }) + .context({ fetch: clientStub }) + .endpoint('some/url') + .default(e => { this.received = e }) + .get() + }) + + it('passes the dropped connection to the default handler as the cause', function () { + expect(this.received.cause).to.be.an.error(TypeError, 'Failed to fetch') + }) + }) + context('responds ok', () => { let api let clientStub @@ -768,6 +788,88 @@ describe('util/api', () => { }) }) + context('body read failure retry', function () { + beforeEach(function () { + this.warn = stub(console, 'warn') + this.clientStub = stub() + this.clientStub.onSecondCall().resolves(response({ body: { foo: 'bar' } })) + }) + + afterEach(function () { + this.warn.restore() + }) + + context('GET with a lost body', function () { + beforeEach(async function () { + this.clientStub.onFirstCall().resolves(bodyFailed(new TypeError('Failed to fetch'))) + + this.res = await createApi() + .context({ fetch: this.clientStub }) + .endpoint('some/url') + .get(({ foo }) => foo) + }) + + it('retries once', function () { + expect(this.clientStub.callCount).to.equal(2) + }) + + it('returns the retried value', function () { + expect(this.res).to.equal('bar') + }) + + it('warns with the cause message', function () { + expect(this.warn.firstCall.args[0]).to.equal('Lost response body (Failed to fetch) when calling https://example.com/api/v1/some/url. Retrying request (1/1)') + }) + }) + + context('GET with a body that is not JSON', function () { + beforeEach(async function () { + this.clientStub.onFirstCall().resolves(bodyFailed(new SyntaxError('Unexpected token <'))) + + await createApi() + .context({ fetch: this.clientStub }) + .endpoint('some/url') + .get() + }) + + it('does not retry', function () { + expect(this.clientStub.callCount).to.equal(1) + }) + }) + + context('POST', function () { + beforeEach(async function () { + this.clientStub.onFirstCall().resolves(bodyFailed(new TypeError('Failed to fetch'))) + + await createApi() + .context({ fetch: this.clientStub }) + .endpoint('some/url') + .default(() => {}) + .post() + }) + + it('does not retry', function () { + expect(this.clientStub.callCount).to.equal(1) + }) + }) + + context('POST with methods configured', function () { + beforeEach(async function () { + this.clientStub.onFirstCall().resolves(bodyFailed(new TypeError('Failed to fetch'))) + + await createApi({ retry: { methods: [ 'POST' ] } }) + .context({ fetch: this.clientStub }) + .endpoint('some/url') + .default(() => {}) + .post() + }) + + it('does not retry', function () { + expect(this.clientStub.callCount).to.equal(1) + }) + }) + }) + context('error thrown in success handler is not caught', () => { const clientStub = stub() const defaultErrorHandler = stub() diff --git a/lib/types.ts b/lib/types.ts index 47b24eb..eec4459 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -24,7 +24,7 @@ export type ApiOptions = { export type RetryOptions = { /** Number of retries after the first request. 0 never retries. Defaults to 1 */ attempts?: number; - /** Node network error codes to retry on, matched on the error or its cause. Defaults to ['ECONNRESET'] */ + /** Node network error codes to retry on, matched on the error or its cause. Defaults to ['ECONNRESET']. A response body lost mid-download on GET, HEAD, OPTIONS always retries, regardless of this list */ errors?: string[]; /** HTTP methods that may be retried. Defaults to ['GET', 'HEAD', 'OPTIONS'] */ methods?: string[];