Add ability to cancel promises via a wrapper (#374)

* Add ability to cancel promises via a wrapper

* Let the ApiHelpers keep track of outstanding requests, provide ApiHelpers.cancel()

web/app/js/components/DeploymentDetail.jsx

@@ -34,6 +34,7 @@ export default class DeploymentDetail extends React.Component {
 
   componentWillUnmount() {
     window.clearInterval(this.timerId);
+    this.api.cancelCurrentRequests();
   }
 
   initialState(location) {
@@ -60,17 +61,19 @@ export default class DeploymentDetail extends React.Component {
 
     let urls = this.api.urlsForResource;
 
-    let podListFetch = this.api.fetchPods();
     let deployMetricsUrl = urls["deployment"].url(this.state.deploy).rollup;
     let upstreamRollupUrl = urls["upstream_deployment"].url(this.state.deploy).rollup;
     let downstreamRollupUrl = urls["downstream_deployment"].url(this.state.deploy).rollup;
 
-    let deployFetch = this.api.fetchMetrics(deployMetricsUrl);
-    let upstreamFetch = this.api.fetchMetrics(upstreamRollupUrl);
-    let downstreamFetch = this.api.fetchMetrics(downstreamRollupUrl);
+    this.api.setCurrentRequests([
+      this.api.fetchMetrics(deployMetricsUrl),
+      this.api.fetchMetrics(upstreamRollupUrl),
+      this.api.fetchMetrics(downstreamRollupUrl),
+      this.api.fetchPods()
+    ]);
 
     // expose serverPromise for testing
-    this.serverPromise = Promise.all([deployFetch, upstreamFetch, downstreamFetch, podListFetch])
+    this.serverPromise = Promise.all(this.api.getCurrentPromises())
       .then(([deployMetrics, upstreamRollup, downstreamRollup, podList]) => {
         let deployRollup = processRollupMetrics(deployMetrics.metrics, "targetDeploy");
         let upstreamMetrics = processRollupMetrics(upstreamRollup.metrics, "sourceDeploy");
@@ -90,10 +93,15 @@ export default class DeploymentDetail extends React.Component {
           loaded: true,
           error: ''
         });
-      }).catch(this.handleApiError);
+      })
+      .catch(this.handleApiError);
   }
 
   handleApiError(e) {
+    if (e.isCanceled) {
+      return;
+    }
+
     this.setState({
       pendingRequests: false,
       error: `Error getting data from server: ${e.message}`

web/app/js/components/DeploymentsList.jsx

@@ -32,6 +32,7 @@ export default class DeploymentsList extends React.Component {
 
   componentWillUnmount() {
     window.clearInterval(this.timerId);
+    this.api.cancelCurrentRequests();
   }
 
   addDeploysWithNoMetrics(deploys, metrics) {
@@ -51,11 +52,13 @@ export default class DeploymentsList extends React.Component {
     }
     this.setState({ pendingRequests: true });
 
-    let rollupRequest = this.api.fetchMetrics(this.api.urlsForResource["deployment"].url().rollup);
-    let podsRequest = this.api.fetchPods();
+    this.api.setCurrentRequests([
+      this.api.fetchMetrics(this.api.urlsForResource["deployment"].url().rollup),
+      this.api.fetchPods()
+    ]);
 
     // expose serverPromise for testing
-    this.serverPromise = Promise.all([rollupRequest, podsRequest])
+    this.serverPromise = Promise.all(this.api.getCurrentPromises())
       .then(([rollup, p]) => {
         let poByDeploy = getPodsByDeployment(p.pods);
         let meshDeploys = processRollupMetrics(rollup.metrics, "targetDeploy");
@@ -72,6 +75,10 @@ export default class DeploymentsList extends React.Component {
   }
 
   handleApiError(e) {
+    if (e.isCanceled) {
+      return;
+    }
+
     this.setState({
       pendingRequests: false,
       error: `Error getting data from server: ${e.message}`

web/app/js/components/ServiceMesh.jsx

@@ -84,6 +84,7 @@ export default class ServiceMesh extends React.Component {
 
   componentWillUnmount() {
     window.clearInterval(this.timerId);
+    this.api.cancelCurrentRequests();
   }
 
   loadFromServer() {
@@ -95,11 +96,13 @@ export default class ServiceMesh extends React.Component {
     let rollupPath = `/api/metrics?aggregation=mesh`;
     let timeseriesPath = `${rollupPath}&timeseries=true`;
 
-    let rollupRequest = this.api.fetchMetrics(rollupPath);
-    let timeseriesRequest = this.api.fetchMetrics(timeseriesPath);
-    let podsRequest = this.api.fetchPods();
+    this.api.setCurrentRequests([
+      this.api.fetchMetrics(rollupPath),
+      this.api.fetchMetrics(timeseriesPath),
+      this.api.fetchPods()
+    ]);
 
-    this.serverPromise = Promise.all([rollupRequest, timeseriesRequest, podsRequest])
+    this.serverPromise = Promise.all(this.api.getCurrentPromises())
       .then(([metrics, ts, pods]) => {
         let m = processRollupMetrics(metrics.metrics, "component");
         let tsByComponent = processTimeseriesMetrics(ts.metrics, "component");
@@ -116,10 +119,15 @@ export default class ServiceMesh extends React.Component {
           loaded: true,
           error: ''
         });
-      }).catch(this.handleApiError);
+      })
+      .catch(this.handleApiError);
   }
 
   handleApiError(e) {
+    if (e.isCanceled) {
+      return;
+    }
+
     this.setState({
       pendingRequests: false,
       error: `Error getting data from server: ${e.message}`

web/app/js/components/Sidebar.jsx

@@ -41,7 +41,7 @@ export default class Sidebar extends React.Component {
     }
     this.setState({ pendingRequests: true });
 
-    this.api.fetchPods().then(r => {
+    this.api.fetchPods().promise.then(r => {
       let deploys =  _.map(getPodsByDeployment(r.pods), 'name');
 
       this.setState({

web/app/js/components/Version.jsx

@@ -30,8 +30,8 @@ export default class Version extends React.Component {
     let versionUrl = `https://versioncheck.conduit.io/version.json?version=${this.props.releaseVersion}?uuid=${this.props.uuid}`;
     let versionFetch = ApiHelpers("").fetch(versionUrl);
     // expose serverPromise for testing
-    this.serverPromise = Promise.all([versionFetch])
-      .then(([resp]) => {
+    this.serverPromise = versionFetch.promise
+      .then(resp => {
         this.setState({
           latest: resp.version,
           loaded: true,

web/app/js/components/util/ApiHelpers.jsx

@@ -3,6 +3,39 @@ import { Link } from 'react-router-dom';
 import React from 'react';
 import 'whatwg-fetch';
 
+const checkFetchOk = resp => {
+  if (!resp.ok) {
+    throw Error(resp.statusText);
+  } else {
+    return resp;
+  }
+};
+
+// makeCancelable from @istarkov
+// https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html
+const makeCancelable = (promise, onSuccess) => {
+  let hasCanceled_ = false;
+
+  const wrappedPromise = new Promise((resolve, reject) => {
+    return promise.then(
+      result => hasCanceled_ ? reject({ isCanceled: true }) : resolve(result),
+      error => hasCanceled_ ? reject({ isCanceled: true }) : reject(error)
+    );
+  })
+    .then(checkFetchOk)
+    .then(onSuccess);
+
+  return {
+    promise: wrappedPromise,
+    cancel: () => {
+      hasCanceled_ = true;
+    },
+    status: () => {
+      return hasCanceled_;
+    }
+  };
+};
+
 export const ApiHelpers = (pathPrefix, defaultMetricsWindow = '10m') => {
   let metricsWindow = defaultMetricsWindow;
   const podsPath = `/api/pods`;
@@ -18,7 +51,8 @@ export const ApiHelpers = (pathPrefix, defaultMetricsWindow = '10m') => {
     if (!_.isEmpty(pathPrefix)) {
       path = `${pathPrefix}${path}`;
     }
-    return fetch(path).then(handleFetchErr).then(r => r.json());
+
+    return makeCancelable(fetch(path), r => r.json());
   };
 
   const fetchMetrics = path => {
@@ -36,12 +70,7 @@ export const ApiHelpers = (pathPrefix, defaultMetricsWindow = '10m') => {
     return apiFetch(podsPath);
   };
 
-  const handleFetchErr = resp => {
-    if (!resp.ok) {
-      throw Error(resp.statusText);
-    }
-    return resp;
-  };
+
 
   const getMetricsWindow = () => metricsWindow;
   const getMetricsWindowDisplayText = () => validMetricsWindows[metricsWindow];
@@ -92,6 +121,20 @@ export const ApiHelpers = (pathPrefix, defaultMetricsWindow = '10m') => {
     }
   };
 
+  // maintain a list of a component's requests,
+  // convenient for providing a cancel() functionality
+  let currentRequests = [];
+  const setCurrentRequests = cancelablePromises => {
+    currentRequests = cancelablePromises;
+  };
+  const getCurrentPromises = () => {
+    return _.map(currentRequests, 'promise');
+  };
+  const cancelCurrentRequests = () => {
+    _.each(currentRequests, promise => {
+      promise.cancel();
+    });
+  };
 
   // prefix all links in the app with `pathPrefix`
   const ConduitLink = props => {
@@ -113,6 +156,11 @@ export const ApiHelpers = (pathPrefix, defaultMetricsWindow = '10m') => {
     getValidMetricsWindows: () => _.keys(validMetricsWindows),
     getMetricsWindowDisplayText,
     urlsForResource: urlsForResource,
-    ConduitLink
+    ConduitLink,
+    setCurrentRequests,
+    getCurrentPromises,
+    cancelCurrentRequests,
+    // DO NOT USE makeCancelable, use fetch, this is only exposed for testing
+    makeCancelable
   };
 };

web/app/test/ApiHelpersTest.jsx

@@ -1,4 +1,5 @@
 /* eslint-disable */
+import _ from 'lodash';
 import 'raf/polyfill'; // the polyfill import must be first
 import Adapter from 'enzyme-adapter-react-16';
 import { ApiHelpers } from '../js/components/util/ApiHelpers.jsx';
@@ -22,7 +23,7 @@ describe('ApiHelpers', () => {
       ok: true,
       json: () => Promise.resolve({ metrics: [] })
     });
-    api = ApiHelpers("");
+    api = ApiHelpers('');
   });
 
   afterEach(() => {
@@ -88,6 +89,71 @@ describe('ApiHelpers', () => {
     });
   });
 
+  describe('makeCancelable', () => {
+    it('wraps the original promise', () => {
+      let p = Promise.resolve({ result: 'my response', ok: true });
+      let cancelablePromise = api.makeCancelable(p);
+
+      return cancelablePromise.promise
+        .then(resp => {
+          expect(resp.result).to.equal('my response');
+        });
+    });
+
+    it('returns an error on original promise rejection', () => {
+      let p = Promise.reject({ rejectionReason: 'it is bad' });
+      let cancelablePromise = api.makeCancelable(p);
+
+      return cancelablePromise.promise
+        .then(() => {
+          return Promise.reject('Expected method to reject.');
+        })
+        .catch(e => {
+          expect(e).to.deep.equal({ rejectionReason: 'it is bad' });
+        });
+    });
+
+    it('returns an error if the fetch did not go well', () => {
+      let reason = { rejectionReason: 'it is bad' };
+      let p = Promise.reject(reason);
+      let cancelablePromise = api.makeCancelable(p);
+
+      return cancelablePromise.promise
+        .then(() => {
+          return Promise.reject('Expected method to reject.');
+        })
+        .catch(e => {
+          expect(e).to.deep.equal(reason);
+        });
+    });
+
+    it('calls the provided success handler on response success', () => {
+      let onSuccess = sinon.spy();
+      let fakeFetchResults = { result: 5, ok: true };
+      let p = Promise.resolve(fakeFetchResults);
+      let cancelablePromise = api.makeCancelable(p, onSuccess);
+
+      return cancelablePromise.promise
+        .then(() => {
+          expect(onSuccess.calledOnce).to.be.true;
+          expect(onSuccess.args[0][0]).to.deep.equal(fakeFetchResults);
+        });
+    });
+
+    it('allows you to cancel a promise', () => {
+      let p = Promise.resolve({ result: 'my response', ok: true });
+      let cancelablePromise = api.makeCancelable(p);
+      cancelablePromise.cancel();
+
+      return cancelablePromise.promise
+        .then(() => {
+          return Promise.reject('Expected method to reject.');
+        }).catch(resp => {
+          expect(resp.isCanceled).to.be.true;
+        });
+    });
+  });
+
   describe('fetch', () => {
     it('adds pathPrefix to a metrics request', () => {
       api = ApiHelpers('/the/path/prefix');
@@ -115,11 +181,38 @@ describe('ApiHelpers', () => {
       api = ApiHelpers('');
       let errorHandler = sinon.spy();
 
-      api.fetch('/resource/foo')
-        .catch(errorHandler);
+      let f = api.fetch('/resource/foo');
 
-      expect(errorHandler.args[0][0].message).to.equal(errorMessage);
-      expect(errorHandler.calledOnce).to.be.true;
+      return f.promise
+        .then(() => {
+          return Promise.reject('Expected method to reject.');
+        }, errorHandler)
+        .then(() => {
+          expect(errorHandler.args[0][0].message).to.equal(errorMessage);
+          expect(errorHandler.calledOnce).to.be.true;
+        });
+    });
+
+    it('correctly passes through rejection messages', () => {
+      let rejectionMessage = "hm, an error";
+      fetchStub.returnsPromise().rejects({
+        myReason: rejectionMessage
+      });
+
+      api = ApiHelpers('');
+      let rejectHandler = sinon.spy();
+
+      let f = api.fetch('/resource/foo');
+
+      return f.promise
+        .then(() => {
+          return Promise.reject('Expected method to reject.');
+        }, rejectHandler)
+        .then(() => {
+          expect(rejectHandler.args[0][0]).to.have.own.property('myReason');
+          expect(rejectHandler.args[0][0].myReason).to.equal(rejectionMessage);
+          expect(rejectHandler.calledOnce).to.be.true;
+        });
     });
   });