Let the new syntax highlighting layer properly deal with cancellation Release-Notes: skip Google-Bug-Id: b/219684547 Change-Id: I7230475686d6c24801d851f9733ffa53bf35e36d
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts index 9d2a819..85f8ce0 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -98,10 +98,8 @@ import {distinctUntilChanged, map} from 'rxjs/operators'; import {deepEqual} from '../../../utils/deep-util'; import {Category} from '../../../api/checks'; -import { - GrSyntaxLayerWorker, - CODE_MAX_LINES, -} from '../../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker'; +import {GrSyntaxLayerWorker} from '../../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker'; +import {CODE_MAX_LINES} from '../../../services/highlight/highlight-service'; const EMPTY_BLAME = 'No blame information for this diff.';
diff --git a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts index 59ce396..82dfb35 100644 --- a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts +++ b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts
@@ -10,6 +10,7 @@ import {Side} from '../../../constants/constants'; import {getAppContext} from '../../../services/app-context'; import {SyntaxLayerLine} from '../../../types/syntax-worker-api'; +import {CancelablePromise, util} from '../../../scripts/util'; const LANGUAGE_MAP = new Map<string, string>([ ['application/dart', 'dart'], @@ -125,29 +126,35 @@ 'variable', ]); -/** - * Safe guard for not killing the browser. - */ -export const CODE_MAX_LINES = 20 * 1000; - -/** - * Safe guard for not killing the browser. Maximum in number of chars. - */ -const CODE_MAX_LENGTH = 25 * CODE_MAX_LINES; - export class GrSyntaxLayerWorker implements DiffLayer { diff?: DiffInfo; enabled = true; - private leftRanges: SyntaxLayerLine[] = []; + // private, but visible for testing + leftRanges: SyntaxLayerLine[] = []; - private rightRanges: SyntaxLayerLine[] = []; + // private, but visible for testing + rightRanges: SyntaxLayerLine[] = []; + + /** + * We are keeping a reference around to the async computation, such that we + * can cancel it, if needed. + */ + private leftPromise?: CancelablePromise<SyntaxLayerLine[]>; + + /** + * We are keeping a reference around to the async computation, such that we + * can cancel it, if needed. + */ + private rightPromise?: CancelablePromise<SyntaxLayerLine[]>; private listeners: DiffLayerListener[] = []; private readonly highlightService = getAppContext().highlightService; + private readonly reportingService = getAppContext().reportingService; + setEnabled(enabled: boolean) { this.enabled = enabled; } @@ -220,6 +227,10 @@ this.diff = diff; this.leftRanges = []; this.rightRanges = []; + if (this.leftPromise) this.leftPromise.cancel(); + if (this.rightPromise) this.rightPromise.cancel(); + this.leftPromise = undefined; + this.rightPromise = undefined; if (!this.enabled || !this.diff) return; const leftLanguage = this._getLanguage(this.diff.meta_a); @@ -245,17 +256,26 @@ leftContent = leftContent.trimEnd(); rightContent = rightContent.trimEnd(); - const leftPromise = this.highlight(leftLanguage, leftContent); - const rightPromise = this.highlight(rightLanguage, rightContent); - this.leftRanges = await leftPromise; - this.rightRanges = await rightPromise; - this.notify(); + try { + this.leftPromise = this.highlight(leftLanguage, leftContent); + this.rightPromise = this.highlight(rightLanguage, rightContent); + this.leftRanges = await this.leftPromise; + this.rightRanges = await this.rightPromise; + this.notify(); + } catch (err) { + if (!err.isCanceled) this.reportingService.error(err); + // One source of "error" can promise cancelation. + this.leftRanges = []; + this.rightRanges = []; + } } - async highlight(language?: string, code?: string) { - if (!language || !code) return []; - if (code.length > CODE_MAX_LENGTH) return []; - return this.highlightService.highlight(language, code); + highlight( + language?: string, + code?: string + ): CancelablePromise<SyntaxLayerLine[]> { + const hlPromise = this.highlightService.highlight(language, code); + return util.makeCancelable(hlPromise); } notify() {
diff --git a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts index c9e70ad..ee82d5d 100644 --- a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts +++ b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker_test.ts
@@ -5,6 +5,7 @@ */ import {DiffInfo, GrDiffLineType, Side} from '../../../api/diff'; import '../../../test/common-test-setup-karma'; +import {mockPromise, stubHighlightService} from '../../../test/test-utils'; import {SyntaxLayerLine} from '../../../types/syntax-worker-api'; import {GrDiffLine} from '../gr-diff/gr-diff-line'; import {GrSyntaxLayerWorker} from './gr-syntax-layer-worker'; @@ -75,40 +76,78 @@ setup(() => { layer = new GrSyntaxLayerWorker(); - listener = sinon.stub(); - layer.addListener(listener); - sinon.stub(layer, 'highlight').callsFake((lang?: string) => { - if (lang === 'lang-left') return Promise.resolve(leftRanges); - if (lang === 'lang-right') return Promise.resolve(rightRanges); - return Promise.resolve([]); + }); + + test('cancel processing', async () => { + const mockPromise1 = mockPromise<SyntaxLayerLine[]>(); + const mockPromise2 = mockPromise<SyntaxLayerLine[]>(); + const mockPromise3 = mockPromise<SyntaxLayerLine[]>(); + const mockPromise4 = mockPromise<SyntaxLayerLine[]>(); + const stub = stubHighlightService('highlight'); + stub.onCall(0).returns(mockPromise1); + stub.onCall(1).returns(mockPromise2); + stub.onCall(2).returns(mockPromise3); + stub.onCall(3).returns(mockPromise4); + + const processPromise1 = layer.process(diff); + // Calling the process() a second time means that the promises created + // during the first call are cancelled. + const processPromise2 = layer.process(diff); + // We can await the outer promise even before the inner promises resolve, + // because cancelling rejects the inner promises. + await processPromise1; + // It does not matter actually, whether these two inner promises are + // resolved or not. + mockPromise1.resolve(leftRanges); + mockPromise2.resolve(rightRanges); + // Both ranges must still be empty, because the promise of the first call + // must have been cancelled and the returned ranges ignored. + assert.isEmpty(layer.leftRanges); + assert.isEmpty(layer.rightRanges); + // Lets' resolve and await the promises of the second as normal. + mockPromise3.resolve(leftRanges); + mockPromise4.resolve(rightRanges); + await processPromise2; + assert.equal(layer.leftRanges, leftRanges); + }); + + suite('annotate and listen', () => { + setup(() => { + listener = sinon.stub(); + layer.addListener(listener); + stubHighlightService('highlight').callsFake((lang?: string) => { + if (lang === 'lang-left') return Promise.resolve(leftRanges); + if (lang === 'lang-right') return Promise.resolve(rightRanges); + return Promise.resolve([]); + }); }); - }); - test('process and annotate line 2 LEFT', async () => { - await layer.process(diff); - const el = annotate(Side.LEFT, 1, 'import it;'); - assert.equal( - el.innerHTML, - '<hl class="gr-diff gr-syntax gr-syntax-literal">import</hl> it;' - ); - assert.equal(listener.callCount, 2); - assert.equal(listener.getCall(0).args[0], 1); - assert.equal(listener.getCall(0).args[1], 1); - assert.equal(listener.getCall(0).args[2], Side.LEFT); - assert.equal(listener.getCall(1).args[0], 3); - assert.equal(listener.getCall(1).args[1], 3); - assert.equal(listener.getCall(1).args[2], Side.RIGHT); - }); + test('process and annotate line 2 LEFT', async () => { + await layer.process(diff); + const el = annotate(Side.LEFT, 1, 'import it;'); + assert.equal( + el.innerHTML, + '<hl class="gr-diff gr-syntax gr-syntax-literal">import</hl> it;' + ); + assert.equal(listener.callCount, 2); + assert.equal(listener.getCall(0).args[0], 1); + assert.equal(listener.getCall(0).args[1], 1); + assert.equal(listener.getCall(0).args[2], Side.LEFT); + assert.equal(listener.getCall(1).args[0], 3); + assert.equal(listener.getCall(1).args[1], 3); + assert.equal(listener.getCall(1).args[2], Side.RIGHT); + }); - test('process and annotate line 3 RIGHT', async () => { - await layer.process(diff); - const el = annotate(Side.RIGHT, 3, ' public static final {'); - assert.equal( - el.innerHTML, - ' <hl class="gr-diff gr-syntax gr-syntax-literal">public</hl> ' + - '<hl class="gr-diff gr-syntax gr-syntax-keyword">static</hl> ' + - '<hl class="gr-diff gr-syntax gr-syntax-name">final</hl> {' - ); - assert.equal(listener.callCount, 2); + test('process and annotate line 3 RIGHT', async () => { + await layer.process(diff); + const el = annotate(Side.RIGHT, 3, ' public static final {'); + assert.equal( + el.innerHTML, + ' <hl class="gr-diff gr-syntax gr-syntax-literal">public</hl> ' + + '<hl class="gr-diff gr-syntax gr-syntax-keyword">static</hl> ' + + '<hl class="gr-diff gr-syntax gr-syntax-name">final</hl> {' + ); + assert.equal(listener.callCount, 2); + }); }); });
diff --git a/polygerrit-ui/app/services/highlight/highlight-service.ts b/polygerrit-ui/app/services/highlight/highlight-service.ts index 3dd3cf1..80da260 100644 --- a/polygerrit-ui/app/services/highlight/highlight-service.ts +++ b/polygerrit-ui/app/services/highlight/highlight-service.ts
@@ -30,6 +30,16 @@ const WORKER_POOL_SIZE = 3; /** + * Safe guard for not killing the browser. + */ +export const CODE_MAX_LINES = 20 * 1000; + +/** + * Safe guard for not killing the browser. Maximum in number of chars. + */ +const CODE_MAX_LENGTH = 25 * CODE_MAX_LINES; + +/** * Service for syntax highlighting. Maintains some HighlightJS workers doing * their job in the background. */ @@ -115,7 +125,12 @@ if (resolver) resolver(result.ranges ?? []); } - async highlight(language: string, code: string): Promise<SyntaxLayerLine[]> { + async highlight( + language?: string, + code?: string + ): Promise<SyntaxLayerLine[]> { + if (!language || !code) return []; + if (code.length > CODE_MAX_LENGTH) return []; const worker = await this.requestWorker(); const message: SyntaxWorkerRequest = { type: SyntaxWorkerMessageType.REQUEST,
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts index f0c8a84..0d9cf3c 100644 --- a/polygerrit-ui/app/test/test-utils.ts +++ b/polygerrit-ui/app/test/test-utils.ts
@@ -30,6 +30,7 @@ import {Key, Modifier} from '../utils/dom-util'; import {Observable} from 'rxjs'; import {filter, take, timeout} from 'rxjs/operators'; +import {HighlightService} from '../services/highlight/highlight-service'; export {query, queryAll, queryAndAssert} from '../utils/common-util'; export interface MockPromise<T> extends Promise<T> { @@ -121,6 +122,12 @@ return sinon.stub(getAppContext().shortcutsService, method); } +export function stubHighlightService<K extends keyof HighlightService>( + method: K +) { + return sinon.stub(getAppContext().highlightService, method); +} + export function stubStorage<K extends keyof StorageService>(method: K) { return sinon.stub(getAppContext().storageService, method); }