Fixes #11181: Wait for any running file search before spawning the next

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<C> extends TPromise<C> {
+export class DeferredTPromise<T> extends TPromise<T> {
 
-	constructor(init:(complete: TValueCallback<C>, error:(err:any)=>void)=>void) {
-		super((c, e) => setTimeout(() => init(c, e)));
+	public canceled = false;
+
+	private completeCallback: TValueCallback<T>;
+	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<C,P> extends PPromise<C, P> {

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<void>;
 
 	constructor(private cacheQuery: (cacheKey: string) => ISearchQuery, private doLoad: (query: ISearchQuery) => TPromise<any>, private doDispose: (cacheKey: string) => TPromise<void>, 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;

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<any>} = {};
+		public disposing: {[cacheKey: string]: DeferredTPromise<void>} = {};
+
+		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<any> {
+			const promise = new DeferredTPromise<any>();
+			this.loading[query.cacheKey] = promise;
+			return promise;
+		}
+
+		public dispose(cacheKey: string): TPromise<void> {
+			const promise = new DeferredTPromise<void>();
+			this.disposing[cacheKey] = promise;
+			return promise;
+		}
+	}
+});