From 5d37245ec00ed98861d64bcec114860f2a97265b Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Thu, 9 May 2019 10:17:55 -0700 Subject: [PATCH 01/10] Remove timeout factory, use jest fake timers for testing --- .../__test__/browserDatafileManager.spec.ts | 13 +- .../httpPollingDatafileManager.spec.ts | 116 ++++++++++-------- .../__test__/nodeDatafileManager.spec.ts | 16 ++- .../__test__/testTimeoutFactory.ts | 37 ------ .../datafile-manager/__test__/testUtils.ts | 11 ++ .../datafile-manager/src/datafileManager.ts | 3 - .../src/httpPollingDatafileManager.ts | 17 +-- .../datafile-manager/src/timeoutFactory.ts | 28 ----- 8 files changed, 94 insertions(+), 147 deletions(-) delete mode 100644 packages/datafile-manager/__test__/testTimeoutFactory.ts create mode 100644 packages/datafile-manager/__test__/testUtils.ts delete mode 100644 packages/datafile-manager/src/timeoutFactory.ts diff --git a/packages/datafile-manager/__test__/browserDatafileManager.spec.ts b/packages/datafile-manager/__test__/browserDatafileManager.spec.ts index 5f2b2492f..d5f38c74c 100644 --- a/packages/datafile-manager/__test__/browserDatafileManager.spec.ts +++ b/packages/datafile-manager/__test__/browserDatafileManager.spec.ts @@ -17,19 +17,18 @@ import BrowserDatafileManager from '../src/browserDatafileManager' import * as browserRequest from '../src/browserRequest' import { Headers, AbortableRequest } from '../src/http' -import TestTimeoutFactory from './testTimeoutFactory' +import { advanceTimersByTime, getTimerCount } from './testUtils'; describe('browserDatafileManager', () => { - const testTimeoutFactory: TestTimeoutFactory = new TestTimeoutFactory() - let makeGetRequestSpy: jest.SpyInstance beforeEach(() => { + jest.useFakeTimers() makeGetRequestSpy = jest.spyOn(browserRequest, 'makeGetRequest') }) afterEach(() => { jest.restoreAllMocks() - testTimeoutFactory.cleanup() + jest.clearAllTimers() }) it('calls makeGetRequest when started', async () => { @@ -69,11 +68,10 @@ describe('browserDatafileManager', () => { const manager = new BrowserDatafileManager({ sdkKey: '1234', autoUpdate: true, - timeoutFactory: testTimeoutFactory, }) manager.start() await manager.onReady() - testTimeoutFactory.timeoutFns[0]() + await advanceTimersByTime(300000) expect(makeGetRequestSpy).toBeCalledTimes(2) expect(makeGetRequestSpy.mock.calls[1][0]).toBe('https://cdn.optimizely.com/datafiles/1234.json') expect(makeGetRequestSpy.mock.calls[1][1]).toEqual({ @@ -96,12 +94,11 @@ describe('browserDatafileManager', () => { }) const manager = new BrowserDatafileManager({ sdkKey: '1234', - timeoutFactory: testTimeoutFactory, }) manager.start() await manager.onReady() // Should not set a timeout for a later update - expect(testTimeoutFactory.timeoutFns.length).toBe(0) + expect(getTimerCount()).toBe(0) await manager.stop() }) diff --git a/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts b/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts index 00dc8d468..669bb5c78 100644 --- a/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts +++ b/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts @@ -17,7 +17,7 @@ import HTTPPollingDatafileManager from '../src/httpPollingDatafileManager' import { Headers, AbortableRequest, Response } from '../src/http' import { DatafileManagerConfig } from '../src/datafileManager'; -import TestTimeoutFactory from './testTimeoutFactory' +import { advanceTimersByTime, getTimerCount } from './testUtils' jest.mock('../src/backoffController', () => { return jest.fn().mockImplementation(() => { @@ -59,29 +59,23 @@ class TestDatafileManager extends HTTPPollingDatafileManager { } describe('httpPollingDatafileManager', () => { - const testTimeoutFactory: TestTimeoutFactory = new TestTimeoutFactory() - - function createTestManager(config: DatafileManagerConfig): TestDatafileManager { - return new TestDatafileManager({ - ...config, - timeoutFactory: testTimeoutFactory - }) - } + beforeEach(() => { + jest.useFakeTimers() + }) let manager: TestDatafileManager afterEach(async () => { - testTimeoutFactory.cleanup() - if (manager) { manager.stop() } jest.clearAllMocks() jest.restoreAllMocks() + jest.clearAllTimers() }) describe('when constructed with sdkKey and datafile and autoUpdate: true,', () => { beforeEach(() => { - manager = createTestManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: true }) + manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: true }) }) it('returns the passed datafile from get', () => { @@ -118,7 +112,9 @@ describe('httpPollingDatafileManager', () => { }) expect(manager.get()).toEqual({ foo: 'bar' }) updateFn.mockReset() - testTimeoutFactory.timeoutFns[0]() + + await advanceTimersByTime(300000) + expect(manager.responsePromises.length).toBe(2) await manager.responsePromises[1] expect(updateFn).toBeCalledTimes(1) @@ -131,7 +127,7 @@ describe('httpPollingDatafileManager', () => { describe('when constructed with sdkKey and datafile and autoUpdate: false,', () => { beforeEach(() => { - manager = createTestManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: false }) + manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: false }) }) it('returns the passed datafile from get', () => { @@ -160,13 +156,13 @@ describe('httpPollingDatafileManager', () => { datafile: { foo: 'bar' } }) expect(manager.get()).toEqual({ foo: 'bar' }) - expect(testTimeoutFactory.timeoutFns.length).toBe(0) + expect(getTimerCount()).toBe(0) }) }) describe('when constructed with sdkKey and autoUpdate: true', () => { beforeEach(() => { - manager = createTestManager({ sdkKey: '123', updateInterval: 1000, autoUpdate: true }) + manager = new TestDatafileManager({ sdkKey: '123', updateInterval: 1000, autoUpdate: true }) }) describe('initial state', () => { @@ -215,22 +211,32 @@ describe('httpPollingDatafileManager', () => { ) manager.start() await manager.onReady() - testTimeoutFactory.timeoutFns[0]() + await advanceTimersByTime(1000) + expect(manager.responsePromises.length).toBe(2) await manager.responsePromises[1] expect(manager.get()).toEqual({ foo: 'bar' }) }) describe('live updates', () => { - it('passes the update interval to its timeoutFactory setTimeout method', () => { - manager.queuedResponses.push({ - statusCode: 200, - body: '{"foo3": "bar3"}', - headers: {}, - }) - const setTimeoutSpy: jest.SpyInstance<() => void, [() => void, number]> = jest.spyOn(testTimeoutFactory, 'setTimeout') + it('sets a timeout to update again after the update interval', async () => { + manager.queuedResponses.push( + { + statusCode: 200, + body: '{"foo3": "bar3"}', + headers: {}, + }, + { + statusCode: 200, + body: '{"foo4": "bar4"}', + headers: {}, + }, + ) + const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest') manager.start() - expect(setTimeoutSpy).toBeCalledTimes(1) - expect(setTimeoutSpy.mock.calls[0][1]).toBe(1000) + expect(makeGetRequestSpy).toBeCalledTimes(1) + await manager.responsePromises[0] + await advanceTimersByTime(1000) + expect(makeGetRequestSpy).toBeCalledTimes(2) }) it('emits update events after live updates', async () => { @@ -260,7 +266,7 @@ describe('httpPollingDatafileManager', () => { expect(manager.get()).toEqual({ foo: 'bar' }) expect(updateFn).toBeCalledTimes(0) - testTimeoutFactory.timeoutFns[0]() + await advanceTimersByTime(1000) await manager.responsePromises[1] expect(updateFn).toBeCalledTimes(1) expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo2: 'bar2' } }) @@ -268,7 +274,7 @@ describe('httpPollingDatafileManager', () => { updateFn.mockReset() - testTimeoutFactory.timeoutFns[1]() + await advanceTimersByTime(1000) await manager.responsePromises[2] expect(updateFn).toBeCalledTimes(1) expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo3: 'bar3' } }) @@ -285,13 +291,11 @@ describe('httpPollingDatafileManager', () => { abort() {}, responsePromise, }) - const setTimeoutSpy = jest.spyOn(testTimeoutFactory, 'setTimeout') manager.start() - expect(setTimeoutSpy).toBeCalledTimes(1) expect(makeGetRequestSpy).toBeCalledTimes(1) - testTimeoutFactory.timeoutFns[0]() + await advanceTimersByTime(1000) expect(makeGetRequestSpy).toBeCalledTimes(1) resolveResponsePromise!({ @@ -301,7 +305,6 @@ describe('httpPollingDatafileManager', () => { }) await responsePromise expect(makeGetRequestSpy).toBeCalledTimes(2) - expect(setTimeoutSpy).toBeCalledTimes(2) }) }) @@ -317,10 +320,9 @@ describe('httpPollingDatafileManager', () => { manager.start() await manager.onReady() - expect(testTimeoutFactory.timeoutFns.length).toBe(1) - expect(testTimeoutFactory.cancelFns.length).toBe(1) + expect(getTimerCount()).toBe(1) manager.stop() - expect(testTimeoutFactory.cancelFns[0]).toBeCalledTimes(1) + expect(getTimerCount()).toBe(0) }) it('cancels reactions to a pending fetch when stop is called', async () => { @@ -340,7 +342,9 @@ describe('httpPollingDatafileManager', () => { manager.start() await manager.onReady() expect(manager.get()).toEqual({ foo: 'bar' }) - testTimeoutFactory.timeoutFns[0]() + + advanceTimersByTime(1000) + expect(manager.responsePromises.length).toBe(2) manager.stop() await manager.responsePromises[1] @@ -383,9 +387,9 @@ describe('httpPollingDatafileManager', () => { expect(manager.responsePromises.length).toBe(1) await manager.responsePromises[0] // Not ready yet due to first request failed, but should have queued a live update - expect(testTimeoutFactory.timeoutFns.length).toBe(1) + expect(getTimerCount()).toBe(1) // Trigger the update, should fetch the next response which should succeed, then we get ready - testTimeoutFactory.timeoutFns[0]() + advanceTimersByTime(1000) await manager.onReady() expect(manager.get()).toEqual({ foo: 'bar' }) }) @@ -416,7 +420,7 @@ describe('httpPollingDatafileManager', () => { // First response promise was for the initial 200 response expect(manager.responsePromises.length).toBe(1) // Trigger the queued update - testTimeoutFactory.timeoutFns[0]() + advanceTimersByTime(1000) // Second response promise is for the 304 response expect(manager.responsePromises.length).toBe(2) await manager.responsePromises[1] @@ -443,7 +447,7 @@ describe('httpPollingDatafileManager', () => { manager.start() await manager.onReady() const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest') - testTimeoutFactory.timeoutFns[0]() + advanceTimersByTime(1000) expect(makeGetRequestSpy).toBeCalledTimes(1) const firstCall = makeGetRequestSpy.mock.calls[0] const headers = firstCall[1] @@ -458,7 +462,9 @@ describe('httpPollingDatafileManager', () => { const BackoffControllerMock = (BackoffController as unknown) as jest.Mock const getDelayMock = BackoffControllerMock.mock.results[0].value.getDelay getDelayMock.mockImplementationOnce(() => 5432) - const setTimeoutSpy = jest.spyOn(testTimeoutFactory, 'setTimeout') + + const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest') + manager.queuedResponses.push( { statusCode: 404, @@ -468,8 +474,15 @@ describe('httpPollingDatafileManager', () => { ) manager.start() await manager.responsePromises[0] - expect(setTimeoutSpy).toBeCalledTimes(1) - expect(setTimeoutSpy.mock.calls[0][1]).toBe(5432) + expect(makeGetRequestSpy).toBeCalledTimes(1) + + // Should not make another request after 1 second because the error should have triggered backoff + advanceTimersByTime(1000) + expect(makeGetRequestSpy).toBeCalledTimes(1) + + // But after another 5 seconds, another request should be made + advanceTimersByTime(5000) + expect(makeGetRequestSpy).toBeCalledTimes(2) }) it('calls countError on the backoff controller when a non-success status code response is received', async () => { @@ -531,7 +544,7 @@ describe('httpPollingDatafileManager', () => { describe('when constructed with sdkKey and autoUpdate: false', () => { beforeEach(() => { - manager = createTestManager({ sdkKey: '123', autoUpdate: false }) + manager = new TestDatafileManager({ sdkKey: '123', autoUpdate: false }) }) it('after being started, fetches the datafile and resolves onReady', async () => { @@ -555,7 +568,7 @@ describe('httpPollingDatafileManager', () => { manager.on('update', updateFn) manager.start() await manager.onReady() - expect(testTimeoutFactory.timeoutFns.length).toBe(0) + expect(getTimerCount()).toBe(0) }) // TODO: figure out what's wrong with this test @@ -579,7 +592,7 @@ describe('httpPollingDatafileManager', () => { describe('when constructed with sdkKey and a valid urlTemplate', () => { beforeEach(() => { - manager = createTestManager({ + manager = new TestDatafileManager({ sdkKey: '456', updateInterval: 1000, urlTemplate: 'https://localhost:5556/datafiles/%s', @@ -602,22 +615,23 @@ describe('httpPollingDatafileManager', () => { describe('when constructed with an update interval below the minimum', () => { beforeEach(() => { - manager = createTestManager({ sdkKey: '123', updateInterval: 500, autoUpdate: true }) + manager = new TestDatafileManager({ sdkKey: '123', updateInterval: 500, autoUpdate: true }) }) it('uses the default update interval', async () => { + const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest') + manager.queuedResponses.push({ statusCode: 200, body: '{"foo3": "bar3"}', headers: {}, }) - const setTimeoutSpy: jest.SpyInstance<() => void, [() => void, number]> = jest.spyOn(testTimeoutFactory, 'setTimeout') - manager.start() await manager.onReady() - expect(setTimeoutSpy).toBeCalledTimes(1) - expect(setTimeoutSpy.mock.calls[0][1]).toBe(300000) + expect(makeGetRequestSpy).toBeCalledTimes(1) + await advanceTimersByTime(300000) + expect(makeGetRequestSpy).toBeCalledTimes(2) }) }) }) diff --git a/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts b/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts index 2f561cf2f..636e0d91f 100644 --- a/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts +++ b/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts @@ -17,19 +17,19 @@ import NodeDatafileManager from '../src/nodeDatafileManager' import * as nodeRequest from '../src/nodeRequest' import { Headers, AbortableRequest } from '../src/http' -import TestTimeoutFactory from './testTimeoutFactory' +import { advanceTimersByTime, getTimerCount } from './testUtils'; describe('nodeDatafileManager', () => { - const testTimeoutFactory: TestTimeoutFactory = new TestTimeoutFactory() - let makeGetRequestSpy: jest.SpyInstance beforeEach(() => { + jest.useFakeTimers() makeGetRequestSpy = jest.spyOn(nodeRequest, 'makeGetRequest') }) afterEach(() => { + jest.clearAllMocks() jest.restoreAllMocks() - testTimeoutFactory.cleanup() + jest.clearAllTimers() }) it('calls nodeEnvironment.makeGetRequest when started', async () => { @@ -69,11 +69,10 @@ describe('nodeDatafileManager', () => { const manager = new NodeDatafileManager({ sdkKey: '1234', autoUpdate: true, - timeoutFactory: testTimeoutFactory, }) manager.start() await manager.onReady() - testTimeoutFactory.timeoutFns[0]() + await advanceTimersByTime(300000) expect(makeGetRequestSpy).toBeCalledTimes(2) expect(makeGetRequestSpy.mock.calls[1][0]).toBe('https://cdn.optimizely.com/datafiles/1234.json') expect(makeGetRequestSpy.mock.calls[1][1]).toEqual({ @@ -96,13 +95,12 @@ describe('nodeDatafileManager', () => { }) const manager = new NodeDatafileManager({ sdkKey: '1234', - timeoutFactory: testTimeoutFactory, }) manager.start() await manager.onReady() // Should set a timeout for a later update - expect(testTimeoutFactory.timeoutFns.length).toBe(1) - testTimeoutFactory.timeoutFns[0]() + expect(getTimerCount()).toBe(1) + await advanceTimersByTime(300000) expect(makeGetRequestSpy).toBeCalledTimes(2) await manager.stop() diff --git a/packages/datafile-manager/__test__/testTimeoutFactory.ts b/packages/datafile-manager/__test__/testTimeoutFactory.ts deleted file mode 100644 index 7b1db3667..000000000 --- a/packages/datafile-manager/__test__/testTimeoutFactory.ts +++ /dev/null @@ -1,37 +0,0 @@ -/** - * Copyright 2019, Optimizely - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { TimeoutFactory } from '../src/timeoutFactory' - -export default class TestTimeoutFactory implements TimeoutFactory { - timeoutFns: Array<() => void> = [] - - cancelFns: Array<() => void> = [] - - setTimeout(onTimeout: () => void, timeout: number): () => void { - const cancelFn = jest.fn() - this.timeoutFns.push(() => { - onTimeout() - }) - this.cancelFns.push(cancelFn) - return cancelFn - } - - cleanup() { - this.timeoutFns = [] - this.cancelFns = [] - } -} diff --git a/packages/datafile-manager/__test__/testUtils.ts b/packages/datafile-manager/__test__/testUtils.ts new file mode 100644 index 000000000..aae4ee6e7 --- /dev/null +++ b/packages/datafile-manager/__test__/testUtils.ts @@ -0,0 +1,11 @@ +export function advanceTimersByTime(waitMs: number): Promise { + const timeoutPromise: Promise = new Promise(res => setTimeout(res, waitMs)) + jest.advanceTimersByTime(waitMs) + return timeoutPromise +} + +export function getTimerCount(): number { + // Type definition for jest doesn't include this, but it exists + // https://jestjs.io/docs/en/jest-object#jestgettimercount + return (jest as any).getTimerCount() +} diff --git a/packages/datafile-manager/src/datafileManager.ts b/packages/datafile-manager/src/datafileManager.ts index a1bb737e5..b4210e99d 100644 --- a/packages/datafile-manager/src/datafileManager.ts +++ b/packages/datafile-manager/src/datafileManager.ts @@ -14,8 +14,6 @@ * limitations under the License. */ -import { TimeoutFactory } from "./timeoutFactory"; - export interface DatafileUpdate { datafile: object } @@ -41,7 +39,6 @@ export interface DatafileManagerConfig { autoUpdate?: boolean datafile?: object sdkKey: string - timeoutFactory?: TimeoutFactory, updateInterval?: number urlTemplate?: string } diff --git a/packages/datafile-manager/src/httpPollingDatafileManager.ts b/packages/datafile-manager/src/httpPollingDatafileManager.ts index 62f4387aa..79420f903 100644 --- a/packages/datafile-manager/src/httpPollingDatafileManager.ts +++ b/packages/datafile-manager/src/httpPollingDatafileManager.ts @@ -20,7 +20,6 @@ import { DatafileManager, DatafileManagerConfig, DatafileUpdate } from './datafi import EventEmitter from './eventEmitter' import { AbortableRequest, Response, Headers } from './http'; import { DEFAULT_UPDATE_INTERVAL, MIN_UPDATE_INTERVAL, DEFAULT_URL_TEMPLATE } from './config' -import { TimeoutFactory, DEFAULT_TIMEOUT_FACTORY } from './timeoutFactory' import BackoffController from './backoffController'; const logger = getLogger('DatafileManager') @@ -61,7 +60,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana private readonly updateInterval: number - private cancelTimeout: (() => void) | null + private currentTimeout: any private isStarted: boolean @@ -69,8 +68,6 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana private datafileUrl: string - private timeoutFactory: TimeoutFactory - private currentRequest: AbortableRequest | null private backoffController: BackoffController @@ -90,7 +87,6 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana datafile, autoUpdate = false, sdkKey, - timeoutFactory = DEFAULT_TIMEOUT_FACTORY, updateInterval = DEFAULT_UPDATE_INTERVAL, urlTemplate = DEFAULT_URL_TEMPLATE, } = configWithDefaultsApplied @@ -114,7 +110,6 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana this.datafileUrl = sprintf(urlTemplate, sdkKey) - this.timeoutFactory = timeoutFactory this.emitter = new EventEmitter() this.autoUpdate = autoUpdate if (isValidUpdateInterval(updateInterval)) { @@ -123,7 +118,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana logger.warn('Invalid updateInterval %s, defaulting to %s', updateInterval, DEFAULT_UPDATE_INTERVAL) this.updateInterval = DEFAULT_UPDATE_INTERVAL } - this.cancelTimeout = null + this.currentTimeout = null this.currentRequest = null this.backoffController = new BackoffController() this.syncOnCurrentRequestComplete = false @@ -145,9 +140,9 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana stop(): Promise { logger.debug('Datafile manager stopped') this.isStarted = false - if (this.cancelTimeout) { - this.cancelTimeout() - this.cancelTimeout = null + if (this.currentTimeout) { + clearTimeout(this.currentTimeout) + this.currentTimeout = null } this.emitter.removeAllListeners() @@ -272,7 +267,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana const currentBackoffDelay = this.backoffController.getDelay() const nextUpdateDelay = Math.max(currentBackoffDelay, this.updateInterval) logger.debug('Scheduling sync in %s ms', nextUpdateDelay) - this.cancelTimeout = this.timeoutFactory.setTimeout(() => { + this.currentTimeout = setTimeout(() => { if (this.currentRequest) { this.syncOnCurrentRequestComplete = true } else { diff --git a/packages/datafile-manager/src/timeoutFactory.ts b/packages/datafile-manager/src/timeoutFactory.ts deleted file mode 100644 index a9dff614f..000000000 --- a/packages/datafile-manager/src/timeoutFactory.ts +++ /dev/null @@ -1,28 +0,0 @@ -/** - * Copyright 2019, Optimizely - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -export interface TimeoutFactory { - setTimeout(onTimeout: () => void, timeout: number): () => void -} - -export const DEFAULT_TIMEOUT_FACTORY: TimeoutFactory = { - setTimeout(onTimeout: () => void, timeout: number) { - const timeoutId = setTimeout(onTimeout, timeout) - return () => { - clearTimeout(timeoutId) - } - } -} From 674cd9ec1efbc57861b5b5ff9511f4d3424d3bff Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Thu, 9 May 2019 11:01:38 -0700 Subject: [PATCH 02/10] Remove clearAllMocks --- packages/datafile-manager/__test__/nodeDatafileManager.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts b/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts index 636e0d91f..1d90d6677 100644 --- a/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts +++ b/packages/datafile-manager/__test__/nodeDatafileManager.spec.ts @@ -27,7 +27,6 @@ describe('nodeDatafileManager', () => { }) afterEach(() => { - jest.clearAllMocks() jest.restoreAllMocks() jest.clearAllTimers() }) From da378b3574df8443686576af784f7ea9f867ae39 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Thu, 9 May 2019 11:31:32 -0700 Subject: [PATCH 03/10] License header on new file --- packages/datafile-manager/__test__/testUtils.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/datafile-manager/__test__/testUtils.ts b/packages/datafile-manager/__test__/testUtils.ts index aae4ee6e7..9d58c593b 100644 --- a/packages/datafile-manager/__test__/testUtils.ts +++ b/packages/datafile-manager/__test__/testUtils.ts @@ -1,3 +1,19 @@ +/** + * Copyright 2019, Optimizely + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + export function advanceTimersByTime(waitMs: number): Promise { const timeoutPromise: Promise = new Promise(res => setTimeout(res, waitMs)) jest.advanceTimersByTime(waitMs) From 0ebe8015eafe89073f81ebe496626aada5c155f3 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Thu, 9 May 2019 17:03:28 -0700 Subject: [PATCH 04/10] Make timeout 60 seconds. Add test. Use nise instead of sinon to get access to timeout property on fake XHR. --- .../__test__/browserRequest.spec.ts | 22 ++++++--- packages/datafile-manager/package.json | 4 +- .../datafile-manager/src/browserRequest.ts | 10 ++++ packages/datafile-manager/src/config.ts | 2 + packages/datafile-manager/yarn.lock | 47 +++++-------------- 5 files changed, 41 insertions(+), 44 deletions(-) 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/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/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== From f3dea83d7bc7d273b66a59c659607e5ca2ac3959 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Thu, 9 May 2019 18:06:16 -0700 Subject: [PATCH 05/10] Add timeout in node request --- .../__test__/nodeRequest.spec.ts | 38 +++++++++++++++++++ packages/datafile-manager/src/nodeRequest.ts | 9 +++++ 2 files changed, 47 insertions(+) diff --git a/packages/datafile-manager/__test__/nodeRequest.spec.ts b/packages/datafile-manager/__test__/nodeRequest.spec.ts index 7674ed50d..1f7bc1ae1 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,42 @@ describe('nodeEnvironment', () => { }) scope.done() }) + + describe('timeout', () => { + beforeEach(() => { + jest.useFakeTimers() + }) + + afterEach(() => { + jest.clearAllTimers() + }) + + it('rejects the response promise when the timeout fires', 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/src/nodeRequest.ts b/packages/datafile-manager/src/nodeRequest.ts index ee0cfa2ff..07a2298eb 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,7 @@ function getResponseFromRequest(request: http.ClientRequest): Promise }) request.on('error', (err: any) => { + clearTimeout(timeout) if (err instanceof Error) { reject(err) } else if (typeof err === 'string') { From e36b9af84cb311dc4862a4a2970ac482dd7f66d7 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Thu, 9 May 2019 18:08:33 -0700 Subject: [PATCH 06/10] Add changelog entry --- packages/datafile-manager/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/datafile-manager/CHANGELOG.md b/packages/datafile-manager/CHANGELOG.md index 21c21274c..5f556f493 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: From bb919311b535c45f206c0a4528f1c60dab062f1d Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 10 May 2019 10:03:28 -0700 Subject: [PATCH 07/10] Tweaks --- packages/datafile-manager/__test__/nodeRequest.spec.ts | 2 +- packages/datafile-manager/src/nodeRequest.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/datafile-manager/__test__/nodeRequest.spec.ts b/packages/datafile-manager/__test__/nodeRequest.spec.ts index 1f7bc1ae1..c8d46bd01 100644 --- a/packages/datafile-manager/__test__/nodeRequest.spec.ts +++ b/packages/datafile-manager/__test__/nodeRequest.spec.ts @@ -167,7 +167,7 @@ describe('nodeEnvironment', () => { jest.clearAllTimers() }) - it('rejects the response promise when the timeout fires', async () => { + 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) diff --git a/packages/datafile-manager/src/nodeRequest.ts b/packages/datafile-manager/src/nodeRequest.ts index 07a2298eb..aa56d5eeb 100644 --- a/packages/datafile-manager/src/nodeRequest.ts +++ b/packages/datafile-manager/src/nodeRequest.ts @@ -101,6 +101,7 @@ function getResponseFromRequest(request: http.ClientRequest): Promise request.on('error', (err: any) => { clearTimeout(timeout) + if (err instanceof Error) { reject(err) } else if (typeof err === 'string') { From fd0d3215d5b430058e1adece9042420f95f73bcf Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 10 May 2019 10:46:43 -0700 Subject: [PATCH 08/10] Clear timeout when request aborted --- packages/datafile-manager/src/nodeRequest.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/datafile-manager/src/nodeRequest.ts b/packages/datafile-manager/src/nodeRequest.ts index aa56d5eeb..d561f486f 100644 --- a/packages/datafile-manager/src/nodeRequest.ts +++ b/packages/datafile-manager/src/nodeRequest.ts @@ -61,11 +61,12 @@ function createHeadersFromNodeIncomingMessage( return headers } -function getResponseFromRequest(request: http.ClientRequest): Promise { +function getResponseFromRequest(request: http.ClientRequest): [Promise, NodeJS.Timeout] { + let timeout!: NodeJS.Timeout // 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(() => { + const promise: Promise = new Promise((resolve, reject) => { + timeout = setTimeout(() => { request.abort() reject(new Error('Request timed out')) }, REQUEST_TIMEOUT_MS) @@ -111,6 +112,7 @@ function getResponseFromRequest(request: http.ClientRequest): Promise } }) }) + return [promise, timeout] } export function makeGetRequest(reqUrl: string, headers: Headers): AbortableRequest { @@ -138,12 +140,13 @@ export function makeGetRequest(reqUrl: string, headers: Headers): AbortableReque } const request = requester(requestOptions) - const responsePromise = getResponseFromRequest(request) + const [responsePromise, timeout] = getResponseFromRequest(request) request.end() return { abort() { + clearTimeout(timeout) request.abort() }, responsePromise, From aaa1caa20e131b9cac2caa272d01fc0692985228 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 10 May 2019 11:38:15 -0700 Subject: [PATCH 09/10] remove blank line --- packages/datafile-manager/__test__/nodeRequest.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/datafile-manager/__test__/nodeRequest.spec.ts b/packages/datafile-manager/__test__/nodeRequest.spec.ts index c8d46bd01..c9154e5fe 100644 --- a/packages/datafile-manager/__test__/nodeRequest.spec.ts +++ b/packages/datafile-manager/__test__/nodeRequest.spec.ts @@ -193,6 +193,5 @@ describe('nodeEnvironment', () => { scope.off('request', requestListener) }) }) - }) }) From c3dcb25160ad66bd54d20d594a6dc5bec4b6fbb0 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 10 May 2019 11:47:43 -0700 Subject: [PATCH 10/10] Remove unnecessary clearTimeout --- packages/datafile-manager/src/nodeRequest.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/datafile-manager/src/nodeRequest.ts b/packages/datafile-manager/src/nodeRequest.ts index d561f486f..aa56d5eeb 100644 --- a/packages/datafile-manager/src/nodeRequest.ts +++ b/packages/datafile-manager/src/nodeRequest.ts @@ -61,12 +61,11 @@ function createHeadersFromNodeIncomingMessage( return headers } -function getResponseFromRequest(request: http.ClientRequest): [Promise, NodeJS.Timeout] { - let timeout!: NodeJS.Timeout +function getResponseFromRequest(request: http.ClientRequest): Promise { // TODO: When we drop support for Node 6, consider using util.promisify instead of // constructing own Promise - const promise: Promise = new Promise((resolve, reject) => { - timeout = setTimeout(() => { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { request.abort() reject(new Error('Request timed out')) }, REQUEST_TIMEOUT_MS) @@ -112,7 +111,6 @@ function getResponseFromRequest(request: http.ClientRequest): [Promise } }) }) - return [promise, timeout] } export function makeGetRequest(reqUrl: string, headers: Headers): AbortableRequest { @@ -140,13 +138,12 @@ export function makeGetRequest(reqUrl: string, headers: Headers): AbortableReque } const request = requester(requestOptions) - const [responsePromise, timeout] = getResponseFromRequest(request) + const responsePromise = getResponseFromRequest(request) request.end() return { abort() { - clearTimeout(timeout) request.abort() }, responsePromise,