From 4c25dfcfd9d859c06e93a7c4c0b19085777caa3f Mon Sep 17 00:00:00 2001 From: Ole Rehmsen Date: Thu, 7 Mar 2019 17:28:03 +0100 Subject: [PATCH] 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/gr-diff-builder/gr-diff-builder.html | 53 ++++++--- .../gr-diff-processor/gr-diff-processor.html | 2 + .../gr-diff-processor/gr-diff-processor.js | 101 +++++++++++------- .../diff/gr-syntax-layer/gr-syntax-layer.html | 1 + .../diff/gr-syntax-layer/gr-syntax-layer.js | 80 ++++++++------ polygerrit-ui/app/scripts/util.js | 34 ++++++ 6 files changed, 183 insertions(+), 88 deletions(-) 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 29cc3b9ed5..9646b06bd5 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 @@ limitations under the License. + @@ -108,6 +109,13 @@ limitations under the License. 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 @@ limitations under the License. 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 @@ limitations under the License. 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 baa8bba009..663cf258a6 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 @@ limitations under the License. + + 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 dead8d75f2..a9268a7882 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; + } + + // 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); + } + }; - // 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); - }); + })); + 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 017cd5d5b5..67c32bb641 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 @@ limitations under the License. + 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 a7e73779bc..32eb969f75 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 b4ab21a616..624992b3e4 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);