diff --git a/packages/datafile-manager/CHANGELOG.md b/packages/datafile-manager/CHANGELOG.md index a7a9c5cbd..a4179e9d3 100644 --- a/packages/datafile-manager/CHANGELOG.md +++ b/packages/datafile-manager/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] Changes that have landed but are not yet released. +### New Features +- Added 60 second timeout for all requests + ### Changed - Changed value for node in engines in package.json from >=4.0.0 to >=6.0.0 - Updated polling behavior: diff --git a/packages/datafile-manager/__test__/browserRequest.spec.ts b/packages/datafile-manager/__test__/browserRequest.spec.ts index 46f6b3b26..004d93af0 100644 --- a/packages/datafile-manager/__test__/browserRequest.spec.ts +++ b/packages/datafile-manager/__test__/browserRequest.spec.ts @@ -18,19 +18,19 @@ */ import { - SinonFakeXMLHttpRequest, - SinonFakeXMLHttpRequestStatic, - useFakeXMLHttpRequest, -} from 'sinon' + FakeXMLHttpRequest, + FakeXMLHttpRequestStatic, + fakeXhr +} from 'nise' import { makeGetRequest } from '../src/browserRequest' describe('browserRequest', () => { describe('makeGetRequest', () => { - let mockXHR: SinonFakeXMLHttpRequestStatic - let xhrs: SinonFakeXMLHttpRequest[] + let mockXHR: FakeXMLHttpRequestStatic + let xhrs: FakeXMLHttpRequest[] beforeEach(() => { xhrs = [] - mockXHR = useFakeXMLHttpRequest() + mockXHR = fakeXhr.useFakeXMLHttpRequest() mockXHR.onCreate = req => xhrs.push(req) }) @@ -126,5 +126,13 @@ describe('browserRequest', () => { xhrs[0].error() await expect(req.responsePromise).rejects.toThrow() }) + + it('sets a timeout on the request object', () => { + const onCreateMock = jest.fn() + mockXHR.onCreate = onCreateMock + makeGetRequest('https://cdn.optimizely.com/datafiles/123.json', {}) + expect(onCreateMock).toBeCalledTimes(1) + expect(onCreateMock.mock.calls[0][0].timeout).toBe(60000) + }) }) }) diff --git a/packages/datafile-manager/__test__/nodeRequest.spec.ts b/packages/datafile-manager/__test__/nodeRequest.spec.ts index 7674ed50d..c9154e5fe 100644 --- a/packages/datafile-manager/__test__/nodeRequest.spec.ts +++ b/packages/datafile-manager/__test__/nodeRequest.spec.ts @@ -16,6 +16,7 @@ import nock from 'nock' import { makeGetRequest } from '../src/nodeRequest' +import { advanceTimersByTime } from './testUtils' beforeAll(() => { nock.disableNetConnect() @@ -156,5 +157,41 @@ describe('nodeEnvironment', () => { }) scope.done() }) + + describe('timeout', () => { + beforeEach(() => { + jest.useFakeTimers() + }) + + afterEach(() => { + jest.clearAllTimers() + }) + + it('rejects the response promise and aborts the request when the response is not received before the timeout', async () => { + const scope = nock(host) + .get(path) + .delay(61000) + .reply(200, '{"foo":"bar"}') + + const abortEventListener = jest.fn() + let emittedReq: any + const requestListener = (request: any) => { + emittedReq = request + emittedReq.once('abort', abortEventListener) + } + scope.on('request', requestListener) + + const req = makeGetRequest(`${host}${path}`, {}) + await advanceTimersByTime(60000) + await expect(req.responsePromise).rejects.toThrow() + expect(abortEventListener).toBeCalledTimes(1) + + scope.done() + if (emittedReq) { + emittedReq.off('abort', abortEventListener) + } + scope.off('request', requestListener) + }) + }) }) }) diff --git a/packages/datafile-manager/package.json b/packages/datafile-manager/package.json index fdf3a2181..4de7f2459 100644 --- a/packages/datafile-manager/package.json +++ b/packages/datafile-manager/package.json @@ -25,12 +25,12 @@ }, "devDependencies": { "@types/jest": "^24.0.9", + "@types/nise": "^1.4.0", "@types/nock": "^9.3.1", "@types/node": "^11.11.7", - "@types/sinon": "^7.0.10", "jest": "^24.1.0", + "nise": "^1.4.10", "nock": "^10.0.6", - "sinon": "^7.2.7", "ts-jest": "^24.0.0", "typescript": "^3.3.3333" }, diff --git a/packages/datafile-manager/src/browserRequest.ts b/packages/datafile-manager/src/browserRequest.ts index 7d9fef620..0dbbcef78 100644 --- a/packages/datafile-manager/src/browserRequest.ts +++ b/packages/datafile-manager/src/browserRequest.ts @@ -15,6 +15,10 @@ */ import { AbortableRequest, Response, Headers } from './http' +import { REQUEST_TIMEOUT_MS } from './config' +import { getLogger } from '@optimizely/js-sdk-logging' + +const logger = getLogger('DatafileManager') const GET_METHOD = 'GET' const READY_STATE_DONE = 4 @@ -74,6 +78,12 @@ export function makeGetRequest(reqUrl: string, headers: Headers): AbortableReque } } + req.timeout = REQUEST_TIMEOUT_MS + + req.ontimeout = () => { + logger.error('Request timed out') + } + req.send() }) diff --git a/packages/datafile-manager/src/config.ts b/packages/datafile-manager/src/config.ts index 6bdb6aa25..8cc9faf54 100644 --- a/packages/datafile-manager/src/config.ts +++ b/packages/datafile-manager/src/config.ts @@ -21,3 +21,5 @@ export const MIN_UPDATE_INTERVAL = 1000 export const DEFAULT_URL_TEMPLATE = `https://cdn.optimizely.com/datafiles/%s.json` export const BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT = [0, 8, 16, 32, 64, 128, 256, 512] + +export const REQUEST_TIMEOUT_MS = 60 * 1000 // 1 minute diff --git a/packages/datafile-manager/src/nodeRequest.ts b/packages/datafile-manager/src/nodeRequest.ts index ee0cfa2ff..aa56d5eeb 100644 --- a/packages/datafile-manager/src/nodeRequest.ts +++ b/packages/datafile-manager/src/nodeRequest.ts @@ -18,6 +18,7 @@ import http from 'http' import https from 'https' import url from 'url' import { Headers, AbortableRequest, Response } from './http' +import { REQUEST_TIMEOUT_MS } from './config'; // Shared signature between http.request and https.request type ClientRequestCreator = (options: http.RequestOptions) => http.ClientRequest @@ -64,6 +65,11 @@ function getResponseFromRequest(request: http.ClientRequest): Promise // TODO: When we drop support for Node 6, consider using util.promisify instead of // constructing own Promise return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + request.abort() + reject(new Error('Request timed out')) + }, REQUEST_TIMEOUT_MS) + request.once('response', (incomingMessage: http.IncomingMessage) => { if (request.aborted) { return @@ -83,6 +89,8 @@ function getResponseFromRequest(request: http.ClientRequest): Promise return } + clearTimeout(timeout) + resolve({ statusCode: incomingMessage.statusCode, body: responseData, @@ -92,6 +100,8 @@ function getResponseFromRequest(request: http.ClientRequest): Promise }) request.on('error', (err: any) => { + clearTimeout(timeout) + if (err instanceof Error) { reject(err) } else if (typeof err === 'string') { diff --git a/packages/datafile-manager/yarn.lock b/packages/datafile-manager/yarn.lock index 8f434abd1..ca80f55ed 100644 --- a/packages/datafile-manager/yarn.lock +++ b/packages/datafile-manager/yarn.lock @@ -145,14 +145,14 @@ dependencies: uuid "^3.3.2" -"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2", "@sinonjs/commons@^1.3.1": +"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2": version "1.4.0" resolved "https://registry.yarnpkg.com/@sinonjs/commons/-/commons-1.4.0.tgz#7b3ec2d96af481d7a0321252e7b1c94724ec5a78" integrity sha512-9jHK3YF/8HtJ9wCAbG+j8cD0i0+ATS9A7gXFqS36TblLPNy6rEEc+SB0imo91eCboGaBYGV/MT1/br/J+EE7Tw== dependencies: type-detect "4.0.8" -"@sinonjs/formatio@^3.1.0", "@sinonjs/formatio@^3.2.1": +"@sinonjs/formatio@^3.1.0": version "3.2.1" resolved "https://registry.yarnpkg.com/@sinonjs/formatio/-/formatio-3.2.1.tgz#52310f2f9bcbc67bdac18c94ad4901b95fde267e" integrity sha512-tsHvOB24rvyvV2+zKMmPkZ7dXX6LSLKZ7aOtXY6Edklp0uRcgGpOsQTTGTcWViFyx4uhWc6GV8QdnALbIbIdeQ== @@ -160,10 +160,10 @@ "@sinonjs/commons" "^1" "@sinonjs/samsam" "^3.1.0" -"@sinonjs/samsam@^3.1.0", "@sinonjs/samsam@^3.2.0": - version "3.3.0" - resolved "https://registry.yarnpkg.com/@sinonjs/samsam/-/samsam-3.3.0.tgz#9557ea89cd39dbc94ffbd093c8085281cac87416" - integrity sha512-beHeJM/RRAaLLsMJhsCvHK31rIqZuobfPLa/80yGH5hnD8PV1hyh9xJBJNFfNmO7yWqm+zomijHsXpI6iTQJfQ== +"@sinonjs/samsam@^3.1.0": + version "3.3.1" + resolved "https://registry.yarnpkg.com/@sinonjs/samsam/-/samsam-3.3.1.tgz#e88c53fbd9d91ad9f0f2b0140c16c7c107fe0d07" + integrity sha512-wRSfmyd81swH0hA1bxJZJ57xr22kC07a1N4zuIL47yTS04bDk6AoCkczcqHEjcRPmJ+FruGJ9WBQiJwMtIElFw== dependencies: "@sinonjs/commons" "^1.0.2" array-from "^2.1.1" @@ -186,6 +186,11 @@ dependencies: "@types/jest-diff" "*" +"@types/nise@^1.4.0": + version "1.4.0" + resolved "https://registry.yarnpkg.com/@types/nise/-/nise-1.4.0.tgz#83af8ffc6ec4c622ffdbf298491137346f482a35" + integrity sha512-DPxmjiDwubsNmguG5X4fEJ+XCyzWM3GXWsqQlvUcjJKa91IOoJUy51meDr0GkzK64qqNcq85ymLlyjoct9tInw== + "@types/nock@^9.3.1": version "9.3.1" resolved "https://registry.yarnpkg.com/@types/nock/-/nock-9.3.1.tgz#7d761a43a10aebc7ec6bae29d89afc6cbffa5d30" @@ -203,11 +208,6 @@ resolved "https://registry.yarnpkg.com/@types/node/-/node-11.11.7.tgz#f1c35a906b82adae76ede5ab0d2088e58fa37843" integrity sha512-bHbRcyD6XpXVLg42QYaQCjvDXaCFkvb3WbCIxSDmhGbJYVroxvYzekk9QGg1beeIawfvSLkdZpP0h7jxE4ihnA== -"@types/sinon@^7.0.10": - version "7.0.10" - resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-7.0.10.tgz#1f921f0c347b19f754e61dbc671c088df73fe1ff" - integrity sha512-4w7SvsiUOtd4mUfund9QROPSJ5At/GQskDpqd87pJIRI6ULWSJqHI3GIZE337wQuN3aznroJGr94+o8fwvL37Q== - abab@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/abab/-/abab-2.0.0.tgz#aba0ab4c5eee2d4c79d3487d85450fb2376ebb0f" @@ -841,11 +841,6 @@ diff-sequences@^24.0.0: resolved "https://registry.yarnpkg.com/diff-sequences/-/diff-sequences-24.0.0.tgz#cdf8e27ed20d8b8d3caccb4e0c0d8fe31a173013" integrity sha512-46OkIuVGBBnrC0soO/4LHu5LHGHx0uhP65OVz8XOrAJpqiCB2aVIuESvjI1F9oqebuvY8lekS1pt6TN7vt7qsw== -diff@^3.5.0: - version "3.5.0" - resolved "https://registry.yarnpkg.com/diff/-/diff-3.5.0.tgz#800c0dd1e0a8bfbc95835c202ad220fe317e5a12" - integrity sha512-A46qtFgd+g7pDZinpnwiRJtxbC1hpgf0uzP3iG89scHk0AUC7A1TGxf5OiiOUv/JMZR8GOt8hL900hV0bOy5xA== - domexception@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/domexception/-/domexception-1.0.1.tgz#937442644ca6a31261ef36e3ec677fe805582c90" @@ -2115,11 +2110,6 @@ lolex@^2.3.2: resolved "https://registry.yarnpkg.com/lolex/-/lolex-2.7.5.tgz#113001d56bfc7e02d56e36291cc5c413d1aa0733" integrity sha512-l9x0+1offnKKIzYVjyXU2SiwhXDLekRzKyhnbyldPHvC7BvLPVpdNUNR2KeMAiCN2D/kLNttZgQD5WjSxuBx3Q== -lolex@^3.1.0: - version "3.1.0" - resolved "https://registry.yarnpkg.com/lolex/-/lolex-3.1.0.tgz#1a7feb2fefd75b3e3a7f79f0e110d9476e294434" - integrity sha512-zFo5MgCJ0rZ7gQg69S4pqBsLURbFw11X68C18OcJjJQbqaXm2NoTrGl1IMM3TIz0/BnN1tIs2tzmmqvCsOMMjw== - loose-envify@^1.0.0: version "1.4.0" resolved "https://registry.yarnpkg.com/loose-envify/-/loose-envify-1.4.0.tgz#71ee51fa7be4caec1a63839f7e682d8132d30caf" @@ -3013,19 +3003,6 @@ signal-exit@^3.0.0, signal-exit@^3.0.2: resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.2.tgz#b5fdc08f1287ea1178628e415e25132b73646c6d" integrity sha1-tf3AjxKH6hF4Yo5BXiUTK3NkbG0= -sinon@^7.2.7: - version "7.2.7" - resolved "https://registry.yarnpkg.com/sinon/-/sinon-7.2.7.tgz#ee90f83ce87d9a6bac42cf32a3103d8c8b1bfb68" - integrity sha512-rlrre9F80pIQr3M36gOdoCEWzFAMDgHYD8+tocqOw+Zw9OZ8F84a80Ds69eZfcjnzDqqG88ulFld0oin/6rG/g== - dependencies: - "@sinonjs/commons" "^1.3.1" - "@sinonjs/formatio" "^3.2.1" - "@sinonjs/samsam" "^3.2.0" - diff "^3.5.0" - lolex "^3.1.0" - nise "^1.4.10" - supports-color "^5.5.0" - sisteransi@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/sisteransi/-/sisteransi-1.0.0.tgz#77d9622ff909080f1c19e5f4a1df0c1b0a27b88c" @@ -3239,7 +3216,7 @@ strip-json-comments@~2.0.1: resolved "https://registry.yarnpkg.com/strip-json-comments/-/strip-json-comments-2.0.1.tgz#3c531942e908c2697c0ec344858c286c7ca0a60a" integrity sha1-PFMZQukIwml8DsNEhYwobHygpgo= -supports-color@^5.3.0, supports-color@^5.5.0: +supports-color@^5.3.0: version "5.5.0" resolved "https://registry.yarnpkg.com/supports-color/-/supports-color-5.5.0.tgz#e2e69a44ac8772f78a1ec0b35b689df6530efc8f" integrity sha512-QjVjwdXIt408MIiAqCX4oUKsgU2EqAGzs2Ppkm4aQYbjm+ZEWEcW4SfFNTr4uMNZma0ey4f5lgLrkB0aX0QMow==