diff --git a/app/lib/request.lib.js b/app/lib/request.lib.js index 1f69c5ce47..8e8065f816 100644 --- a/app/lib/request.lib.js +++ b/app/lib/request.lib.js @@ -9,6 +9,63 @@ const { HttpsProxyAgent } = require('hpagent') const requestConfig = require('../../config/request.config.js') +/** + * Returns an object containing the default options. + * + * We want to be able to export these options so we can set specific settings that may be multiple objects deep. For + * example, if we want to set `retry.backoffLimit` then we can't simply pass `{ retry: { backoffLimit: 5 } }` to the lib + * as `additionalOptions` as this would replace the entire `retry` section. We would therefore want to get the exported + * `retry` section and add our setting to it before passing it back as `additionalOptions`. + * + * Note that we have a function here rather than defining a const as this did not allow us to override settings using + * Sinon to stub `requestConfig`; the const appeared to have its values fixed when the file was required, whereas a + * function generates its values each time it's called. + * + * @returns {Object} default options to pass to Got when making a request + */ +function defaultOptions () { + return { + // This uses the ternary operator to give either an `agent` object or an empty object, and the spread operator to + // bring the result back into the top level of the `defaultOptions` object. + ...(requestConfig.httpProxy + ? { + agent: { + https: new HttpsProxyAgent({ proxy: requestConfig.httpProxy }) + } + } + : {}), + // If we don't have this setting Got will throw its own HTTPError unless the result is 2xx or 3xx. That makes it + // impossible to see what the status code was because it doesn't get set on the response object Got provides when + // an error is thrown. With this set Got will treat a 404 in the same way it treats a 204. + throwHttpErrors: false, + // Got has a built in retry mechanism. We have found though you have to be careful with what gets retried. Our + // preference is to only retry in the event of a timeout on assumption the destination server might be busy but has + // a chance to succeed when attempted again + retry: { + // The default is also 2 retries before erroring. We specify it to make this fact visible + limit: 2, + // We ensure that the only network errors Got retries are timeout errors + errorCodes: ['ETIMEDOUT'], + // By default, Got does not retry PATCH and POST requests. As we only retry timeouts there is no risk in retrying + // our PATCH and POST requests. So, we set methods to be Got's defaults plus 'PATCH' and 'POST' + methods: ['GET', 'PATCH', 'POST', 'PUT', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE'], + // We set statusCodes as an empty array to ensure that 4xx, 5xx etc. errors are not retried + statusCodes: [] + }, + // Got states + // + // > It is a good practice to set a timeout to prevent hanging requests. By default, there is no timeout set. + timeout: { + request: requestConfig.timeout + }, + hooks: { + beforeRetry: [ + _beforeRetryHook + ] + } + } +} + /** * Make a GET request to the specified URL * @@ -104,48 +161,7 @@ function _logFailure (method, result, url, additionalOptions) { * @param {Object} additionalOptions Object of custom options */ function _requestOptions (additionalOptions) { - const defaultOptions = { - // This uses the ternary operator to give either an `agent` object or an empty object, and the spread operator to - // bring the result back into the top level of the `defaultOptions` object. - ...(requestConfig.httpProxy - ? { - agent: { - https: new HttpsProxyAgent({ proxy: requestConfig.httpProxy }) - } - } - : {}), - // If we don't have this setting Got will throw its own HTTPError unless the result is 2xx or 3xx. That makes it - // impossible to see what the status code was because it doesn't get set on the response object Got provides when - // an error is thrown. With this set Got will treat a 404 in the same way it treats a 204. - throwHttpErrors: false, - // Got has a built in retry mechanism. We have found though you have to be careful with what gets retried. Our - // preference is to only retry in the event of a timeout on assumption the destination server might be busy but has - // a chance to succeed when attempted again - retry: { - // The default is also 2 retries before erroring. We specify it to make this fact visible - limit: 2, - // We ensure that the only network errors Got retries are timeout errors - errorCodes: ['ETIMEDOUT'], - // By default, Got does not retry PATCH and POST requests. As we only retry timeouts there is no risk in retrying - // our PATCH and POST requests. So, we set methods to be Got's defaults plus 'PATCH' and 'POST' - methods: ['GET', 'PATCH', 'POST', 'PUT', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE'], - // We set statusCodes as an empty array to ensure that 4xx, 5xx etc. errors are not retried - statusCodes: [] - }, - // Got states - // - // > It is a good practice to set a timeout to prevent hanging requests. By default, there is no timeout set. - timeout: { - request: requestConfig.timeout - }, - hooks: { - beforeRetry: [ - _beforeRetryHook - ] - } - } - - return { ...defaultOptions, ...additionalOptions } + return { ...defaultOptions(), ...additionalOptions } } async function _sendRequest (method, url, additionalOptions) { @@ -178,5 +194,6 @@ async function _sendRequest (method, url, additionalOptions) { module.exports = { get, patch, - post + post, + defaultOptions: defaultOptions() } diff --git a/test/lib/request.lib.test.js b/test/lib/request.lib.test.js index 9c85adf746..a768e372b1 100644 --- a/test/lib/request.lib.test.js +++ b/test/lib/request.lib.test.js @@ -17,6 +17,22 @@ const RequestLib = require('../../app/lib/request.lib.js') describe('RequestLib', () => { const testDomain = 'http://example.com' + + // NOTE: We make the tests run much faster by setting backoffLimit to 50 in Got's retry options. The time between + // retries in Got is based on a computed value; ((2 ** (attemptCount - 1)) * 1000) + noise + // + // This means the delay increases exponentially between retries. That's great when making requests for real but it + // drastically slows down our test suite. Prior to making this change the tests for this module took an avg of 40 secs + // to finish. The backoffLimit sets an upper limit for the computed value. When applied here it brings the avg down + // to 16 secs. + // + // The behaviour doesn't change; we are still retrying requests. But now we are only waiting a maximum of 50ms between + // them. + const shortBackoffLimitRetryOptions = { + ...RequestLib.defaultOptions.retry, + backoffLimit: 50 + } + let notifierStub beforeEach(() => { @@ -156,13 +172,13 @@ describe('RequestLib', () => { describe('the result it returns', () => { it("has a 'succeeded' property marked as false", async () => { - const result = await RequestLib.get(testDomain) + const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.succeeded).to.be.false() }) it("has a 'response' property containing the error", async () => { - const result = await RequestLib.get(testDomain) + const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.response).to.exist() expect(result.response).to.be.an.error() @@ -191,13 +207,13 @@ describe('RequestLib', () => { describe('the result it returns', () => { it("has a 'succeeded' property marked as true", async () => { - const result = await RequestLib.get(testDomain) + const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.succeeded).to.be.true() }) it("has a 'response' property containing the web response", async () => { - const result = await RequestLib.get(testDomain) + const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.response).to.exist() expect(result.response.statusCode).to.equal(200) @@ -365,13 +381,13 @@ describe('RequestLib', () => { describe('the result it returns', () => { it("has a 'succeeded' property marked as false", async () => { - const result = await RequestLib.patch(testDomain) + const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.succeeded).to.be.false() }) it("has a 'response' property containing the error", async () => { - const result = await RequestLib.patch(testDomain) + const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.response).to.exist() expect(result.response).to.be.an.error() @@ -400,13 +416,13 @@ describe('RequestLib', () => { describe('the result it returns', () => { it("has a 'succeeded' property marked as true", async () => { - const result = await RequestLib.patch(testDomain) + const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.succeeded).to.be.true() }) it("has a 'response' property containing the web response", async () => { - const result = await RequestLib.patch(testDomain) + const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.response).to.exist() expect(result.response.statusCode).to.equal(200) @@ -574,13 +590,13 @@ describe('RequestLib', () => { describe('the result it returns', () => { it("has a 'succeeded' property marked as false", async () => { - const result = await RequestLib.post(testDomain) + const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.succeeded).to.be.false() }) it("has a 'response' property containing the error", async () => { - const result = await RequestLib.post(testDomain) + const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.response).to.exist() expect(result.response).to.be.an.error() @@ -609,13 +625,13 @@ describe('RequestLib', () => { describe('the result it returns', () => { it("has a 'succeeded' property marked as true", async () => { - const result = await RequestLib.post(testDomain) + const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.succeeded).to.be.true() }) it("has a 'response' property containing the web response", async () => { - const result = await RequestLib.post(testDomain) + const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions }) expect(result.response).to.exist() expect(result.response.statusCode).to.equal(200)