backups - suspend/resume onWillShutdown

src/vs/workbench/services/workingCopy/common/workingCopyBackupTracker.ts

@@ -64,6 +64,8 @@ export abstract class WorkingCopyBackupTracker extends Disposable {
 		this._register(this.workingCopyEditorService.onDidRegisterHandler(handler => this.restoreBackups(handler)));
 	}
 
+	protected abstract onBeforeShutdown(reason: ShutdownReason): boolean | Promise<boolean>;
+
 	private onWillShutdown(): void {
 
 		// Here we know that we will shutdown. Any backup operation that is
@@ -80,14 +82,6 @@ export abstract class WorkingCopyBackupTracker extends Disposable {
 
 	//#region Backup Creator
 
-	// A map from working copy to a version ID we compute on each content
-	// change. This version ID allows to e.g. ask if a backup for a specific
-	// content has been made before closing.
-	private readonly mapWorkingCopyToContentVersion = new Map<IWorkingCopy, number>();
-
-	// A map of scheduled pending backup operations for working copies
-	protected readonly pendingBackupOperations = new Map<IWorkingCopy, IDisposable>();
-
 	// Delay creation of backups when content changes to avoid too much
 	// load on the backup service when the user is typing into the editor
 	// Since we always schedule a backup, even when auto save is on, we
@@ -102,7 +96,22 @@ export abstract class WorkingCopyBackupTracker extends Disposable {
 		[AutoSaveMode.AFTER_LONG_DELAY]: 1000
 	};
 
+	// A map from working copy to a version ID we compute on each content
+	// change. This version ID allows to e.g. ask if a backup for a specific
+	// content has been made before closing.
+	private readonly mapWorkingCopyToContentVersion = new Map<IWorkingCopy, number>();
+
+	// A map of scheduled pending backup operations for working copies
+	protected readonly pendingBackupOperations = new Map<IWorkingCopy, IDisposable>();
+
+	private suspended = false;
+
 	private onDidRegister(workingCopy: IWorkingCopy): void {
+		if (this.suspended) {
+			this.logService.warn(`[backup tracker] suspended, ignoring register event`, workingCopy.resource.toString(true), workingCopy.typeId);
+			return;
+		}
+
 		if (workingCopy.isDirty()) {
 			this.scheduleBackup(workingCopy);
 		}
@@ -113,11 +122,22 @@ export abstract class WorkingCopyBackupTracker extends Disposable {
 		// Remove from content version map
 		this.mapWorkingCopyToContentVersion.delete(workingCopy);
 
+		// Check suspended
+		if (this.suspended) {
+			this.logService.warn(`[backup tracker] suspended, ignoring unregister event`, workingCopy.resource.toString(true), workingCopy.typeId);
+			return;
+		}
+
 		// Discard backup
 		this.discardBackup(workingCopy);
 	}
 
 	private onDidChangeDirty(workingCopy: IWorkingCopy): void {
+		if (this.suspended) {
+			this.logService.warn(`[backup tracker] suspended, ignoring dirty change event`, workingCopy.resource.toString(true), workingCopy.typeId);
+			return;
+		}
+
 		if (workingCopy.isDirty()) {
 			this.scheduleBackup(workingCopy);
 		} else {
@@ -131,6 +151,12 @@ export abstract class WorkingCopyBackupTracker extends Disposable {
 		const contentVersionId = this.getContentVersion(workingCopy);
 		this.mapWorkingCopyToContentVersion.set(workingCopy, contentVersionId + 1);
 
+		// Check suspended
+		if (this.suspended) {
+			this.logService.warn(`[backup tracker] suspended, ignoring content change event`, workingCopy.resource.toString(true), workingCopy.typeId);
+			return;
+		}
+
 		// Schedule backup if dirty
 		if (workingCopy.isDirty()) {
 			// this listener will make sure that the backup is
@@ -248,7 +274,11 @@ export abstract class WorkingCopyBackupTracker extends Disposable {
 		this.pendingBackupOperations.clear();
 	}
 
-	protected abstract onBeforeShutdown(reason: ShutdownReason): boolean | Promise<boolean>;
+	protected suspendBackupOperations(): { resume: () => void } {
+		this.suspended = true;
+
+		return { resume: () => this.suspended = false };
+	}
 
 	//#endregion
 

src/vs/workbench/services/workingCopy/electron-sandbox/workingCopyBackupTracker.ts

@@ -48,7 +48,7 @@ export class NativeWorkingCopyBackupTracker extends WorkingCopyBackupTracker imp
 		super(workingCopyBackupService, workingCopyService, logService, lifecycleService, filesConfigurationService, workingCopyEditorService, editorService, editorGroupService);
 	}
 
-	protected onBeforeShutdown(reason: ShutdownReason): Promise<boolean> {
+	protected async onBeforeShutdown(reason: ShutdownReason): Promise<boolean> {
 
 		// Important: we are about to shutdown and handle dirty working copies
 		// and backups. We do not want any pending backup ops to interfer with
@@ -58,14 +58,27 @@ export class NativeWorkingCopyBackupTracker extends WorkingCopyBackupTracker imp
 		// (https://github.com/microsoft/vscode/issues/138055)
 		this.cancelBackupOperations();
 
-		// Dirty working copies need treatment on shutdown
-		const dirtyWorkingCopies = this.workingCopyService.dirtyWorkingCopies;
-		if (dirtyWorkingCopies.length) {
-			return this.onBeforeShutdownWithDirty(reason, dirtyWorkingCopies);
-		}
+		// For the duration of the shutdown handling, suspend backup operations
+		// and only resume after we have handled backups. Similar to above, we
+		// do not want to trigger backup tracking during our shutdown handling
+		// but we must resume, in case of a veto afterwards.
+		const { resume } = this.suspendBackupOperations();
+
+		try {
 
-		// No dirty working copies
-		return this.onBeforeShutdownWithoutDirty();
+			// Dirty working copies need treatment on shutdown
+			const dirtyWorkingCopies = this.workingCopyService.dirtyWorkingCopies;
+			if (dirtyWorkingCopies.length) {
+				return await this.onBeforeShutdownWithDirty(reason, dirtyWorkingCopies);
+			}
+
+			// No dirty working copies
+			else {
+				return await this.onBeforeShutdownWithoutDirty();
+			}
+		} finally {
+			resume();
+		}
 	}
 
 	protected async onBeforeShutdownWithDirty(reason: ShutdownReason, dirtyWorkingCopies: readonly IWorkingCopy[]): Promise<boolean> {

src/vs/workbench/services/workingCopy/test/electron-browser/workingCopyBackupTracker.test.ts

@@ -43,6 +43,7 @@ import { IWorkingCopyEditorService } from 'vs/workbench/services/workingCopy/com
 import { TestContextService, TestWorkingCopy } from 'vs/workbench/test/common/workbenchTestServices';
 import { CancellationToken } from 'vs/base/common/cancellation';
 import { IWorkingCopyBackup } from 'vs/workbench/services/workingCopy/common/workingCopy';
+import { timeout } from 'vs/base/common/async';
 
 flakySuite('WorkingCopyBackupTracker (native)', function () {
 
@@ -360,7 +361,7 @@ flakySuite('WorkingCopyBackupTracker (native)', function () {
 		await cleanup();
 	});
 
-	test('onWillShutdown - pending backup operations canceled', async function () {
+	test('onWillShutdown - pending backup operations canceled and new ones suspended', async function () {
 		const { accessor, tracker, cleanup } = await createTracker();
 
 		const resource = toResource.call(this, '/path/index.txt');
@@ -374,10 +375,23 @@ flakySuite('WorkingCopyBackupTracker (native)', function () {
 		assert.strictEqual(tracker.pendingBackupOperationCount, 1);
 
 		const event = new TestBeforeShutdownEvent();
+		const finalVeto = timeout(1).then(() => false);
+		event.finalVeto(() => finalVeto);
 		accessor.lifecycleService.fireBeforeShutdown(event);
 
 		assert.strictEqual(tracker.pendingBackupOperationCount, 0);
 
+		model?.textEditorModel?.setValue('bar');
+		assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
+		assert.strictEqual(tracker.pendingBackupOperationCount, 0);
+
+		await finalVeto;
+
+		// Ops are resumed after handling!
+		model?.textEditorModel?.setValue('foo');
+		assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
+		assert.strictEqual(tracker.pendingBackupOperationCount, 1);
+
 		await cleanup();
 	});