ignore listeners that failed (error, timeout) more than n times, #239

src/vs/base/common/async.ts

@@ -383,7 +383,7 @@ export function sequence<T>(promiseFactory: ITask<TPromise<T>>[]): TPromise<T[]>
 	}
 
 	function thenHandler(result: any): Promise {
-		if (result) {
+		if (result !== undefined && result !== null) {
 			results.push(result);
 		}
 

src/vs/workbench/api/node/extHostDocumentSaveParticipant.ts

@@ -16,19 +16,26 @@ import {fromRange} from 'vs/workbench/api/node/extHostTypeConverters';
 import {IResourceEdit} from 'vs/editor/common/services/bulkEdit';
 import {ExtHostDocuments} from 'vs/workbench/api/node/extHostDocuments';
 
+declare class WeakMap<K, V> {
+	// delete(key: K): boolean;
+	get(key: K): V;
+	// has(key: K): boolean;
+	set(key: K, value?: V): WeakMap<K, V>;
+}
 
 export class ExtHostDocumentSaveParticipant extends ExtHostDocumentSaveParticipantShape {
 
 	private _documents: ExtHostDocuments;
 	private _workspace: MainThreadWorkspaceShape;
-	private _listenerTimeout: number;
 	private _callbacks = new CallbackList();
+	private _badListeners = new WeakMap<Function, number>();
+	private _thresholds: { timeout: number; errors: number; };
 
-	constructor(documents: ExtHostDocuments, workspace: MainThreadWorkspaceShape, listenerTimeout: number = 1000) {
+	constructor(documents: ExtHostDocuments, workspace: MainThreadWorkspaceShape, thresholds: { timeout: number; errors: number; } = { timeout: 1000, errors: 5 }) {
 		super();
 		this._documents = documents;
 		this._workspace = workspace;
-		this._listenerTimeout = listenerTimeout;
+		this._thresholds = thresholds;
 	}
 
 	dispose(): void {
@@ -38,21 +45,47 @@ export class ExtHostDocumentSaveParticipant extends ExtHostDocumentSaveParticipa
 	get onWillSaveTextDocumentEvent(): Event<vscode.TextDocumentWillSaveEvent> {
 		return (listener, thisArg, disposables) => {
 			this._callbacks.add(listener, thisArg);
-			const result = { dispose: () => this._callbacks.remove(listener, thisArg) };
+			const result = {
+				dispose: () => {
+					this._callbacks.remove(listener, thisArg);
+				}
+			};
 			if (Array.isArray(disposables)) {
 				disposables.push(result);
 			}
 			return result;
 		};
 	}
 
-	$participateInSave(resource: URI): TPromise<any[]> {
+	$participateInSave(resource: URI): TPromise<boolean[]> {
 		const entries = this._callbacks.entries();
 
 		return sequence(entries.map(([fn, thisArg]) => {
 			return () => {
+
+				const errors = this._badListeners.get(fn);
+				if (errors > this._thresholds.errors) {
+					// ignored
+					return TPromise.wrap(false);
+				}
+
 				const document = this._documents.getDocumentData(resource).document;
-				return this._deliverEventAsync(fn, thisArg, document);
+				return this._deliverEventAsync(fn, thisArg, document).then(() => {
+					// don't send result across the wire
+					return true;
+
+				}, err => {
+					if (!(err instanceof Error) || (<Error>err).message !== 'concurrent_edits') {
+						const errors = this._badListeners.get(fn);
+						this._badListeners.set(fn, !errors ? 1 : errors + 1);
+
+						// todo@joh signal to the listener?
+						// if (errors === this._thresholds.errors) {
+						// 	console.warn('BAD onWillSaveTextDocumentEvent-listener is from now on being ignored');
+						// }
+					}
+					return false;
+				});
 			};
 		}));
 	}
@@ -77,15 +110,15 @@ export class ExtHostDocumentSaveParticipant extends ExtHostDocumentSaveParticipa
 			// fire event
 			listener.apply(thisArg, [event]);
 		} catch (err) {
-			return TPromise.as(new Error(err));
+			return TPromise.wrapError(err);
 		}
 
 		// freeze promises after event call
 		Object.freeze(promises);
 
 		return new TPromise<any[]>((resolve, reject) => {
 			// join on all listener promises, reject after timeout
-			const handle = setTimeout(() => reject(new Error('timeout')), this._listenerTimeout);
+			const handle = setTimeout(() => reject(new Error('timeout')), this._thresholds.timeout);
 			return always(TPromise.join(promises), () => clearTimeout(handle)).then(resolve, reject);
 
 		}).then(values => {
@@ -114,11 +147,7 @@ export class ExtHostDocumentSaveParticipant extends ExtHostDocumentSaveParticipa
 			}
 
 			// TODO@joh bubble this to listener?
-			return new Error('ignoring change because of concurrent edits');
-
-		}, err => {
-			// soft ignore, turning into result
-			return err;
+			return TPromise.wrapError(new Error('concurrent_edits'));
 		});
 	}
 }

src/vs/workbench/test/node/api/extHostDocumentSaveParticipant.test.ts

@@ -92,8 +92,7 @@ suite('ExtHostDocumentSaveParticipant', () => {
 			sub.dispose();
 
 			const [first] = values;
-			assert.ok(first instanceof Error);
-			assert.ok((<Error>first).message);
+			assert.equal(first, false);
 		});
 	});
 
@@ -134,6 +133,27 @@ suite('ExtHostDocumentSaveParticipant', () => {
 		});
 	});
 
+	test('event delivery, ignore bad listeners', () => {
+		const participant = new ExtHostDocumentSaveParticipant(documents, workspace, { timeout: 5, errors: 1 });
+
+		let callCount = 0;
+		let sub = participant.onWillSaveTextDocumentEvent(function (event) {
+			callCount += 1;
+			throw new Error('boom');
+		});
+
+		return TPromise.join([
+			participant.$participateInSave(resource),
+			participant.$participateInSave(resource),
+			participant.$participateInSave(resource),
+			participant.$participateInSave(resource)
+
+		]).then(values => {
+			sub.dispose();
+			assert.equal(callCount, 2);
+		});
+	});
+
 	test('event delivery, waitUntil', () => {
 		const participant = new ExtHostDocumentSaveParticipant(documents, workspace);
 
@@ -174,7 +194,7 @@ suite('ExtHostDocumentSaveParticipant', () => {
 	});
 
 	test('event delivery, waitUntil will timeout', () => {
-		const participant = new ExtHostDocumentSaveParticipant(documents, workspace, 5);
+		const participant = new ExtHostDocumentSaveParticipant(documents, workspace, { timeout: 5, errors: 3 });
 
 		let sub = participant.onWillSaveTextDocumentEvent(function (event) {
 			event.waitUntil(TPromise.timeout(15));
@@ -184,8 +204,7 @@ suite('ExtHostDocumentSaveParticipant', () => {
 			sub.dispose();
 
 			const [first] = values;
-			assert.ok(first instanceof Error);
-			assert.ok((<Error>first).message);
+			assert.equal(first, false);
 		});
 	});
 
@@ -256,7 +275,7 @@ suite('ExtHostDocumentSaveParticipant', () => {
 			sub.dispose();
 
 			assert.equal(edits, undefined);
-			assert.ok((<Error>values[0]).message);
+			assert.equal(values[0], false);
 		});
 
 	});