From 3b896ae4d83606d99b7d4cc8d810366609234d24 Mon Sep 17 00:00:00 2001 From: Christof Marti Date: Thu, 15 Sep 2016 17:06:36 -0700 Subject: [PATCH] Fixes #11181: Wait for any running file search before spawning the next --- src/vs/test/utils/promiseTestUtils.ts | 33 ++- .../parts/search/browser/openFileHandler.ts | 51 +++-- .../test/browser/openFileHandler.test.ts | 203 ++++++++++++++++++ 3 files changed, 270 insertions(+), 17 deletions(-) create mode 100644 src/vs/workbench/parts/search/test/browser/openFileHandler.test.ts diff --git a/src/vs/test/utils/promiseTestUtils.ts b/src/vs/test/utils/promiseTestUtils.ts index 4096e44e0fc..31742c8467f 100644 --- a/src/vs/test/utils/promiseTestUtils.ts +++ b/src/vs/test/utils/promiseTestUtils.ts @@ -3,15 +3,40 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { TPromise, PPromise, TValueCallback, TProgressCallback } from 'vs/base/common/winjs.base'; +import { TPromise, PPromise, TValueCallback, TProgressCallback, ProgressCallback } from 'vs/base/common/winjs.base'; import errors = require('vs/base/common/errors'); -export class DeferredTPromise extends TPromise { +export class DeferredTPromise extends TPromise { - constructor(init:(complete: TValueCallback, error:(err:any)=>void)=>void) { - super((c, e) => setTimeout(() => init(c, e))); + public canceled = false; + + private completeCallback: TValueCallback; + private errorCallback: (err:any)=>void; + private progressCallback: ProgressCallback; + + constructor() { + super((c, e, p) => { + this.completeCallback= c; + this.errorCallback= e; + this.progressCallback= p; + }, () => this.oncancel()); } + public complete(value: T) { + this.completeCallback(value); + } + + public error(err: any) { + this.errorCallback(err); + } + + public progress(p: any) { + this.progressCallback(p); + } + + private oncancel(): void { + this.canceled = true; + } } export class DeferredPPromise extends PPromise { diff --git a/src/vs/workbench/parts/search/browser/openFileHandler.ts b/src/vs/workbench/parts/search/browser/openFileHandler.ts index a505e8b70e2..e54cfbd9d35 100644 --- a/src/vs/workbench/parts/search/browser/openFileHandler.ts +++ b/src/vs/workbench/parts/search/browser/openFileHandler.ts @@ -201,13 +201,23 @@ export class OpenFileHandler extends QuickOpenHandler { } } -class CacheState { +enum LoadingPhase { + Created = 1, + Loading, + Loaded, + Errored, + Disposed +} - public query: ISearchQuery; +/** + * Exported for testing. + */ +export class CacheState { private _cacheKey = uuid.generateUuid(); - private _isLoaded = false; + private query: ISearchQuery; + private loadingPhase = LoadingPhase.Created; private promise: TPromise; constructor(private cacheQuery: (cacheKey: string) => ISearchQuery, private doLoad: (query: ISearchQuery) => TPromise, private doDispose: (cacheKey: string) => TPromise, private previous: CacheState) { @@ -223,34 +233,49 @@ class CacheState { } public get cacheKey(): string { - return this._isLoaded || !this.previous ? this._cacheKey : this.previous.cacheKey; + return this.loadingPhase === LoadingPhase.Loaded || !this.previous ? this._cacheKey : this.previous.cacheKey; } public get isLoaded(): boolean { - return this._isLoaded || !this.previous ? this._isLoaded : this.previous.isLoaded; + const isLoaded = this.loadingPhase === LoadingPhase.Loaded; + return isLoaded || !this.previous ? isLoaded : this.previous.isLoaded; + } + + public get isUpdating(): boolean { + const isUpdating = this.loadingPhase === LoadingPhase.Loading; + return isUpdating || !this.previous ? isUpdating : this.previous.isUpdating; } public load(): void { + if (this.isUpdating) { + return; + } + this.loadingPhase = LoadingPhase.Loading; this.promise = this.doLoad(this.query) .then(() => { - this._isLoaded = true; + this.loadingPhase = LoadingPhase.Loaded; if (this.previous) { this.previous.dispose(); this.previous = null; } }, err => { + this.loadingPhase = LoadingPhase.Errored; errors.onUnexpectedError(err); }); } public dispose(): void { - this.promise.then(null, () => { }) - .then(() => { - this._isLoaded = false; - return this.doDispose(this._cacheKey); - }).then(null, err => { - errors.onUnexpectedError(err); - }); + if (this.promise) { + this.promise.then(null, () => { }) + .then(() => { + this.loadingPhase = LoadingPhase.Disposed; + return this.doDispose(this._cacheKey); + }).then(null, err => { + errors.onUnexpectedError(err); + }); + } else { + this.loadingPhase = LoadingPhase.Disposed; + } if (this.previous) { this.previous.dispose(); this.previous = null; diff --git a/src/vs/workbench/parts/search/test/browser/openFileHandler.test.ts b/src/vs/workbench/parts/search/test/browser/openFileHandler.test.ts new file mode 100644 index 00000000000..af9970bd3d2 --- /dev/null +++ b/src/vs/workbench/parts/search/test/browser/openFileHandler.test.ts @@ -0,0 +1,203 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ +'use strict'; + +import * as assert from 'assert'; +import * as errors from 'vs/base/common/errors'; +import * as objects from 'vs/base/common/objects'; +import {TPromise} from 'vs/base/common/winjs.base'; +import {CacheState} from 'vs/workbench/parts/search/browser/openFileHandler'; +import {DeferredTPromise} from 'vs/test/utils/promiseTestUtils'; +import {QueryType, ISearchQuery} from 'vs/platform/search/common/search'; + +suite('CacheState', () => { + + test('reuse old cacheKey until new cache is loaded', function () { + + const cache = new MockCache(); + + const first = createCacheState(cache); + const firstKey = first.cacheKey; + assert.strictEqual(first.isLoaded, false); + assert.strictEqual(first.isUpdating, false); + + first.load(); + assert.strictEqual(first.isLoaded, false); + assert.strictEqual(first.isUpdating, true); + + cache.loading[firstKey].complete(null); + assert.strictEqual(first.isLoaded, true); + assert.strictEqual(first.isUpdating, false); + + const second = createCacheState(cache, first); + second.load(); + assert.strictEqual(second.isLoaded, true); + assert.strictEqual(second.isUpdating, true); + assert.strictEqual(Object.keys(cache.disposing).length, 0); + assert.strictEqual(second.cacheKey, firstKey); // still using old cacheKey + + const secondKey = cache.cacheKeys[1]; + cache.loading[secondKey].complete(null); + assert.strictEqual(second.isLoaded, true); + assert.strictEqual(second.isUpdating, false); + assert.strictEqual(Object.keys(cache.disposing).length, 1); + assert.strictEqual(second.cacheKey, secondKey); + }); + + test('do not spawn additional load if previous is still loading', function () { + + const cache = new MockCache(); + + const first = createCacheState(cache); + const firstKey = first.cacheKey; + first.load(); + assert.strictEqual(first.isLoaded, false); + assert.strictEqual(first.isUpdating, true); + assert.strictEqual(Object.keys(cache.loading).length, 1); + + const second = createCacheState(cache, first); + second.load(); + assert.strictEqual(second.isLoaded, false); + assert.strictEqual(second.isUpdating, true); + assert.strictEqual(cache.cacheKeys.length, 2); + assert.strictEqual(Object.keys(cache.loading).length, 1); // still only one loading + assert.strictEqual(second.cacheKey, firstKey); + + cache.loading[firstKey].complete(null); + assert.strictEqual(second.isLoaded, true); + assert.strictEqual(second.isUpdating, false); + assert.strictEqual(Object.keys(cache.disposing).length, 0); + }); + + test('do not use previous cacheKey if query changed', function () { + + const cache = new MockCache(); + + const first = createCacheState(cache); + const firstKey = first.cacheKey; + first.load(); + cache.loading[firstKey].complete(null); + assert.strictEqual(first.isLoaded, true); + assert.strictEqual(first.isUpdating, false); + assert.strictEqual(Object.keys(cache.disposing).length, 0); + + cache.baseQuery.excludePattern = { '**/node_modules': true }; + const second = createCacheState(cache, first); + assert.strictEqual(second.isLoaded, false); + assert.strictEqual(second.isUpdating, false); + assert.strictEqual(Object.keys(cache.disposing).length, 1); + + second.load(); + assert.strictEqual(second.isLoaded, false); + assert.strictEqual(second.isUpdating, true); + assert.notStrictEqual(second.cacheKey, firstKey); // not using old cacheKey + const secondKey = cache.cacheKeys[1]; + assert.strictEqual(second.cacheKey, secondKey); + + cache.loading[secondKey].complete(null); + assert.strictEqual(second.isLoaded, true); + assert.strictEqual(second.isUpdating, false); + assert.strictEqual(Object.keys(cache.disposing).length, 1); + }); + + test('dispose propagates', function () { + + const cache = new MockCache(); + + const first = createCacheState(cache); + const firstKey = first.cacheKey; + first.load(); + cache.loading[firstKey].complete(null); + const second = createCacheState(cache, first); + assert.strictEqual(second.isLoaded, true); + assert.strictEqual(second.isUpdating, false); + assert.strictEqual(Object.keys(cache.disposing).length, 0); + + second.dispose(); + assert.strictEqual(second.isLoaded, false); + assert.strictEqual(second.isUpdating, false); + assert.strictEqual(Object.keys(cache.disposing).length, 1); + assert.ok(cache.disposing[firstKey]); + }); + + test('keep using old cacheKey when loading fails', function () { + + const cache = new MockCache(); + + const first = createCacheState(cache); + const firstKey = first.cacheKey; + first.load(); + cache.loading[firstKey].complete(null); + + const second = createCacheState(cache, first); + second.load(); + const secondKey = cache.cacheKeys[1]; + var origErrorHandler = errors.errorHandler.getUnexpectedErrorHandler(); + try { + errors.setUnexpectedErrorHandler(() => null); + cache.loading[secondKey].error('loading failed'); + } finally { + errors.setUnexpectedErrorHandler(origErrorHandler); + } + assert.strictEqual(second.isLoaded, true); + assert.strictEqual(second.isUpdating, false); + assert.strictEqual(Object.keys(cache.loading).length, 2); + assert.strictEqual(Object.keys(cache.disposing).length, 0); + assert.strictEqual(second.cacheKey, firstKey); // keep using old cacheKey + + const third = createCacheState(cache, second); + third.load(); + assert.strictEqual(third.isLoaded, true); + assert.strictEqual(third.isUpdating, true); + assert.strictEqual(Object.keys(cache.loading).length, 3); + assert.strictEqual(Object.keys(cache.disposing).length, 0); + assert.strictEqual(third.cacheKey, firstKey); + + const thirdKey = cache.cacheKeys[2]; + cache.loading[thirdKey].complete(null); + assert.strictEqual(third.isLoaded, true); + assert.strictEqual(third.isUpdating, false); + assert.strictEqual(Object.keys(cache.loading).length, 3); + assert.strictEqual(Object.keys(cache.disposing).length, 2); + assert.strictEqual(third.cacheKey, thirdKey); // recover with next successful load + }); + + function createCacheState(cache: MockCache, previous?: CacheState): CacheState { + return new CacheState( + cacheKey => cache.query(cacheKey), + query => cache.load(query), + cacheKey => cache.dispose(cacheKey), + previous + ); + } + + class MockCache { + + public cacheKeys: string[] = []; + public loading: {[cacheKey: string]: DeferredTPromise} = {}; + public disposing: {[cacheKey: string]: DeferredTPromise} = {}; + + public baseQuery: ISearchQuery = { + type: QueryType.File + }; + + public query(cacheKey: string): ISearchQuery { + this.cacheKeys.push(cacheKey); + return objects.assign({ cacheKey: cacheKey }, this.baseQuery); + } + + public load(query: ISearchQuery): TPromise { + const promise = new DeferredTPromise(); + this.loading[query.cacheKey] = promise; + return promise; + } + + public dispose(cacheKey: string): TPromise { + const promise = new DeferredTPromise(); + this.disposing[cacheKey] = promise; + return promise; + } + } +});