fix (datafile management): Update polling behavior (#266)

Summary:
- When autoUpdate is true, start the timer for the following request immediately, instead of waiting for the response to start the timer
- When the timeout fires before the response is received, wait for the response to complete and then start the next request immediately. This ensures there is at most 1 request in flight at any time.

Test plan:
Added new unit tests. Manually tested.

Issues:
https://optimizely.atlassian.net/browse/OASIS-4628

Author: mjc1283

packages/datafile-manager/CHANGELOG.md

@@ -9,6 +9,9 @@ Changes that have landed but are not yet released.
 
 ### Changed
 - Changed value for node in engines in package.json from >=4.0.0 to >=6.0.0
+- Updated polling behavior:
+  - Start update interval timer immediately after request
+  - When update interval timer fires during request, wait until request completes, then immediately start next request
 
 ## [0.2.0] - April 9, 2019
 

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

@@ -221,17 +221,14 @@ describe('httpPollingDatafileManager', () => {
       })
 
       describe('live updates', () => {
-        it('passes the update interval to its timeoutFactory setTimeout method', async () => {
+        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')
-
           manager.start()
-          await manager.onReady()
           expect(setTimeoutSpy).toBeCalledTimes(1)
           expect(setTimeoutSpy.mock.calls[0][1]).toBe(1000)
         })
@@ -278,6 +275,36 @@ describe('httpPollingDatafileManager', () => {
           expect(manager.get()).toEqual({ foo3: 'bar3' })
         })
 
+        describe('when the update interval time fires before the request is complete', () => {
+          it('waits until the request is complete before making the next request', async () => {
+            let resolveResponsePromise: (resp: Response) => void
+            const responsePromise: Promise<Response> = new Promise(res => {
+              resolveResponsePromise = res
+            })
+            const makeGetRequestSpy = jest.spyOn(manager, 'makeGetRequest').mockReturnValueOnce({
+              abort() {},
+              responsePromise,
+            })
+            const setTimeoutSpy = jest.spyOn(testTimeoutFactory, 'setTimeout')
+
+            manager.start()
+            expect(setTimeoutSpy).toBeCalledTimes(1)
+            expect(makeGetRequestSpy).toBeCalledTimes(1)
+
+            testTimeoutFactory.timeoutFns[0]()
+            expect(makeGetRequestSpy).toBeCalledTimes(1)
+
+            resolveResponsePromise!({
+              statusCode: 200,
+              body: '{"foo": "bar"}',
+              headers: {},
+            })
+            await responsePromise
+            expect(makeGetRequestSpy).toBeCalledTimes(2)
+            expect(setTimeoutSpy).toBeCalledTimes(2)
+          })
+        })
+
         it('cancels a pending timeout when stop is called', async () => {
           manager.queuedResponses.push(
             {

packages/datafile-manager/src/httpPollingDatafileManager.ts

@@ -61,7 +61,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
 
   private readonly updateInterval: number
 
-  private cancelTimeout?: () => void
+  private cancelTimeout: (() => void) | null
 
   private isStarted: boolean
 
@@ -71,10 +71,16 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
 
   private timeoutFactory: TimeoutFactory
 
-  private currentRequest?: AbortableRequest
+  private currentRequest: AbortableRequest | null
 
   private backoffController: BackoffController
 
+  // When true, this means the update interval timeout fired before the current
+  // sync completed. In that case, we should sync again immediately upon
+  // completion of the current request, instead of waiting another update
+  // interval.
+  private syncOnCurrentRequestComplete: boolean
+
   constructor(config: DatafileManagerConfig) {
     const configWithDefaultsApplied: DatafileManagerConfig = {
       ...this.getConfigDefaults(),
@@ -117,7 +123,10 @@ 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.currentRequest = null
     this.backoffController = new BackoffController()
+    this.syncOnCurrentRequestComplete = false
   }
 
   get(): object | null {
@@ -138,14 +147,14 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
     this.isStarted = false
     if (this.cancelTimeout) {
       this.cancelTimeout()
-      this.cancelTimeout = undefined
+      this.cancelTimeout = null
     }
 
     this.emitter.removeAllListeners()
 
     if (this.currentRequest) {
       this.currentRequest.abort()
-      this.currentRequest = undefined
+      this.currentRequest = null
     }
 
     return Promise.resolve()
@@ -209,15 +218,17 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
       return
     }
 
-    this.currentRequest = undefined
+    this.currentRequest = null
 
-    if (this.autoUpdate) {
-      this.scheduleNextUpdate()
-    }
     if (!this.isReadyPromiseSettled && !this.autoUpdate) {
       // We will never resolve ready, so reject it
       this.rejectReadyPromise(new Error('Failed to become ready'))
     }
+
+    if (this.autoUpdate && this.syncOnCurrentRequestComplete) {
+      this.syncDatafile()
+    }
+    this.syncOnCurrentRequestComplete = false
   }
 
   private syncDatafile(): void {
@@ -241,6 +252,10 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
     this.currentRequest.responsePromise
       .then(onRequestResolved, onRequestRejected)
       .then(onRequestComplete, onRequestComplete)
+
+    if (this.autoUpdate) {
+      this.scheduleNextUpdate()
+    }
   }
 
   private resolveReadyPromise(): void {
@@ -258,7 +273,11 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
     const nextUpdateDelay = Math.max(currentBackoffDelay, this.updateInterval)
     logger.debug('Scheduling sync in %s ms', nextUpdateDelay)
     this.cancelTimeout = this.timeoutFactory.setTimeout(() => {
-      this.syncDatafile()
+      if (this.currentRequest) {
+        this.syncOnCurrentRequestComplete = true
+      } else {
+        this.syncDatafile()
+      }
     }, nextUpdateDelay)
   }