From 6ac06331707173de637f0bd2fe71cd39f2ad2222 Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Fri, 6 Nov 2020 16:22:16 -0600 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Retry=20specific=20request=20error?= =?UTF-8?q?=20codes=20(#76)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/client/src/utils.js | 68 ++++++++++++++++------------- packages/client/test/client.test.js | 27 +++++++++--- packages/client/test/helper.js | 5 +++ 3 files changed, 65 insertions(+), 35 deletions(-) diff --git a/packages/client/src/utils.js b/packages/client/src/utils.js index 3f10a1a09..96fc893e2 100644 --- a/packages/client/src/utils.js +++ b/packages/client/src/utils.js @@ -97,6 +97,22 @@ export function httpAgentFor(url) { }); } +const RETRY_ERROR_CODES = [ + 'ECONNREFUSED', 'ECONNRESET', 'EPIPE', + 'EHOSTUNREACH', 'EAI_AGAIN' +]; + +// Returns true or false if an error should cause the request to be retried +function shouldRetryRequest(error) { + if (error.response) { + return error.response.status >= 500 && error.response.status < 600; + } else if (error.code) { + return RETRY_ERROR_CODES.includes(error.code); + } else { + return false; + } +} + // Returns a promise that resolves when the request is successful and rejects // when a non-successful response is received. The rejected error contains // response data and any received error details. Server 500 errors are retried @@ -107,45 +123,37 @@ export function request(url, { body, ...options }) { options = { ...options, protocol, hostname, port, path: pathname + search }; return retry((resolve, reject, retry) => { + let handleError = error => { + return shouldRetryRequest(error) + ? retry(error) : reject(error); + }; + http.request(options) .on('response', res => { let status = res.statusCode; let raw = ''; - res.setEncoding('utf8'); - res.on('data', chunk => { - raw += chunk; - }); + res.setEncoding('utf8') + .on('data', chunk => (raw += chunk)) + .on('error', handleError) + .on('end', () => { + let body = raw; + try { body = JSON.parse(raw); } catch (e) {} - res.on('end', () => { - let body = raw; - - // attempt to parse json responses - try { body = JSON.parse(raw); } catch (e) {} - - // success - if (status >= 200 && status < 300) { - resolve(body); - } else { - // error - let err = Object.assign(new Error(), { - response: { status, body }, - // use first error detail or the status message - message: body?.errors?.find(e => e.detail)?.detail || ( - `${status} ${res.statusMessage || raw}` - ) - }); - - // retry 500s - if (status >= 500) { - retry(err); + if (status >= 200 && status < 300) { + resolve(body); } else { - reject(err); + handleError(Object.assign(new Error(), { + response: { status, body }, + // use first error detail or the status message + message: body?.errors?.find(e => e.detail)?.detail || ( + `${status} ${res.statusMessage || raw}` + ) + })); } - } - }); + }); }) - .on('error', reject) + .on('error', handleError) .end(body); }); } diff --git a/packages/client/test/client.test.js b/packages/client/test/client.test.js index 0e9517916..889aff719 100644 --- a/packages/client/test/client.test.js +++ b/packages/client/test/client.test.js @@ -545,16 +545,33 @@ describe('PercyClient', () => { expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(4); }); - it('fails retrying after 5 attempts', async () => { - mockAPI.reply('/snapshots/123/finalize', () => [502, { success: false }]); - await expect(client.finalizeSnapshot(123)).rejects.toThrow('502 {"success":false}'); - expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(5); + it('retries certain request errors', async () => { + mockAPI.cleanAll().nock.persist(false) + .post('/snapshots/123/finalize').replyWithError({ code: 'ECONNREFUSED' }) + .post('/snapshots/123/finalize').replyWithError({ code: 'EHOSTUNREACH' }) + .post('/snapshots/123/finalize').replyWithError({ code: 'ECONNRESET' }) + .post('/snapshots/123/finalize').replyWithError({ code: 'EAI_AGAIN' }) + .post('/snapshots/123/finalize').reply(200, { success: true }); + + await expect(client.finalizeSnapshot(123)).resolves.toEqual({ success: true }); + expect(mockAPI.nock.isDone()).toBe(true); }); - it('does not retry request errors', async () => { + it('does not retry bad requests or unknown errors', async () => { mockAPI.reply('/snapshots/123/finalize', () => [400, { errors: [{ detail: 'Wrong' }] }]); await expect(client.finalizeSnapshot(123)).rejects.toThrow('Wrong'); expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(1); + + mockAPI.cleanAll().nock.persist(false) + .post('/snapshots/123/finalize').replyWithError(new Error('Unknown')); + await expect(client.finalizeSnapshot(123)).rejects.toThrow('Unknown'); + expect(mockAPI.nock.isDone()).toBe(true); + }); + + it('fails retrying after 5 attempts', async () => { + mockAPI.reply('/snapshots/123/finalize', () => [502, { success: false }]); + await expect(client.finalizeSnapshot(123)).rejects.toThrow('502 {"success":false}'); + expect(mockAPI.requests['/snapshots/123/finalize']).toHaveLength(5); }); }); diff --git a/packages/client/test/helper.js b/packages/client/test/helper.js index 5ddfabe07..df57cf8ef 100644 --- a/packages/client/test/helper.js +++ b/packages/client/test/helper.js @@ -70,6 +70,11 @@ const mockAPI = { this.replies[path] = this.replies[path] || []; this.replies[path].push(handler); return this; + }, + + cleanAll() { + nock.cleanAll(); + return this; } };