Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit cec5eee

Browse files
mjc1283mjc1283
authored
feat (datafile manager): Increasing backoff delay after network errors or non-success response status (optimizely#7)
Summary: Implement an increasing delay for the next request after a response Promise is rejected, or resolved with a non-successful status. If the configured updateInterval is greater than the calculated delay, we just use the updateInterval. Test plan: Added unit tests for new module backoffController. Added unit tests for httpPollingDatafileManager backoff functionality.
1 parent c8650e3 commit cec5eee

File tree

5 files changed

+235
-16
lines changed

5 files changed

+235
-16
lines changed

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

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/**
2+
* Copyright 2019, Optimizely
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
import BackoffController from '../src/backoffController'
18+
19+
describe('backoffController', () => {
20+
describe('getDelay', () => {
21+
it('returns 0 from getDelay if there have been no errors', () => {
22+
const controller = new BackoffController()
23+
expect(controller.getDelay()).toBe(0)
24+
})
25+
26+
it('increases the delay returned from getDelay (up to a maximum value) after each call to countError', () => {
27+
const controller = new BackoffController()
28+
controller.countError()
29+
expect(controller.getDelay()).toBeGreaterThanOrEqual(2000)
30+
expect(controller.getDelay()).toBeLessThan(3000)
31+
controller.countError()
32+
expect(controller.getDelay()).toBeGreaterThanOrEqual(4000)
33+
expect(controller.getDelay()).toBeLessThan(5000)
34+
controller.countError()
35+
expect(controller.getDelay()).toBeGreaterThanOrEqual(8000)
36+
expect(controller.getDelay()).toBeLessThan(9000)
37+
controller.countError()
38+
expect(controller.getDelay()).toBeGreaterThanOrEqual(16000)
39+
expect(controller.getDelay()).toBeLessThan(17000)
40+
controller.countError()
41+
expect(controller.getDelay()).toBeGreaterThanOrEqual(32000)
42+
expect(controller.getDelay()).toBeLessThan(33000)
43+
// Maximum reached - additional errors should not increase the delay further
44+
controller.countError()
45+
expect(controller.getDelay()).toBeGreaterThanOrEqual(32000)
46+
expect(controller.getDelay()).toBeLessThan(33000)
47+
})
48+
49+
it('resets the error count when reset is called', () => {
50+
const controller = new BackoffController()
51+
controller.countError()
52+
expect(controller.getDelay()).toBeGreaterThan(0)
53+
controller.reset()
54+
expect(controller.getDelay()).toBe(0)
55+
})
56+
})
57+
})

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

Lines changed: 91 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,33 @@ import { Headers, AbortableRequest, Response } from '../src/http'
1919
import { DatafileManagerConfig } from '../src/datafileManager';
2020
import TestTimeoutFactory from './testTimeoutFactory'
2121

22+
jest.mock('../src/backoffController', () => {
23+
return jest.fn().mockImplementation(() => {
24+
const getDelayMock = jest.fn().mockImplementation(() => 0)
25+
return {
26+
getDelay: getDelayMock,
27+
countError: jest.fn(),
28+
reset: jest.fn(),
29+
}
30+
})
31+
});
32+
33+
import BackoffController from '../src/backoffController'
34+
2235
// Test implementation:
2336
// - Does not make any real requests: just resolves with queued responses (tests push onto queuedResponses)
2437
class TestDatafileManager extends HTTPPollingDatafileManager {
25-
queuedResponses: Response[] = []
38+
queuedResponses: (Response | Error)[] = []
2639

2740
responsePromises: Promise<Response>[] = []
2841

2942
makeGetRequest(url: string, headers: Headers): AbortableRequest {
30-
const nextResponse: Response | undefined = this.queuedResponses.pop()
43+
const nextResponse: Error | Response | undefined = this.queuedResponses.pop()
3144
let responsePromise: Promise<Response>
3245
if (nextResponse === undefined) {
3346
responsePromise = Promise.reject('No responses queued')
47+
} else if (nextResponse instanceof Error) {
48+
responsePromise = Promise.reject(nextResponse)
3449
} else {
3550
responsePromise = Promise.resolve(nextResponse)
3651
}
@@ -56,6 +71,7 @@ describe('httpPollingDatafileManager', () => {
5671
if (manager) {
5772
manager.stop()
5873
}
74+
jest.clearAllMocks()
5975
jest.restoreAllMocks()
6076
})
6177

@@ -321,6 +337,79 @@ describe('httpPollingDatafileManager', () => {
321337
})
322338
})
323339
})
340+
341+
describe('backoff', () => {
342+
it('uses the delay from the backoff controller getDelay method when greater than updateInterval', async () => {
343+
const BackoffControllerMock = (BackoffController as unknown) as jest.Mock<BackoffController, []>
344+
const getDelayMock = BackoffControllerMock.mock.results[0].value.getDelay
345+
getDelayMock.mockImplementationOnce(() => 5432)
346+
const setTimeoutSpy = jest.spyOn(testTimeoutFactory, 'setTimeout')
347+
manager.queuedResponses.push(
348+
{
349+
statusCode: 404,
350+
body: '',
351+
headers: {}
352+
}
353+
)
354+
manager.start()
355+
await manager.responsePromises[0]
356+
expect(setTimeoutSpy).toBeCalledTimes(1)
357+
expect(setTimeoutSpy.mock.calls[0][1]).toBe(5432)
358+
})
359+
360+
it('calls countError on the backoff controller when a non-success status code response is received', async () => {
361+
manager.queuedResponses.push(
362+
{
363+
statusCode: 404,
364+
body: '',
365+
headers: {}
366+
}
367+
)
368+
manager.start()
369+
await manager.responsePromises[0]
370+
const BackoffControllerMock = (BackoffController as unknown) as jest.Mock<BackoffController, []>
371+
expect(BackoffControllerMock.mock.results[0].value.countError).toBeCalledTimes(1)
372+
})
373+
374+
it('calls countError on the backoff controller when the response promise rejects', async () => {
375+
manager.queuedResponses.push(new Error('Connection failed'))
376+
manager.start()
377+
try {
378+
await manager.responsePromises[0]
379+
} catch (e) {
380+
}
381+
const BackoffControllerMock = (BackoffController as unknown) as jest.Mock<BackoffController, []>
382+
expect(BackoffControllerMock.mock.results[0].value.countError).toBeCalledTimes(1)
383+
})
384+
385+
it('calls reset on the backoff controller when a success status code response is received', async () => {
386+
manager.queuedResponses.push(
387+
{
388+
statusCode: 200,
389+
body: '{"foo": "bar"}',
390+
headers: {
391+
'Last-Modified': 'Fri, 08 Mar 2019 18:57:17 GMT',
392+
},
393+
}
394+
)
395+
manager.start()
396+
const BackoffControllerMock = (BackoffController as unknown) as jest.Mock<BackoffController, []>
397+
// Reset is called in start - we want to check that it is also called after the response, so reset the mock here
398+
BackoffControllerMock.mock.results[0].value.reset.mockReset()
399+
await manager.onReady()
400+
expect(BackoffControllerMock.mock.results[0].value.reset).toBeCalledTimes(1)
401+
})
402+
403+
it('resets the backoff controller when start is called', async () => {
404+
const BackoffControllerMock = (BackoffController as unknown) as jest.Mock<BackoffController, []>
405+
manager.start()
406+
expect(BackoffControllerMock.mock.results[0].value.reset).toBeCalledTimes(1)
407+
try {
408+
await manager.responsePromises[0]
409+
} catch (e) {
410+
}
411+
})
412+
})
324413
})
325414
})
326415
})

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/**
2+
* Copyright 2019, Optimizely
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
import {
18+
BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT,
19+
BACKOFF_MAX_ERROR_COUNT,
20+
} from './config'
21+
22+
function randomMilliseconds() {
23+
return Math.round(Math.random() * 1000)
24+
}
25+
26+
export default class BackoffController {
27+
private errorCount = 0
28+
29+
getDelay(): number {
30+
if (this.errorCount === 0) {
31+
return 0
32+
}
33+
const baseWaitSeconds =
34+
BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT[
35+
Math.min(BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT.length - 1, this.errorCount)
36+
]
37+
return baseWaitSeconds * 1000 + randomMilliseconds()
38+
}
39+
40+
countError(): void {
41+
if (this.errorCount < BACKOFF_MAX_ERROR_COUNT) {
42+
this.errorCount++
43+
}
44+
}
45+
46+
reset(): void {
47+
this.errorCount = 0
48+
}
49+
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,7 @@ export const MIN_UPDATE_INTERVAL = 1
2121
export const SDK_KEY_TOKEN = '$SDK_KEY'
2222

2323
export const DEFAULT_URL_TEMPLATE = `https://cdn.optimizely.com/datafiles/${SDK_KEY_TOKEN}.json`
24+
25+
export const BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT = [0, 2, 4, 8, 16, 32]
26+
27+
export const BACKOFF_MAX_ERROR_COUNT = 10

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

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import EventEmitter from './eventEmitter'
2020
import { AbortableRequest, Response, Headers } from './http';
2121
import { DEFAULT_UPDATE_INTERVAL, MIN_UPDATE_INTERVAL, DEFAULT_URL_TEMPLATE, SDK_KEY_TOKEN } from './config'
2222
import { TimeoutFactory, DEFAULT_TIMEOUT_FACTORY } from './timeoutFactory'
23+
import BackoffController from './backoffController';
2324

2425
const logger = getLogger('DatafileManager')
2526

@@ -29,6 +30,10 @@ function isValidUpdateInterval(updateInterval: number): boolean {
2930
return updateInterval >= MIN_UPDATE_INTERVAL
3031
}
3132

33+
function isSuccessStatusCode(statusCode: number): boolean {
34+
return statusCode >= 200 && statusCode < 400
35+
}
36+
3237
export default abstract class HTTPPollingDatafileManager implements DatafileManager {
3338
// Make an HTTP get request to the given URL with the given headers
3439
// Return an AbortableRequest, which has a promise for a Response.
@@ -66,6 +71,8 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
6671

6772
private currentRequest?: AbortableRequest
6873

74+
private backoffController: BackoffController
75+
6976
constructor(config: DatafileManagerConfig) {
7077
const {
7178
datafile,
@@ -108,6 +115,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
108115
logger.warn('Invalid updateInterval %s, defaulting to %s', updateInterval, DEFAULT_UPDATE_INTERVAL)
109116
this.updateInterval = DEFAULT_UPDATE_INTERVAL
110117
}
118+
this.backoffController = new BackoffController()
111119
}
112120

113121
get(): string | null {
@@ -118,6 +126,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
118126
if (!this.isStarted) {
119127
logger.debug('Datafile manager started')
120128
this.isStarted = true
129+
this.backoffController.reset()
121130
this.syncDatafile()
122131
}
123132
}
@@ -152,11 +161,13 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
152161
return this.urlTemplate.replace(SDK_KEY_TOKEN, sdkKey)
153162
}
154163

155-
private logMakeGetRequestError(err: any): void {
164+
private onRequestRejected(err: any): void {
156165
if (!this.isStarted) {
157166
return
158167
}
159168

169+
this.backoffController.countError()
170+
160171
if (err instanceof Error) {
161172
logger.error('Error fetching datafile: %s', err.message, err)
162173
} else if (typeof err === 'string') {
@@ -166,11 +177,18 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
166177
}
167178
}
168179

169-
private tryUpdatingDatafile(response: Response): void {
180+
private onRequestResolved(response: Response): void {
170181
if (!this.isStarted) {
171182
return
172183
}
173184

185+
if (typeof response.statusCode !== 'undefined' &&
186+
isSuccessStatusCode(response.statusCode)) {
187+
this.backoffController.reset()
188+
} else {
189+
this.backoffController.countError()
190+
}
191+
174192
this.trySavingLastModified(response.headers)
175193

176194
const datafile = this.getNextDatafileFromResponse(response)
@@ -188,7 +206,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
188206
}
189207
}
190208

191-
private onFetchComplete(this: HTTPPollingDatafileManager): void {
209+
private onRequestComplete(this: HTTPPollingDatafileManager): void {
192210
if (!this.isStarted) {
193211
return
194212
}
@@ -215,18 +233,18 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
215233
logger.debug('Making datafile request to url %s with headers: %s', datafileUrl, () => JSON.stringify(headers))
216234
this.currentRequest = this.makeGetRequest(datafileUrl, headers)
217235

218-
const onFetchComplete = () => {
219-
this.onFetchComplete()
236+
const onRequestComplete = () => {
237+
this.onRequestComplete()
220238
}
221-
const tryUpdatingDatafile = (response: Response) => {
222-
this.tryUpdatingDatafile(response)
239+
const onRequestResolved = (response: Response) => {
240+
this.onRequestResolved(response)
223241
}
224-
const logMakeGetRequestError = (err: any) => {
225-
this.logMakeGetRequestError(err)
242+
const onRequestRejected = (err: any) => {
243+
this.onRequestRejected(err)
226244
}
227245
this.currentRequest.responsePromise
228-
.then(tryUpdatingDatafile, logMakeGetRequestError)
229-
.then(onFetchComplete, onFetchComplete)
246+
.then(onRequestResolved, onRequestRejected)
247+
.then(onRequestComplete, onRequestComplete)
230248
}
231249

232250
private resolveReadyPromise(): void {
@@ -240,10 +258,12 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
240258
}
241259

242260
private scheduleNextUpdate(): void {
243-
logger.debug('Scheduling sync in %s ms', this.updateInterval)
261+
const currentBackoffDelay = this.backoffController.getDelay()
262+
const nextUpdateDelay = Math.max(currentBackoffDelay, this.updateInterval)
263+
logger.debug('Scheduling sync in %s ms', nextUpdateDelay)
244264
this.cancelTimeout = this.timeoutFactory.setTimeout(() => {
245265
this.syncDatafile()
246-
}, this.updateInterval)
266+
}, nextUpdateDelay)
247267
}
248268

249269
private getNextDatafileFromResponse(response: Response): string | null {
@@ -254,7 +274,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
254274
if (response.statusCode === 304) {
255275
return null
256276
}
257-
if (response.statusCode >= 200 && response.statusCode < 400) {
277+
if (isSuccessStatusCode(response.statusCode)) {
258278
return response.body
259279
}
260280
return null

0 commit comments

Comments
 (0)