Fix canceling diff processor/syntax layer
The previous implementation has two bugs:
1) When calling process() and then cancel() before then continuation
runs on a microtask, the ongoing work is not actually cancelled. This
can happen at least in the case of loading highlightjs, but probably
also in other cases.
2) When cancel() is called, the promise returned by promise() is
currenty not rejected, potentially dangling there forever and leaking
memory.
Both are fixed in this change by canceling the promise when cancel() is
called.
Change-Id: Ieb0f1fcccd2cda3b2f9648347bc4e4311eb07a06
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index 29cc3b9..9646b06 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -38,6 +38,7 @@
<gr-reporting id="reporting"></gr-reporting>
<gr-js-api-interface id="jsAPI"></gr-js-api-interface>
</template>
+ <script src="../../../scripts/util.js"></script>
<script src="../gr-diff/gr-diff-line.js"></script>
<script src="../gr-diff/gr-diff-group.js"></script>
<script src="../gr-diff-highlight/gr-annotation.js"></script>
@@ -108,6 +109,13 @@
type: Array,
value: () => [],
},
+ /**
+ * The promise last returned from `render()` while the asynchronous
+ * rendering is running - `null` otherwise. Provides a `cancel()`
+ * method that rejects it with `{isCancelled: true}`.
+ * @type {?Object}
+ */
+ _cancelableRenderPromise: Object,
},
get diffElement() {
@@ -146,25 +154,32 @@
reporting.time(TimingLabel.TOTAL);
reporting.time(TimingLabel.CONTENT);
this.dispatchEvent(new CustomEvent('render-start', {bubbles: true}));
- return this.$.processor.process(this.diff.content, isBinary)
- .then(() => {
- if (this.isImageDiff) {
- this._builder.renderDiff();
- }
- this.dispatchEvent(new CustomEvent('render-content',
- {bubbles: true}));
+ this._cancelableRenderPromise = util.makeCancelable(
+ this.$.processor.process(this.diff.content, isBinary)
+ .then(() => {
+ if (this.isImageDiff) {
+ this._builder.renderDiff();
+ }
+ this.dispatchEvent(new CustomEvent('render-content',
+ {bubbles: true}));
- if (this._diffTooLargeForSyntax()) {
- this.$.syntaxLayer.enabled = false;
- }
+ if (this._diffTooLargeForSyntax()) {
+ this.$.syntaxLayer.enabled = false;
+ }
- reporting.timeEnd(TimingLabel.CONTENT);
- reporting.time(TimingLabel.SYNTAX);
- return this.$.syntaxLayer.process().then(() => {
- reporting.timeEnd(TimingLabel.SYNTAX);
- reporting.timeEnd(TimingLabel.TOTAL);
- });
- });
+ reporting.timeEnd(TimingLabel.CONTENT);
+ reporting.time(TimingLabel.SYNTAX);
+ return this.$.syntaxLayer.process().then(() => {
+ reporting.timeEnd(TimingLabel.SYNTAX);
+ reporting.timeEnd(TimingLabel.TOTAL);
+ });
+ }));
+ return this._cancelableRenderPromise
+ .finally(() => { this._cancelableRenderPromise = null; })
+ // Mocca testing does not like uncaught rejections, so we catch
+ // the cancels which are expected and should not throw errors in
+ // tests.
+ .catch(e => { if (!e.isCanceled) return Promise.reject(e); });
},
_setupAnnotationLayers() {
@@ -259,6 +274,10 @@
cancel() {
this.$.processor.cancel();
this.$.syntaxLayer.cancel();
+ if (this._cancelableRenderPromise) {
+ this._cancelableRenderPromise.cancel();
+ this._cancelableRenderPromise = null;
+ }
},
_handlePreferenceError(pref) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.html b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.html
index baa8bba..663cf25 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.html
@@ -20,5 +20,7 @@
<dom-module id="gr-diff-processor">
<script src="../gr-diff/gr-diff-line.js"></script>
<script src="../gr-diff/gr-diff-group.js"></script>
+
+ <script src="../../../scripts/util.js"></script>
<script src="gr-diff-processor.js"></script>
</dom-module>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
index dead8d7..a9268a7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
@@ -80,8 +80,18 @@
value: 64,
},
- /** @type {number|undefined} */
+ /** @type {?number} */
_nextStepHandle: Number,
+ /**
+ * The promise last returned from `process()` while the asynchronous
+ * processing is running - `null` otherwise. Provides a `cancel()`
+ * method that rejects it with `{isCancelled: true}`.
+ * @type {?Object}
+ */
+ _processPromise: {
+ type: Object,
+ value: null,
+ },
_isScrolling: Boolean,
},
@@ -108,6 +118,10 @@
* processed.
*/
process(content, isBinary) {
+ // Cancel any still running process() calls, because they append to the
+ // same groups field.
+ this.cancel();
+
this.groups = [];
this.push('groups', this._makeFileComments());
@@ -115,57 +129,64 @@
// so finish processing.
if (isBinary) { return Promise.resolve(); }
- return new Promise(resolve => {
- const state = {
- lineNums: {left: 0, right: 0},
- sectionIndex: 0,
- };
- content = this._splitCommonGroupsWithComments(content);
+ this._processPromise = util.makeCancelable(
+ new Promise(resolve => {
+ const state = {
+ lineNums: {left: 0, right: 0},
+ sectionIndex: 0,
+ };
- let currentBatch = 0;
- const nextStep = () => {
- if (this._isScrolling) {
- this.async(nextStep, 100);
- return;
- }
- // If we are done, resolve the promise.
- if (state.sectionIndex >= content.length) {
- resolve(this.groups);
- this._nextStepHandle = undefined;
- return;
- }
+ content = this._splitCommonGroupsWithComments(content);
- // Process the next section and incorporate the result.
- const result = this._processNext(state, content);
- for (const group of result.groups) {
- this.push('groups', group);
- currentBatch += group.lines.length;
- }
- state.lineNums.left += result.lineDelta.left;
- state.lineNums.right += result.lineDelta.right;
+ let currentBatch = 0;
+ const nextStep = () => {
+ if (this._isScrolling) {
+ this.async(nextStep, 100);
+ return;
+ }
+ // If we are done, resolve the promise.
+ if (state.sectionIndex >= content.length) {
+ resolve(this.groups);
+ this._nextStepHandle = null;
+ return;
+ }
- // Increment the index and recurse.
- state.sectionIndex++;
- if (currentBatch >= this._asyncThreshold) {
- currentBatch = 0;
- this._nextStepHandle = this.async(nextStep, 1);
- } else {
+ // Process the next section and incorporate the result.
+ const result = this._processNext(state, content);
+ for (const group of result.groups) {
+ this.push('groups', group);
+ currentBatch += group.lines.length;
+ }
+ state.lineNums.left += result.lineDelta.left;
+ state.lineNums.right += result.lineDelta.right;
+
+ // Increment the index and recurse.
+ state.sectionIndex++;
+ if (currentBatch >= this._asyncThreshold) {
+ currentBatch = 0;
+ this._nextStepHandle = this.async(nextStep, 1);
+ } else {
+ nextStep.call(this);
+ }
+ };
+
nextStep.call(this);
- }
- };
-
- nextStep.call(this);
- });
+ }));
+ return this._processPromise
+ .finally(() => { this._processPromise = null; });
},
/**
* Cancel any jobs that are running.
*/
cancel() {
- if (this._nextStepHandle !== undefined) {
+ if (this._nextStepHandle != null) {
this.cancelAsync(this._nextStepHandle);
- this._nextStepHandle = undefined;
+ this._nextStepHandle = null;
+ }
+ if (this._processPromise) {
+ this._processPromise.cancel();
}
},
diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html
index 017cd5d..67c32bb 100644
--- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html
+++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html
@@ -21,6 +21,7 @@
<template>
<gr-lib-loader id="libLoader"></gr-lib-loader>
</template>
+ <script src="../../../scripts/util.js"></script>
<script src="../gr-diff/gr-diff-line.js"></script>
<script src="../gr-diff-highlight/gr-annotation.js"></script>
<script src="gr-syntax-layer.js"></script>
diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
index a7e7377..32eb969 100644
--- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
+++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
@@ -155,6 +155,16 @@
},
/** @type {?number} */
_processHandle: Number,
+ /**
+ * The promise last returned from `process()` while the asynchronous
+ * processing is running - `null` otherwise. Provides a `cancel()`
+ * method that rejects it with `{isCancelled: true}`.
+ * @type {?Object}
+ */
+ _processPromise: {
+ type: Object,
+ value: null,
+ },
_hljs: Object,
},
@@ -212,6 +222,10 @@
* @return {Promise}
*/
process() {
+ // Cancel any still running process() calls, because they append to the
+ // same _baseRanges and _revisionRanges fields.
+ this.cancel();
+
// Discard existing ranges.
this._baseRanges = [];
this._revisionRanges = [];
@@ -220,8 +234,6 @@
return Promise.resolve();
}
- this.cancel();
-
if (this.diff.meta_a) {
this._baseLanguage = this._getLanguage(this.diff.meta_a);
}
@@ -241,49 +253,55 @@
lastNotify: {left: 1, right: 1},
};
- return this._loadHLJS().then(() => {
- return new Promise(resolve => {
- const nextStep = () => {
- this._processHandle = null;
- this._processNextLine(state);
+ this._processPromise = util.makeCancelable(this._loadHLJS()
+ .then(() => {
+ return new Promise(resolve => {
+ const nextStep = () => {
+ this._processHandle = null;
+ this._processNextLine(state);
- // Move to the next line in the section.
- state.lineIndex++;
+ // Move to the next line in the section.
+ state.lineIndex++;
- // If the section has been exhausted, move to the next one.
- if (this._isSectionDone(state)) {
- state.lineIndex = 0;
- state.sectionIndex++;
- }
+ // If the section has been exhausted, move to the next one.
+ if (this._isSectionDone(state)) {
+ state.lineIndex = 0;
+ state.sectionIndex++;
+ }
- // If all sections have been exhausted, finish.
- if (state.sectionIndex >= this.diff.content.length) {
- resolve();
- this._notify(state);
- return;
- }
+ // If all sections have been exhausted, finish.
+ if (state.sectionIndex >= this.diff.content.length) {
+ resolve();
+ this._notify(state);
+ return;
+ }
- if (state.lineIndex % 100 === 0) {
- this._notify(state);
- this._processHandle = this.async(nextStep, ASYNC_DELAY);
- } else {
- nextStep.call(this);
- }
- };
+ if (state.lineIndex % 100 === 0) {
+ this._notify(state);
+ this._processHandle = this.async(nextStep, ASYNC_DELAY);
+ } else {
+ nextStep.call(this);
+ }
+ };
- this._processHandle = this.async(nextStep, 1);
- });
- });
+ this._processHandle = this.async(nextStep, 1);
+ });
+ }));
+ return this._processPromise
+ .finally(() => { this._processPromise = null; });
},
/**
* Cancel any asynchronous syntax processing jobs.
*/
cancel() {
- if (this._processHandle) {
+ if (this._processHandle != null) {
this.cancelAsync(this._processHandle);
this._processHandle = null;
}
+ if (this._processPromise) {
+ this._processPromise.cancel();
+ }
},
_diffChanged() {
diff --git a/polygerrit-ui/app/scripts/util.js b/polygerrit-ui/app/scripts/util.js
index b4ab21a..624992b 100644
--- a/polygerrit-ui/app/scripts/util.js
+++ b/polygerrit-ui/app/scripts/util.js
@@ -41,5 +41,39 @@
}
return '';
};
+
+ /**
+ * Make the promise cancelable.
+ *
+ * Returns a promise with a `cancel()` method wrapped around `promise`.
+ * Calling `cancel()` will reject the returned promise with
+ * {isCancelled: true} synchronously. If the inner promise for a cancelled
+ * promise resolves or rejects this is ignored.
+ */
+ util.makeCancelable = promise => {
+ // True if the promise is either resolved or reject (possibly cancelled)
+ let isDone = false;
+
+ let rejectPromise;
+
+ const wrappedPromise = new Promise((resolve, reject) => {
+ rejectPromise = reject;
+ promise.then(val => {
+ if (!isDone) resolve(val);
+ isDone = true;
+ }, error => {
+ if (!isDone) reject(error);
+ isDone = true;
+ });
+ });
+
+ wrappedPromise.cancel = () => {
+ if (isDone) return;
+ rejectPromise({isCanceled: true});
+ isDone = true;
+ };
+ return wrappedPromise;
+ };
+
window.util = util;
})(window);
