From 1ed73408cd2a9229dfd67de33ccd8a3a801bdcd5 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Fri, 9 Apr 2021 16:02:46 +0200 Subject: [PATCH] file working copy - prolong shutdown for pending saves --- .../services/backup/common/backupRestorer.ts | 2 +- .../electron-browser/backupTracker.test.ts | 38 +++++++------------ .../common/fileWorkingCopyManager.ts | 14 ++++++- .../browser/fileWorkingCopyManager.test.ts | 32 +++++++++++++++- .../test/browser/workbenchTestServices.ts | 24 +++++++++++- 5 files changed, 82 insertions(+), 28 deletions(-) diff --git a/src/vs/workbench/services/backup/common/backupRestorer.ts b/src/vs/workbench/services/backup/common/backupRestorer.ts index 417516ad054..6098e8a45d4 100644 --- a/src/vs/workbench/services/backup/common/backupRestorer.ts +++ b/src/vs/workbench/services/backup/common/backupRestorer.ts @@ -103,7 +103,7 @@ export class BackupRestorer implements IWorkbenchContribution { private async resolveEditor(resource: URI, index: number, hasOpenedEditors: boolean): Promise { - // Set editor as `inatice` if we have other editors + // Set editor as `inactive` if we have other editors const options = { pinned: true, preserveFocus: true, inactive: index > 0 || hasOpenedEditors }; // This is a (weak) strategy to find out if the untitled input had diff --git a/src/vs/workbench/services/backup/test/electron-browser/backupTracker.test.ts b/src/vs/workbench/services/backup/test/electron-browser/backupTracker.test.ts index 05ba21a1ea1..f4dd08b90ad 100644 --- a/src/vs/workbench/services/backup/test/electron-browser/backupTracker.test.ts +++ b/src/vs/workbench/services/backup/test/electron-browser/backupTracker.test.ts @@ -26,7 +26,7 @@ import { IFilesConfigurationService } from 'vs/workbench/services/filesConfigura import { IWorkingCopyService } from 'vs/workbench/services/workingCopy/common/workingCopyService'; import { ILogService } from 'vs/platform/log/common/log'; import { HotExitConfiguration } from 'vs/platform/files/common/files'; -import { ShutdownReason, ILifecycleService, BeforeShutdownEvent } from 'vs/workbench/services/lifecycle/common/lifecycle'; +import { ShutdownReason, ILifecycleService } from 'vs/workbench/services/lifecycle/common/lifecycle'; import { IFileDialogService, ConfirmResult, IDialogService } from 'vs/platform/dialogs/common/dialogs'; import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace'; import { INativeHostService } from 'vs/platform/native/electron-sandbox/native'; @@ -35,7 +35,7 @@ import { workbenchInstantiationService, TestServiceAccessor } from 'vs/workbench import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; -import { createEditorPart, registerTestFileEditor, TestFilesConfigurationService } from 'vs/workbench/test/browser/workbenchTestServices'; +import { createEditorPart, registerTestFileEditor, TestBeforeShutdownEvent, TestFilesConfigurationService } from 'vs/workbench/test/browser/workbenchTestServices'; import { MockContextKeyService } from 'vs/platform/keybinding/test/common/mockKeybindingService'; import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey'; import { IEnvironmentService } from 'vs/platform/environment/common/environment'; @@ -76,16 +76,6 @@ flakySuite('BackupTracker (native)', function () { } } - class BeforeShutdownEventImpl implements BeforeShutdownEvent { - - value: boolean | Promise | undefined; - reason = ShutdownReason.CLOSE; - - veto(value: boolean | Promise): void { - this.value = value; - } - } - let testDir: string; let backupHome: string; let workspaceBackupPath: string; @@ -192,8 +182,8 @@ flakySuite('BackupTracker (native)', function () { const resource = toResource.call(this, '/path/index.txt'); await accessor.editorService.openEditor({ resource, options: { pinned: true } }); - const event = new BeforeShutdownEventImpl(); - accessor.lifecycleService.fireWillShutdown(event); + const event = new TestBeforeShutdownEvent(); + accessor.lifecycleService.fireBeforeShutdown(event); const veto = await event.value; assert.ok(!veto); @@ -216,8 +206,8 @@ flakySuite('BackupTracker (native)', function () { model?.textEditorModel?.setValue('foo'); assert.strictEqual(accessor.workingCopyService.dirtyCount, 1); - const event = new BeforeShutdownEventImpl(); - accessor.lifecycleService.fireWillShutdown(event); + const event = new TestBeforeShutdownEvent(); + accessor.lifecycleService.fireBeforeShutdown(event); const veto = await event.value; assert.ok(veto); @@ -237,8 +227,8 @@ flakySuite('BackupTracker (native)', function () { model?.textEditorModel?.setValue('foo'); assert.strictEqual(accessor.workingCopyService.dirtyCount, 1); - const event = new BeforeShutdownEventImpl(); - accessor.lifecycleService.fireWillShutdown(event); + const event = new TestBeforeShutdownEvent(); + accessor.lifecycleService.fireBeforeShutdown(event); const veto = await event.value; assert.ok(!veto); @@ -262,8 +252,8 @@ flakySuite('BackupTracker (native)', function () { await model?.resolve(); model?.textEditorModel?.setValue('foo'); assert.strictEqual(accessor.workingCopyService.dirtyCount, 1); - const event = new BeforeShutdownEventImpl(); - accessor.lifecycleService.fireWillShutdown(event); + const event = new TestBeforeShutdownEvent(); + accessor.lifecycleService.fireBeforeShutdown(event); const veto = await event.value; assert.ok(!veto); @@ -286,8 +276,8 @@ flakySuite('BackupTracker (native)', function () { await model?.resolve(); model?.textEditorModel?.setValue('foo'); assert.strictEqual(accessor.workingCopyService.dirtyCount, 1); - const event = new BeforeShutdownEventImpl(); - accessor.lifecycleService.fireWillShutdown(event); + const event = new TestBeforeShutdownEvent(); + accessor.lifecycleService.fireBeforeShutdown(event); const veto = await event.value; assert.ok(!veto); @@ -427,9 +417,9 @@ flakySuite('BackupTracker (native)', function () { model?.textEditorModel?.setValue('foo'); assert.strictEqual(accessor.workingCopyService.dirtyCount, 1); - const event = new BeforeShutdownEventImpl(); + const event = new TestBeforeShutdownEvent(); event.reason = shutdownReason; - accessor.lifecycleService.fireWillShutdown(event); + accessor.lifecycleService.fireBeforeShutdown(event); const veto = await event.value; assert.strictEqual(accessor.backupFileService.discardedBackups.length, 0); // When hot exit is set, backups should never be cleaned since the confirm result is cancel diff --git a/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts b/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts index 57a7c5e8dec..210bbf86f7c 100644 --- a/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts +++ b/src/vs/workbench/services/workingCopy/common/fileWorkingCopyManager.ts @@ -5,7 +5,7 @@ import { Disposable, DisposableStore, dispose, IDisposable } from 'vs/base/common/lifecycle'; import { Event, Emitter } from 'vs/base/common/event'; -import { FileWorkingCopy, IFileWorkingCopy, IFileWorkingCopyModel, IFileWorkingCopyModelFactory, IFileWorkingCopySaveOptions } from 'vs/workbench/services/workingCopy/common/fileWorkingCopy'; +import { FileWorkingCopy, FileWorkingCopyState, IFileWorkingCopy, IFileWorkingCopyModel, IFileWorkingCopyModelFactory, IFileWorkingCopySaveOptions } from 'vs/workbench/services/workingCopy/common/fileWorkingCopy'; import { SaveReason } from 'vs/workbench/common/editor'; import { ResourceMap } from 'vs/base/common/map'; import { Promises, ResourceQueue } from 'vs/base/common/async'; @@ -226,9 +226,21 @@ export class FileWorkingCopyManager extends Dis this._register(this.workingCopyFileService.onDidRunWorkingCopyFileOperation(e => this.onDidRunWorkingCopyFileOperation(e))); // Lifecycle + this.lifecycleService.onWillShutdown(event => event.join(this.onWillShutdown(), 'join.fileWorkingCopyManager')); this.lifecycleService.onDidShutdown(() => this.dispose()); } + private async onWillShutdown(): Promise { + let fileWorkingCopies: IFileWorkingCopy[]; + + // As long as file working copies are pending to be saved, we prolong the shutdown + // until that has happened to ensure we are not shutting down in the middle of + // writing to the working copy (https://github.com/microsoft/vscode/issues/116600). + while ((fileWorkingCopies = this.workingCopies.filter(workingCopy => workingCopy.hasState(FileWorkingCopyState.PENDING_SAVE))).length > 0) { + await Promises.settled(fileWorkingCopies.map(workingCopy => workingCopy.joinState(FileWorkingCopyState.PENDING_SAVE))); + } + } + //#region Resolve from file changes private onDidFilesChange(e: FileChangesEvent): void { diff --git a/src/vs/workbench/services/workingCopy/test/browser/fileWorkingCopyManager.test.ts b/src/vs/workbench/services/workingCopy/test/browser/fileWorkingCopyManager.test.ts index b15b4bb83b0..53a46392ce2 100644 --- a/src/vs/workbench/services/workingCopy/test/browser/fileWorkingCopyManager.test.ts +++ b/src/vs/workbench/services/workingCopy/test/browser/fileWorkingCopyManager.test.ts @@ -6,7 +6,7 @@ import * as assert from 'assert'; import { URI } from 'vs/base/common/uri'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; -import { workbenchInstantiationService, TestServiceAccessor } from 'vs/workbench/test/browser/workbenchTestServices'; +import { workbenchInstantiationService, TestServiceAccessor, TestWillShutdownEvent } from 'vs/workbench/test/browser/workbenchTestServices'; import { FileWorkingCopyManager, IFileWorkingCopyManager } from 'vs/workbench/services/workingCopy/common/fileWorkingCopyManager'; import { IFileWorkingCopy, IFileWorkingCopyModel } from 'vs/workbench/services/workingCopy/common/fileWorkingCopy'; import { bufferToStream, VSBuffer } from 'vs/base/common/buffer'; @@ -444,4 +444,34 @@ suite('FileWorkingCopyManager', () => { let canDispose2 = manager.canDispose(workingCopy); assert.strictEqual(canDispose2, true); }); + + test('pending saves join on shutdown', async () => { + const resource1 = URI.file('/path/index_something1.txt'); + const resource2 = URI.file('/path/index_something2.txt'); + + const workingCopy1 = await manager.resolve(resource1); + workingCopy1.model?.updateContents('make dirty'); + + const workingCopy2 = await manager.resolve(resource2); + workingCopy2.model?.updateContents('make dirty'); + + let saved1 = false; + workingCopy1.save().then(() => { + saved1 = true; + }); + + let saved2 = false; + workingCopy2.save().then(() => { + saved2 = true; + }); + + const event = new TestWillShutdownEvent(); + accessor.lifecycleService.fireWillShutdown(event); + + assert.ok(event.value.length > 0); + await Promise.all(event.value); + + assert.strictEqual(saved1, true); + assert.strictEqual(saved2, true); + }); }); diff --git a/src/vs/workbench/test/browser/workbenchTestServices.ts b/src/vs/workbench/test/browser/workbenchTestServices.ts index bb5f1470479..5ac5cee4c04 100644 --- a/src/vs/workbench/test/browser/workbenchTestServices.ts +++ b/src/vs/workbench/test/browser/workbenchTestServices.ts @@ -1057,13 +1057,35 @@ export class TestLifecycleService implements ILifecycleService { }); } - fireWillShutdown(event: BeforeShutdownEvent): void { this._onBeforeShutdown.fire(event); } + fireBeforeShutdown(event: BeforeShutdownEvent): void { this._onBeforeShutdown.fire(event); } + + fireWillShutdown(event: WillShutdownEvent): void { this._onWillShutdown.fire(event); } shutdown(): void { this.fireShutdown(); } } +export class TestBeforeShutdownEvent implements BeforeShutdownEvent { + + value: boolean | Promise | undefined; + reason = ShutdownReason.CLOSE; + + veto(value: boolean | Promise): void { + this.value = value; + } +} + +export class TestWillShutdownEvent implements WillShutdownEvent { + + value: Promise[] = []; + reason = ShutdownReason.CLOSE; + + join(promise: Promise, id: string): void { + this.value.push(promise); + } +} + export class TestTextResourceConfigurationService implements ITextResourceConfigurationService { declare readonly _serviceBrand: undefined;