opendev/gerrit
Code Issues Proposed changes

Fix canceling diff processor/syntax layer

Browse Source 
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
This commit is contained in:
Ole Rehmsen
2019-03-07 17:28:03 +01:00
parent 7a82815691
commit 4c25dfcfd9
6 changed files with 183 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
View File

@@ -38,6 +38,7 @@ limitations under the License.
<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 @@ 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) {

2
polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.html
View File

@@ -20,5 +20,7 @@ limitations under the License.
<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>

101
polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
View File

@@ -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();
}
},

1
polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html
View File

@@ -21,6 +21,7 @@ limitations under the License.
<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>

80
polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
View File

@@ -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() {

34
polygerrit-ui/app/scripts/util.js
View File

@@ -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);