Skip to content

Commit 4e8bd24

Browse files
mjc1283mjc1283
authored
feat(datafile manager): Add request timeouts (#268)
Summary: This adds a 60 second timeout to all requests made by the datafile manager. In the browser, we assign to the timeout property of the XHR. In node, we set our own timeout, and abort the request when the timeout fires. The response promise is rejected when the request times out. Test plan: Manual & unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4685
1 parent bfebb68 commit 4e8bd24

File tree

8 files changed

+91
-44
lines changed

8 files changed

+91
-44
lines changed

‎packages/datafile-manager/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
77
## [Unreleased]
88
Changes that have landed but are not yet released.
99

10+
### New Features
11+
- Added 60 second timeout for all requests
12+
1013
### Changed
1114
- Changed value for node in engines in package.json from >=4.0.0 to >=6.0.0
1215
- Updated polling behavior:

‎packages/datafile-manager/__test__/browserRequest.spec.ts

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,19 @@
1818
*/
1919

2020
import {
21-
SinonFakeXMLHttpRequest,
22-
SinonFakeXMLHttpRequestStatic,
23-
useFakeXMLHttpRequest,
24-
} from 'sinon'
21+
FakeXMLHttpRequest,
22+
FakeXMLHttpRequestStatic,
23+
fakeXhr
24+
} from 'nise'
2525
import { makeGetRequest } from '../src/browserRequest'
2626

2727
describe('browserRequest', () => {
2828
describe('makeGetRequest', () => {
29-
let mockXHR: SinonFakeXMLHttpRequestStatic
30-
let xhrs: SinonFakeXMLHttpRequest[]
29+
let mockXHR: FakeXMLHttpRequestStatic
30+
let xhrs: FakeXMLHttpRequest[]
3131
beforeEach(() => {
3232
xhrs = []
33-
mockXHR = useFakeXMLHttpRequest()
33+
mockXHR = fakeXhr.useFakeXMLHttpRequest()
3434
mockXHR.onCreate = req => xhrs.push(req)
3535
})
3636

@@ -126,5 +126,13 @@ describe('browserRequest', () => {
126126
xhrs[0].error()
127127
await expect(req.responsePromise).rejects.toThrow()
128128
})
129+
130+
it('sets a timeout on the request object', () => {
131+
const onCreateMock = jest.fn()
132+
mockXHR.onCreate = onCreateMock
133+
makeGetRequest('https://cdn.optimizely.com/datafiles/123.json', {})
134+
expect(onCreateMock).toBeCalledTimes(1)
135+
expect(onCreateMock.mock.calls[0][0].timeout).toBe(60000)
136+
})
129137
})
130138
})

‎packages/datafile-manager/__test__/nodeRequest.spec.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import nock from 'nock'
1818
import { makeGetRequest } from '../src/nodeRequest'
19+
import { advanceTimersByTime } from './testUtils'
1920

2021
beforeAll(() => {
2122
nock.disableNetConnect()
@@ -156,5 +157,41 @@ describe('nodeEnvironment', () => {
156157
})
157158
scope.done()
158159
})
160+
161+
describe('timeout', () => {
162+
beforeEach(() => {
163+
jest.useFakeTimers()
164+
})
165+
166+
afterEach(() => {
167+
jest.clearAllTimers()
168+
})
169+
170+
it('rejects the response promise and aborts the request when the response is not received before the timeout', async () => {
171+
const scope = nock(host)
172+
.get(path)
173+
.delay(61000)
174+
.reply(200, '{"foo":"bar"}')
175+
176+
const abortEventListener = jest.fn()
177+
let emittedReq: any
178+
const requestListener = (request: any) => {
179+
emittedReq = request
180+
emittedReq.once('abort', abortEventListener)
181+
}
182+
scope.on('request', requestListener)
183+
184+
const req = makeGetRequest(`${host}${path}`, {})
185+
await advanceTimersByTime(60000)
186+
await expect(req.responsePromise).rejects.toThrow()
187+
expect(abortEventListener).toBeCalledTimes(1)
188+
189+
scope.done()
190+
if (emittedReq) {
191+
emittedReq.off('abort', abortEventListener)
192+
}
193+
scope.off('request', requestListener)
194+
})
195+
})
159196
})
160197
})

‎packages/datafile-manager/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@
2525
},
2626
"devDependencies": {
2727
"@types/jest": "^24.0.9",
28+
"@types/nise": "^1.4.0",
2829
"@types/nock": "^9.3.1",
2930
"@types/node": "^11.11.7",
30-
"@types/sinon": "^7.0.10",
3131
"jest": "^24.1.0",
32+
"nise": "^1.4.10",
3233
"nock": "^10.0.6",
33-
"sinon": "^7.2.7",
3434
"ts-jest": "^24.0.0",
3535
"typescript": "^3.3.3333"
3636
},

‎packages/datafile-manager/src/browserRequest.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@
1515
*/
1616

1717
import { AbortableRequest, Response, Headers } from './http'
18+
import { REQUEST_TIMEOUT_MS } from './config'
19+
import { getLogger } from '@optimizely/js-sdk-logging'
20+
21+
const logger = getLogger('DatafileManager')
1822

1923
const GET_METHOD = 'GET'
2024
const READY_STATE_DONE = 4
@@ -74,6 +78,12 @@ export function makeGetRequest(reqUrl: string, headers: Headers): AbortableReque
7478
}
7579
}
7680

81+
req.timeout = REQUEST_TIMEOUT_MS
82+
83+
req.ontimeout = () => {
84+
logger.error('Request timed out')
85+
}
86+
7787
req.send()
7888
})
7989

‎packages/datafile-manager/src/config.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,5 @@ export const MIN_UPDATE_INTERVAL = 1000
2121
export const DEFAULT_URL_TEMPLATE = `https://cdn.optimizely.com/datafiles/%s.json`
2222

2323
export const BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT = [0, 8, 16, 32, 64, 128, 256, 512]
24+
25+
export const REQUEST_TIMEOUT_MS = 60 * 1000 // 1 minute

‎packages/datafile-manager/src/nodeRequest.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import http from 'http'
1818
import https from 'https'
1919
import url from 'url'
2020
import { Headers, AbortableRequest, Response } from './http'
21+
import { REQUEST_TIMEOUT_MS } from './config';
2122

2223
// Shared signature between http.request and https.request
2324
type ClientRequestCreator = (options: http.RequestOptions) => http.ClientRequest
@@ -64,6 +65,11 @@ function getResponseFromRequest(request: http.ClientRequest): Promise<Response>
6465
// TODO: When we drop support for Node 6, consider using util.promisify instead of
6566
// constructing own Promise
6667
return new Promise((resolve, reject) => {
68+
const timeout = setTimeout(() => {
69+
request.abort()
70+
reject(new Error('Request timed out'))
71+
}, REQUEST_TIMEOUT_MS)
72+
6773
request.once('response', (incomingMessage: http.IncomingMessage) => {
6874
if (request.aborted) {
6975
return
@@ -83,6 +89,8 @@ function getResponseFromRequest(request: http.ClientRequest): Promise<Response>
8389
return
8490
}
8591

92+
clearTimeout(timeout)
93+
8694
resolve({
8795
statusCode: incomingMessage.statusCode,
8896
body: responseData,
@@ -92,6 +100,8 @@ function getResponseFromRequest(request: http.ClientRequest): Promise<Response>
92100
})
93101

94102
request.on('error', (err: any) => {
103+
clearTimeout(timeout)
104+
95105
if (err instanceof Error) {
96106
reject(err)
97107
} else if (typeof err === 'string') {

‎packages/datafile-manager/yarn.lock

Lines changed: 12 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -145,25 +145,25 @@
145145
dependencies:
146146
uuid "^3.3.2"
147147

148-
"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2", "@sinonjs/commons@^1.3.1":
148+
"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2":
149149
version "1.4.0"
150150
resolved "https://registry.yarnpkg.com/@sinonjs/commons/-/commons-1.4.0.tgz#7b3ec2d96af481d7a0321252e7b1c94724ec5a78"
151151
integrity sha512-9jHK3YF/8HtJ9wCAbG+j8cD0i0+ATS9A7gXFqS36TblLPNy6rEEc+SB0imo91eCboGaBYGV/MT1/br/J+EE7Tw==
152152
dependencies:
153153
type-detect "4.0.8"
154154

155-
"@sinonjs/formatio@^3.1.0", "@sinonjs/formatio@^3.2.1":
155+
"@sinonjs/formatio@^3.1.0":
156156
version "3.2.1"
157157
resolved "https://registry.yarnpkg.com/@sinonjs/formatio/-/formatio-3.2.1.tgz#52310f2f9bcbc67bdac18c94ad4901b95fde267e"
158158
integrity sha512-tsHvOB24rvyvV2+zKMmPkZ7dXX6LSLKZ7aOtXY6Edklp0uRcgGpOsQTTGTcWViFyx4uhWc6GV8QdnALbIbIdeQ==
159159
dependencies:
160160
"@sinonjs/commons" "^1"
161161
"@sinonjs/samsam" "^3.1.0"
162162

163-
"@sinonjs/samsam@^3.1.0", "@sinonjs/samsam@^3.2.0":
164-
version "3.3.0"
165-
resolved "https://registry.yarnpkg.com/@sinonjs/samsam/-/samsam-3.3.0.tgz#9557ea89cd39dbc94ffbd093c8085281cac87416"
166-
integrity sha512-beHeJM/RRAaLLsMJhsCvHK31rIqZuobfPLa/80yGH5hnD8PV1hyh9xJBJNFfNmO7yWqm+zomijHsXpI6iTQJfQ==
163+
"@sinonjs/samsam@^3.1.0":
164+
version "3.3.1"
165+
resolved "https://registry.yarnpkg.com/@sinonjs/samsam/-/samsam-3.3.1.tgz#e88c53fbd9d91ad9f0f2b0140c16c7c107fe0d07"
166+
integrity sha512-wRSfmyd81swH0hA1bxJZJ57xr22kC07a1N4zuIL47yTS04bDk6AoCkczcqHEjcRPmJ+FruGJ9WBQiJwMtIElFw==
167167
dependencies:
168168
"@sinonjs/commons" "^1.0.2"
169169
array-from "^2.1.1"
@@ -186,6 +186,11 @@
186186
dependencies:
187187
"@types/jest-diff" "*"
188188

189+
"@types/nise@^1.4.0":
190+
version "1.4.0"
191+
resolved "https://registry.yarnpkg.com/@types/nise/-/nise-1.4.0.tgz#83af8ffc6ec4c622ffdbf298491137346f482a35"
192+
integrity sha512-DPxmjiDwubsNmguG5X4fEJ+XCyzWM3GXWsqQlvUcjJKa91IOoJUy51meDr0GkzK64qqNcq85ymLlyjoct9tInw==
193+
189194
"@types/nock@^9.3.1":
190195
version "9.3.1"
191196
resolved "https://registry.yarnpkg.com/@types/nock/-/nock-9.3.1.tgz#7d761a43a10aebc7ec6bae29d89afc6cbffa5d30"
@@ -203,11 +208,6 @@
203208
resolved "https://registry.yarnpkg.com/@types/node/-/node-11.11.7.tgz#f1c35a906b82adae76ede5ab0d2088e58fa37843"
204209
integrity sha512-bHbRcyD6XpXVLg42QYaQCjvDXaCFkvb3WbCIxSDmhGbJYVroxvYzekk9QGg1beeIawfvSLkdZpP0h7jxE4ihnA==
205210

206-
"@types/sinon@^7.0.10":
207-
version "7.0.10"
208-
resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-7.0.10.tgz#1f921f0c347b19f754e61dbc671c088df73fe1ff"
209-
integrity sha512-4w7SvsiUOtd4mUfund9QROPSJ5At/GQskDpqd87pJIRI6ULWSJqHI3GIZE337wQuN3aznroJGr94+o8fwvL37Q==
210-
211211
abab@^2.0.0:
212212
version "2.0.0"
213213
resolved "https://registry.yarnpkg.com/abab/-/abab-2.0.0.tgz#aba0ab4c5eee2d4c79d3487d85450fb2376ebb0f"
@@ -841,11 +841,6 @@ diff-sequences@^24.0.0:
841841
resolved "https://registry.yarnpkg.com/diff-sequences/-/diff-sequences-24.0.0.tgz#cdf8e27ed20d8b8d3caccb4e0c0d8fe31a173013"
842842
integrity sha512-46OkIuVGBBnrC0soO/4LHu5LHGHx0uhP65OVz8XOrAJpqiCB2aVIuESvjI1F9oqebuvY8lekS1pt6TN7vt7qsw==
843843

844-
diff@^3.5.0:
845-
version "3.5.0"
846-
resolved "https://registry.yarnpkg.com/diff/-/diff-3.5.0.tgz#800c0dd1e0a8bfbc95835c202ad220fe317e5a12"
847-
integrity sha512-A46qtFgd+g7pDZinpnwiRJtxbC1hpgf0uzP3iG89scHk0AUC7A1TGxf5OiiOUv/JMZR8GOt8hL900hV0bOy5xA==
848-
849844
domexception@^1.0.1:
850845
version "1.0.1"
851846
resolved "https://registry.yarnpkg.com/domexception/-/domexception-1.0.1.tgz#937442644ca6a31261ef36e3ec677fe805582c90"
@@ -2115,11 +2110,6 @@ lolex@^2.3.2:
21152110
resolved "https://registry.yarnpkg.com/lolex/-/lolex-2.7.5.tgz#113001d56bfc7e02d56e36291cc5c413d1aa0733"
21162111
integrity sha512-l9x0+1offnKKIzYVjyXU2SiwhXDLekRzKyhnbyldPHvC7BvLPVpdNUNR2KeMAiCN2D/kLNttZgQD5WjSxuBx3Q==
21172112

2118-
lolex@^3.1.0:
2119-
version "3.1.0"
2120-
resolved "https://registry.yarnpkg.com/lolex/-/lolex-3.1.0.tgz#1a7feb2fefd75b3e3a7f79f0e110d9476e294434"
2121-
integrity sha512-zFo5MgCJ0rZ7gQg69S4pqBsLURbFw11X68C18OcJjJQbqaXm2NoTrGl1IMM3TIz0/BnN1tIs2tzmmqvCsOMMjw==
2122-
21232113
loose-envify@^1.0.0:
21242114
version "1.4.0"
21252115
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:
30133003
resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.2.tgz#b5fdc08f1287ea1178628e415e25132b73646c6d"
30143004
integrity sha1-tf3AjxKH6hF4Yo5BXiUTK3NkbG0=
30153005

3016-
sinon@^7.2.7:
3017-
version "7.2.7"
3018-
resolved "https://registry.yarnpkg.com/sinon/-/sinon-7.2.7.tgz#ee90f83ce87d9a6bac42cf32a3103d8c8b1bfb68"
3019-
integrity sha512-rlrre9F80pIQr3M36gOdoCEWzFAMDgHYD8+tocqOw+Zw9OZ8F84a80Ds69eZfcjnzDqqG88ulFld0oin/6rG/g==
3020-
dependencies:
3021-
"@sinonjs/commons" "^1.3.1"
3022-
"@sinonjs/formatio" "^3.2.1"
3023-
"@sinonjs/samsam" "^3.2.0"
3024-
diff "^3.5.0"
3025-
lolex "^3.1.0"
3026-
nise "^1.4.10"
3027-
supports-color "^5.5.0"
3028-
30293006
sisteransi@^1.0.0:
30303007
version "1.0.0"
30313008
resolved "https://registry.yarnpkg.com/sisteransi/-/sisteransi-1.0.0.tgz#77d9622ff909080f1c19e5f4a1df0c1b0a27b88c"
@@ -3239,7 +3216,7 @@ strip-json-comments@~2.0.1:
32393216
resolved "https://registry.yarnpkg.com/strip-json-comments/-/strip-json-comments-2.0.1.tgz#3c531942e908c2697c0ec344858c286c7ca0a60a"
32403217
integrity sha1-PFMZQukIwml8DsNEhYwobHygpgo=
32413218

3242-
supports-color@^5.3.0, supports-color@^5.5.0:
3219+
supports-color@^5.3.0:
32433220
version "5.5.0"
32443221
resolved "https://registry.yarnpkg.com/supports-color/-/supports-color-5.5.0.tgz#e2e69a44ac8772f78a1ec0b35b689df6530efc8f"
32453222
integrity sha512-QjVjwdXIt408MIiAqCX4oUKsgU2EqAGzs2Ppkm4aQYbjm+ZEWEcW4SfFNTr4uMNZma0ey4f5lgLrkB0aX0QMow==

0 commit comments

Comments
 (0)