From 5ec65bdc87fea3b2a71fa823144dc5df3aaa7c8a Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Thu, 23 Jan 2020 12:14:27 +0100 Subject: [PATCH] backup - remove discardAllBackups() method This method is actually deleting the entire backup home folder for the workspace and it is possible that this could lead to race conditions when a backup is running at the same time. --- .../backup/electron-browser/backupTracker.ts | 19 +++-------- .../electron-browser/backupTracker.test.ts | 6 ++-- .../services/backup/common/backup.ts | 5 --- .../backup/common/backupFileService.ts | 23 ------------- .../backupFileService.test.ts | 34 ++----------------- .../workbench/test/workbenchTestServices.ts | 4 --- 6 files changed, 10 insertions(+), 81 deletions(-) diff --git a/src/vs/workbench/contrib/backup/electron-browser/backupTracker.ts b/src/vs/workbench/contrib/backup/electron-browser/backupTracker.ts index 087cae06b13..be8f29a2f77 100644 --- a/src/vs/workbench/contrib/backup/electron-browser/backupTracker.ts +++ b/src/vs/workbench/contrib/backup/electron-browser/backupTracker.ts @@ -182,15 +182,14 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont return true; // veto (save failed or was canceled) } - return this.noVeto(dirtyCount === workingCopies.length ? true /* all */ : workingCopies); // no veto (dirty saved) + return this.noVeto(workingCopies); // no veto (dirty saved) } // Don't Save else if (confirm === ConfirmResult.DONT_SAVE) { - const dirtyCount = this.workingCopyService.dirtyCount; await this.doRevertAllBeforeShutdown(workingCopies); - return this.noVeto(dirtyCount === workingCopies.length ? true /* all */ : workingCopies); // no veto (dirty reverted) + return this.noVeto(workingCopies); // no veto (dirty reverted) } // Cancel @@ -243,7 +242,7 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont } } - private noVeto(backupsToDiscardOrAll: IWorkingCopy[] | boolean): boolean | Promise { + private noVeto(backupsToDiscard: IWorkingCopy[]): boolean | Promise { if (this.lifecycleService.phase < LifecyclePhase.Restored) { return false; // if editors have not restored, we are not up to speed with backups and thus should not discard them } @@ -252,16 +251,6 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont return false; // extension development does not track any backups } - if (backupsToDiscardOrAll === true) { - // discard all backups - return this.backupFileService.discardAllBackups().then(() => false, () => false); - } - - if (Array.isArray(backupsToDiscardOrAll)) { - // otherwise, discard individually - return Promise.all(backupsToDiscardOrAll.map(workingCopy => this.backupFileService.discardBackup(workingCopy.resource))).then(() => false, () => false); - } - - return false; + return Promise.all(backupsToDiscard.map(workingCopy => this.backupFileService.discardBackup(workingCopy.resource))).then(() => false, () => false); } } diff --git a/src/vs/workbench/contrib/backup/test/electron-browser/backupTracker.test.ts b/src/vs/workbench/contrib/backup/test/electron-browser/backupTracker.test.ts index d0d0b3b89f0..4ae7f1e7421 100644 --- a/src/vs/workbench/contrib/backup/test/electron-browser/backupTracker.test.ts +++ b/src/vs/workbench/contrib/backup/test/electron-browser/backupTracker.test.ts @@ -279,11 +279,11 @@ suite('BackupTracker', () => { let veto = event.value; if (typeof veto === 'boolean') { - assert.ok(accessor.backupFileService.didDiscardAllBackups); + assert.ok(accessor.backupFileService.discardedBackups.length > 0); assert.ok(!veto); } else { veto = await veto; - assert.ok(accessor.backupFileService.didDiscardAllBackups); + assert.ok(accessor.backupFileService.discardedBackups.length > 0); assert.ok(!veto); } @@ -452,7 +452,7 @@ suite('BackupTracker', () => { accessor.lifecycleService.fireWillShutdown(event); const veto = await (>event.value); - assert.ok(!accessor.backupFileService.didDiscardAllBackups); // When hot exit is set, backups should never be cleaned since the confirm result is cancel + assert.equal(accessor.backupFileService.discardedBackups.length, 0); // When hot exit is set, backups should never be cleaned since the confirm result is cancel assert.equal(veto, shouldVeto); part.dispose(); diff --git a/src/vs/workbench/services/backup/common/backup.ts b/src/vs/workbench/services/backup/common/backup.ts index 25fae6a15ca..66750a668b8 100644 --- a/src/vs/workbench/services/backup/common/backup.ts +++ b/src/vs/workbench/services/backup/common/backup.ts @@ -68,9 +68,4 @@ export interface IBackupFileService { * @param resource The resource whose backup is being discarded discard to back up. */ discardBackup(resource: URI): Promise; - - /** - * Discards all backups that exist. - */ - discardAllBackups(): Promise; } diff --git a/src/vs/workbench/services/backup/common/backupFileService.ts b/src/vs/workbench/services/backup/common/backupFileService.ts index d4c3416bfc8..0b5cf5a1d8a 100644 --- a/src/vs/workbench/services/backup/common/backupFileService.ts +++ b/src/vs/workbench/services/backup/common/backupFileService.ts @@ -163,10 +163,6 @@ export class BackupFileService implements IBackupFileService { return this.impl.discardBackup(resource); } - discardAllBackups(): Promise { - return this.impl.discardAllBackups(); - } - getBackups(): Promise { return this.impl.getBackups(); } @@ -277,21 +273,6 @@ class BackupFileServiceImpl extends Disposable implements IBackupFileService { }); } - async discardAllBackups(): Promise { - - // Discard each backup and clear model - // We go through the doDiscardBackup() - // method to benefit from the IO queue - const model = await this.ready; - await Promise.all(model.get().map(backupResource => this.doDiscardBackup(backupResource))); - model.clear(); - - // Delete the backup home for this workspace - // It will automatically be populated again - // once another backup is made - await this.deleteIgnoreFileNotFound(this.backupWorkspacePath); - } - private async deleteIgnoreFileNotFound(resource: URI): Promise { try { await this.fileService.del(resource, { recursive: true }); @@ -443,10 +424,6 @@ export class InMemoryBackupFileService implements IBackupFileService { this.backups.delete(this.toBackupResource(resource).toString()); } - async discardAllBackups(): Promise { - this.backups.clear(); - } - toBackupResource(resource: URI): URI { return URI.file(join(resource.scheme, this.hashPath(resource))); } diff --git a/src/vs/workbench/services/backup/test/electron-browser/backupFileService.test.ts b/src/vs/workbench/services/backup/test/electron-browser/backupFileService.test.ts index dadf089a070..ac020313462 100644 --- a/src/vs/workbench/services/backup/test/electron-browser/backupFileService.test.ts +++ b/src/vs/workbench/services/backup/test/electron-browser/backupFileService.test.ts @@ -42,7 +42,6 @@ const barFile = URI.file(platform.isWindows ? 'c:\\Bar' : '/Bar'); const fooBarFile = URI.file(platform.isWindows ? 'c:\\Foo Bar' : '/Foo Bar'); const untitledFile = URI.from({ scheme: Schemas.untitled, path: 'Untitled-1' }); const fooBackupPath = path.join(workspaceBackupPath, 'file', hashPath(fooFile)); -const barBackupPath = path.join(workspaceBackupPath, 'file', hashPath(barFile)); const untitledBackupPath = path.join(workspaceBackupPath, 'untitled', hashPath(untitledFile)); class TestBackupEnvironmentService extends NativeWorkbenchEnvironmentService { @@ -58,6 +57,7 @@ export class NodeTestBackupFileService extends BackupFileService { private backupResourceJoiners: Function[]; private discardBackupJoiners: Function[]; + discardedBackups: URI[]; constructor(workspaceBackupPath: string) { const environmentService = new TestBackupEnvironmentService(workspaceBackupPath); @@ -71,7 +71,7 @@ export class NodeTestBackupFileService extends BackupFileService { this.fileService = fileService; this.backupResourceJoiners = []; this.discardBackupJoiners = []; - this.didDiscardAllBackups = false; + this.discardedBackups = []; } joinBackupResource(): Promise { @@ -92,19 +92,12 @@ export class NodeTestBackupFileService extends BackupFileService { async discardBackup(resource: URI): Promise { await super.discardBackup(resource); + this.discardedBackups.push(resource); while (this.discardBackupJoiners.length) { this.discardBackupJoiners.pop()!(); } } - - didDiscardAllBackups: boolean; - - discardAllBackups(): Promise { - this.didDiscardAllBackups = true; - - return super.discardAllBackups(); - } } suite('BackupFileService', () => { @@ -282,27 +275,6 @@ suite('BackupFileService', () => { }); }); - suite('discardAllBackups', () => { - test('text file', async () => { - await service.backup(fooFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false)); - assert.equal(fs.readdirSync(path.join(workspaceBackupPath, 'file')).length, 1); - await service.backup(barFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false)); - assert.equal(fs.readdirSync(path.join(workspaceBackupPath, 'file')).length, 2); - await service.discardAllBackups(); - assert.equal(fs.existsSync(fooBackupPath), false); - assert.equal(fs.existsSync(barBackupPath), false); - assert.equal(fs.existsSync(path.join(workspaceBackupPath, 'file')), false); - }); - - test('untitled file', async () => { - await service.backup(untitledFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false)); - assert.equal(fs.readdirSync(path.join(workspaceBackupPath, 'untitled')).length, 1); - await service.discardAllBackups(); - assert.equal(fs.existsSync(untitledBackupPath), false); - assert.equal(fs.existsSync(path.join(workspaceBackupPath, 'untitled')), false); - }); - }); - suite('getBackups', () => { test('("file") - text file', async () => { await service.backup(fooFile, createTextBufferFactory('test').create(DefaultEndOfLine.LF).createSnapshot(false)); diff --git a/src/vs/workbench/test/workbenchTestServices.ts b/src/vs/workbench/test/workbenchTestServices.ts index cabc8dfa39e..bab9fb6d2c9 100644 --- a/src/vs/workbench/test/workbenchTestServices.ts +++ b/src/vs/workbench/test/workbenchTestServices.ts @@ -1209,10 +1209,6 @@ export class TestBackupFileService implements IBackupFileService { discardBackup(_resource: URI): Promise { return Promise.resolve(); } - - discardAllBackups(): Promise { - return Promise.resolve(); - } } export class TestCodeEditorService implements ICodeEditorService { -- GitLab