Merge branch '38394-smarter-interval' into 'master'

don't re-run smart interval callback if there is already one in progress

Closes #38394

See merge request gitlab-org/gitlab-ce!15032

5 files changed, 171 insertions, 133 deletions

diff --git a/app/assets/javascripts/smart_interval.js b/app/assets/javascripts/smart_interval.js
index 2bf7a3a5d61..8e931995fc6 100644
--- a/app/assets/javascripts/smart_interval.js
+++ b/app/assets/javascripts/smart_interval.js
@@ -3,9 +3,10 @@
  * and controllable by a public API.
  */
 
-class SmartInterval {
+export default class SmartInterval {
   /**
-   * @param { function } opts.callback Function to be called on each iteration (required)
+   * @param { function } opts.callback Function that returns a promise, called on each iteration
+   *                     unless still in progress (required)
    * @param { milliseconds } opts.startingInterval `currentInterval` is set to this initially
    * @param { milliseconds } opts.maxInterval `currentInterval` will be incremented to this
    * @param { milliseconds } opts.hiddenInterval `currentInterval` is set to this
@@ -42,13 +43,16 @@ class SmartInterval {
     const cfg = this.cfg;
     const state = this.state;
 
-    if (cfg.immediateExecution) {
+    if (cfg.immediateExecution && !this.isLoading) {
       cfg.immediateExecution = false;
-      cfg.callback();
+      this.triggerCallback();
     }
 
     state.intervalId = window.setInterval(() => {
-      cfg.callback();
+      if (this.isLoading) {
+        return;
+      }
+      this.triggerCallback();
 
       if (this.getCurrentInterval() === cfg.maxInterval) {
         return;
@@ -76,7 +80,7 @@ class SmartInterval {
 
   // start a timer, using the existing interval
   resume() {
-    this.stopTimer(); // stop exsiting timer, in case timer was not previously stopped
+    this.stopTimer(); // stop existing timer, in case timer was not previously stopped
     this.start();
   }
 
@@ -104,6 +108,18 @@ class SmartInterval {
     this.initPageUnloadHandling();
   }
 
+  triggerCallback() {
+    this.isLoading = true;
+    this.cfg.callback()
+      .then(() => {
+        this.isLoading = false;
+      })
+      .catch((err) => {
+        this.isLoading = false;
+        throw err;
+      });
+  }
+
   initVisibilityChangeHandling() {
     // cancel interval when tab no longer shown (prevents cached pages from polling)
     document.addEventListener('visibilitychange', this.handleVisibilityChange.bind(this));
@@ -154,4 +170,3 @@ class SmartInterval {
   }
 }
 
-window.gl.SmartInterval = SmartInterval;
diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
index 4f497b204a3..aaff9ee6518 100644
--- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
+++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
@@ -1,3 +1,4 @@
+import SmartInterval from '~/smart_interval';
 import Flash from '../flash';
 import {
   WidgetHeader,
@@ -81,7 +82,7 @@ export default {
       return new MRWidgetService(endpoints);
     },
     checkStatus(cb) {
-      this.service.checkStatus()
+      return this.service.checkStatus()
         .then(res => res.json())
         .then((res) => {
           this.handleNotification(res);
@@ -97,7 +98,7 @@ export default {
         });
     },
     initPolling() {
-      this.pollingInterval = new gl.SmartInterval({
+      this.pollingInterval = new SmartInterval({
         callback: this.checkStatus,
         startingInterval: 10000,
         maxInterval: 30000,
@@ -106,7 +107,7 @@ export default {
       });
     },
     initDeploymentsPolling() {
-      this.deploymentsInterval = new gl.SmartInterval({
+      this.deploymentsInterval = new SmartInterval({
         callback: this.fetchDeployments,
         startingInterval: 30000,
         maxInterval: 120000,
@@ -121,7 +122,7 @@ export default {
       }
     },
     fetchDeployments() {
-      this.service.fetchDeployments()
+      return this.service.fetchDeployments()
         .then(res => res.json())
         .then((res) => {
           if (res.length) {
diff --git a/changelogs/unreleased/38394-smarter-interval.yml b/changelogs/unreleased/38394-smarter-interval.yml
new file mode 100644
index 00000000000..ead8c3eca5a
--- /dev/null
+++ b/changelogs/unreleased/38394-smarter-interval.yml
@@ -0,0 +1,5 @@
+---
+title: Update Merge Request polling so there is only one request at a time
+merge_request: 15032
+author:
+type: fixed
diff --git a/spec/javascripts/smart_interval_spec.js b/spec/javascripts/smart_interval_spec.js
index 7833bf3fb04..1c87fcec245 100644
--- a/spec/javascripts/smart_interval_spec.js
+++ b/spec/javascripts/smart_interval_spec.js
@@ -1,6 +1,6 @@
-import '~/smart_interval';
+import SmartInterval from '~/smart_interval';
 
-(() => {
+describe('SmartInterval', function () {
   const DEFAULT_MAX_INTERVAL = 100;
   const DEFAULT_STARTING_INTERVAL = 5;
   const DEFAULT_SHORT_TIMEOUT = 75;
@@ -9,7 +9,7 @@ import '~/smart_interval';
 
   function createDefaultSmartInterval(config) {
     const defaultParams = {
-      callback: () => {},
+      callback: () => Promise.resolve(),
       startingInterval: DEFAULT_STARTING_INTERVAL,
       maxInterval: DEFAULT_MAX_INTERVAL,
       incrementByFactorOf: DEFAULT_INCREMENT_FACTOR,
@@ -22,158 +22,171 @@ import '~/smart_interval';
       _.extend(defaultParams, config);
     }
 
-    return new gl.SmartInterval(defaultParams);
+    return new SmartInterval(defaultParams);
   }
 
-  describe('SmartInterval', function () {
-    describe('Increment Interval', function () {
-      beforeEach(function () {
-        this.smartInterval = createDefaultSmartInterval();
-      });
+  describe('Increment Interval', function () {
+    beforeEach(function () {
+      this.smartInterval = createDefaultSmartInterval();
+    });
 
-      it('should increment the interval delay', function (done) {
-        const interval = this.smartInterval;
-        setTimeout(() => {
-          const intervalConfig = this.smartInterval.cfg;
-          const iterationCount = 4;
-          const maxIntervalAfterIterations = intervalConfig.startingInterval *
-            (intervalConfig.incrementByFactorOf ** (iterationCount - 1)); // 40
-          const currentInterval = interval.getCurrentInterval();
-
-          // Provide some flexibility for performance of testing environment
-          expect(currentInterval).toBeGreaterThan(intervalConfig.startingInterval);
-          expect(currentInterval <= maxIntervalAfterIterations).toBeTruthy();
-
-          done();
-        }, DEFAULT_SHORT_TIMEOUT); // 4 iterations, increment by 2x = (5 + 10 + 20 + 40)
-      });
+    it('should increment the interval delay', function (done) {
+      const interval = this.smartInterval;
+      setTimeout(() => {
+        const intervalConfig = this.smartInterval.cfg;
+        const iterationCount = 4;
+        const maxIntervalAfterIterations = intervalConfig.startingInterval *
+          (intervalConfig.incrementByFactorOf ** (iterationCount - 1)); // 40
+        const currentInterval = interval.getCurrentInterval();
+
+        // Provide some flexibility for performance of testing environment
+        expect(currentInterval).toBeGreaterThan(intervalConfig.startingInterval);
+        expect(currentInterval <= maxIntervalAfterIterations).toBeTruthy();
+
+        done();
+      }, DEFAULT_SHORT_TIMEOUT); // 4 iterations, increment by 2x = (5 + 10 + 20 + 40)
+    });
 
-      it('should not increment past maxInterval', function (done) {
-        const interval = this.smartInterval;
+    it('should not increment past maxInterval', function (done) {
+      const interval = this.smartInterval;
 
-        setTimeout(() => {
-          const currentInterval = interval.getCurrentInterval();
-          expect(currentInterval).toBe(interval.cfg.maxInterval);
+      setTimeout(() => {
+        const currentInterval = interval.getCurrentInterval();
+        expect(currentInterval).toBe(interval.cfg.maxInterval);
 
-          done();
-        }, DEFAULT_LONG_TIMEOUT);
-      });
+        done();
+      }, DEFAULT_LONG_TIMEOUT);
     });
 
-    describe('Public methods', function () {
-      beforeEach(function () {
-        this.smartInterval = createDefaultSmartInterval();
+    it('does not increment while waiting for callback', function () {
+      jasmine.clock().install();
+
+      const smartInterval = createDefaultSmartInterval({
+        callback: () => new Promise($.noop),
       });
 
-      it('should cancel an interval', function (done) {
-        const interval = this.smartInterval;
+      jasmine.clock().tick(DEFAULT_SHORT_TIMEOUT);
+
+      const oneInterval = smartInterval.cfg.startingInterval * DEFAULT_INCREMENT_FACTOR;
+      expect(smartInterval.getCurrentInterval()).toEqual(oneInterval);
 
-        setTimeout(() => {
-          interval.cancel();
+      jasmine.clock().uninstall();
+    });
+  });
 
-          const intervalId = interval.state.intervalId;
-          const currentInterval = interval.getCurrentInterval();
-          const intervalLowerLimit = interval.cfg.startingInterval;
+  describe('Public methods', function () {
+    beforeEach(function () {
+      this.smartInterval = createDefaultSmartInterval();
+    });
 
-          expect(intervalId).toBeUndefined();
-          expect(currentInterval).toBe(intervalLowerLimit);
+    it('should cancel an interval', function (done) {
+      const interval = this.smartInterval;
 
-          done();
-        }, DEFAULT_SHORT_TIMEOUT);
-      });
+      setTimeout(() => {
+        interval.cancel();
 
-      it('should resume an interval', function (done) {
-        const interval = this.smartInterval;
+        const intervalId = interval.state.intervalId;
+        const currentInterval = interval.getCurrentInterval();
+        const intervalLowerLimit = interval.cfg.startingInterval;
 
-        setTimeout(() => {
-          interval.cancel();
+        expect(intervalId).toBeUndefined();
+        expect(currentInterval).toBe(intervalLowerLimit);
 
-          interval.resume();
+        done();
+      }, DEFAULT_SHORT_TIMEOUT);
+    });
 
-          const intervalId = interval.state.intervalId;
+    it('should resume an interval', function (done) {
+      const interval = this.smartInterval;
 
-          expect(intervalId).toBeTruthy();
+      setTimeout(() => {
+        interval.cancel();
 
-          done();
-        }, DEFAULT_SHORT_TIMEOUT);
-      });
+        interval.resume();
+
+        const intervalId = interval.state.intervalId;
+
+        expect(intervalId).toBeTruthy();
+
+        done();
+      }, DEFAULT_SHORT_TIMEOUT);
     });
+  });
 
-    describe('DOM Events', function () {
-      beforeEach(function () {
-        // This ensures DOM and DOM events are initialized for these specs.
-        setFixtures('<div></div>');
+  describe('DOM Events', function () {
+    beforeEach(function () {
+      // This ensures DOM and DOM events are initialized for these specs.
+      setFixtures('<div></div>');
 
-        this.smartInterval = createDefaultSmartInterval();
-      });
+      this.smartInterval = createDefaultSmartInterval();
+    });
 
-      it('should pause when page is not visible', function (done) {
-        const interval = this.smartInterval;
+    it('should pause when page is not visible', function (done) {
+      const interval = this.smartInterval;
 
-        setTimeout(() => {
-          expect(interval.state.intervalId).toBeTruthy();
+      setTimeout(() => {
+        expect(interval.state.intervalId).toBeTruthy();
 
-          // simulates triggering of visibilitychange event
-          interval.handleVisibilityChange({ target: { visibilityState: 'hidden' } });
+        // simulates triggering of visibilitychange event
+        interval.handleVisibilityChange({ target: { visibilityState: 'hidden' } });
 
-          expect(interval.state.intervalId).toBeUndefined();
-          done();
-        }, DEFAULT_SHORT_TIMEOUT);
-      });
+        expect(interval.state.intervalId).toBeUndefined();
+        done();
+      }, DEFAULT_SHORT_TIMEOUT);
+    });
 
-      it('should change to the hidden interval when page is not visible', function (done) {
-        const HIDDEN_INTERVAL = 1500;
-        const interval = createDefaultSmartInterval({ hiddenInterval: HIDDEN_INTERVAL });
+    it('should change to the hidden interval when page is not visible', function (done) {
+      const HIDDEN_INTERVAL = 1500;
+      const interval = createDefaultSmartInterval({ hiddenInterval: HIDDEN_INTERVAL });
 
-        setTimeout(() => {
-          expect(interval.state.intervalId).toBeTruthy();
-          expect(interval.getCurrentInterval() >= DEFAULT_STARTING_INTERVAL &&
-            interval.getCurrentInterval() <= DEFAULT_MAX_INTERVAL).toBeTruthy();
+      setTimeout(() => {
+        expect(interval.state.intervalId).toBeTruthy();
+        expect(interval.getCurrentInterval() >= DEFAULT_STARTING_INTERVAL &&
+          interval.getCurrentInterval() <= DEFAULT_MAX_INTERVAL).toBeTruthy();
 
-          // simulates triggering of visibilitychange event
-          interval.handleVisibilityChange({ target: { visibilityState: 'hidden' } });
+        // simulates triggering of visibilitychange event
+        interval.handleVisibilityChange({ target: { visibilityState: 'hidden' } });
 
-          expect(interval.state.intervalId).toBeTruthy();
-          expect(interval.getCurrentInterval()).toBe(HIDDEN_INTERVAL);
-          done();
-        }, DEFAULT_SHORT_TIMEOUT);
-      });
+        expect(interval.state.intervalId).toBeTruthy();
+        expect(interval.getCurrentInterval()).toBe(HIDDEN_INTERVAL);
+        done();
+      }, DEFAULT_SHORT_TIMEOUT);
+    });
 
-      it('should resume when page is becomes visible at the previous interval', function (done) {
-        const interval = this.smartInterval;
+    it('should resume when page is becomes visible at the previous interval', function (done) {
+      const interval = this.smartInterval;
 
-        setTimeout(() => {
-          expect(interval.state.intervalId).toBeTruthy();
+      setTimeout(() => {
+        expect(interval.state.intervalId).toBeTruthy();
 
-          // simulates triggering of visibilitychange event
-          interval.handleVisibilityChange({ target: { visibilityState: 'hidden' } });
+        // simulates triggering of visibilitychange event
+        interval.handleVisibilityChange({ target: { visibilityState: 'hidden' } });
 
-          expect(interval.state.intervalId).toBeUndefined();
+        expect(interval.state.intervalId).toBeUndefined();
 
-          // simulates triggering of visibilitychange event
-          interval.handleVisibilityChange({ target: { visibilityState: 'visible' } });
+        // simulates triggering of visibilitychange event
+        interval.handleVisibilityChange({ target: { visibilityState: 'visible' } });
 
-          expect(interval.state.intervalId).toBeTruthy();
+        expect(interval.state.intervalId).toBeTruthy();
 
-          done();
-        }, DEFAULT_SHORT_TIMEOUT);
-      });
+        done();
+      }, DEFAULT_SHORT_TIMEOUT);
+    });
 
-      it('should cancel on page unload', function (done) {
-        const interval = this.smartInterval;
+    it('should cancel on page unload', function (done) {
+      const interval = this.smartInterval;
 
-        setTimeout(() => {
-          $(document).triggerHandler('beforeunload');
-          expect(interval.state.intervalId).toBeUndefined();
-          expect(interval.getCurrentInterval()).toBe(interval.cfg.startingInterval);
-          done();
-        }, DEFAULT_SHORT_TIMEOUT);
-      });
+      setTimeout(() => {
+        $(document).triggerHandler('beforeunload');
+        expect(interval.state.intervalId).toBeUndefined();
+        expect(interval.getCurrentInterval()).toBe(interval.cfg.startingInterval);
+        done();
+      }, DEFAULT_SHORT_TIMEOUT);
+    });
 
-      it('should execute callback before first interval', function () {
-        const interval = createDefaultSmartInterval({ immediateExecution: true });
-        expect(interval.cfg.immediateExecution).toBeFalsy();
-      });
+    it('should execute callback before first interval', function () {
+      const interval = createDefaultSmartInterval({ immediateExecution: true });
+      expect(interval.cfg.immediateExecution).toBeFalsy();
     });
   });
-})(window.gl || (window.gl = {}));
+});
diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js
index e4324e91502..ba3721e7c84 100644
--- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js
+++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js
@@ -121,24 +121,28 @@ describe('mrWidgetOptions', () => {
 
     describe('initPolling', () => {
       it('should call SmartInterval', () => {
-        spyOn(gl, 'SmartInterval').and.returnValue({
-          resume() {},
-          stopTimer() {},
-        });
+        spyOn(vm, 'checkStatus').and.returnValue(Promise.resolve());
+        jasmine.clock().install();
         vm.initPolling();
 
+        expect(vm.checkStatus).not.toHaveBeenCalled();
+
+        jasmine.clock().tick(10000);
+
         expect(vm.pollingInterval).toBeDefined();
-        expect(gl.SmartInterval).toHaveBeenCalled();
+        expect(vm.checkStatus).toHaveBeenCalled();
+
+        jasmine.clock().uninstall();
       });
     });
 
     describe('initDeploymentsPolling', () => {
       it('should call SmartInterval', () => {
-        spyOn(gl, 'SmartInterval');
+        spyOn(vm, 'fetchDeployments').and.returnValue(Promise.resolve());
         vm.initDeploymentsPolling();
 
         expect(vm.deploymentsInterval).toBeDefined();
-        expect(gl.SmartInterval).toHaveBeenCalled();
+        expect(vm.fetchDeployments).toHaveBeenCalled();
       });
     });