From 05e242f927ec512dfd5a4e5e628f64b8541f3c1a Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 1 Nov 2016 19:03:25 -0700 Subject: [PATCH] Write to workspaces.json exclusively on main process Fixes #14056 --- src/vs/code/electron-main/backup.ts | 23 ++++++++-- src/vs/code/electron-main/lifecycle.ts | 12 +++++- src/vs/code/test/electron-main/backup.test.ts | 16 ++++--- src/vs/test/utils/servicesTestUtils.ts | 4 -- .../services/backup/common/backup.ts | 8 ---- .../services/backup/node/backupFileService.ts | 42 ++++--------------- .../services/backup/node/backupService.ts | 6 +-- .../backup/test/backupFileService.test.ts | 32 -------------- 8 files changed, 52 insertions(+), 91 deletions(-) diff --git a/src/vs/code/electron-main/backup.ts b/src/vs/code/electron-main/backup.ts index 297b6a6a422..f630cd919dc 100644 --- a/src/vs/code/electron-main/backup.ts +++ b/src/vs/code/electron-main/backup.ts @@ -43,10 +43,19 @@ export interface IBackupService { /** * Gets the set of untitled file backups for a particular workspace. * - * @param workspace The workspace to get the backups for for. + * @param workspace The workspace to get the backups for. * @return The absolute paths for all the untitled file _backups_. */ getWorkspaceUntitledFileBackupsSync(workspace: Uri): string[]; + + /** + * Gets whether the workspace has backups associated with it (ie. if the workspace backup + * directory exists). + * + * @param workspace The workspace to evaluate. + * @return Whether the workspace has backups. + */ + doesWorkspaceHaveBackups(workspace: Uri): boolean; } export class BackupService implements IBackupService { @@ -99,8 +108,7 @@ export class BackupService implements IBackupService { } public getWorkspaceUntitledFileBackupsSync(workspace: Uri): string[] { - const workspaceHash = crypto.createHash('md5').update(workspace.fsPath).digest('hex'); - const untitledDir = path.join(this.backupHome, workspaceHash, 'untitled'); + const untitledDir = path.join(this.getWorkspaceBackupDirectory(workspace), 'untitled'); // Allow sync here as it's only used in workbench initialization's critical path try { @@ -110,6 +118,15 @@ export class BackupService implements IBackupService { } } + public doesWorkspaceHaveBackups(workspace: Uri): boolean { + return fs.existsSync(this.getWorkspaceBackupDirectory(workspace)); + } + + private getWorkspaceBackupDirectory(workspace: Uri): string { + const workspaceHash = crypto.createHash('md5').update(workspace.fsPath).digest('hex'); + return path.join(this.backupHome, workspaceHash); + } + private loadSync(): void { try { this.workspacesJsonContent = JSON.parse(fs.readFileSync(this.workspacesJsonPath, 'utf8').toString()); // invalid JSON or permission issue can happen here diff --git a/src/vs/code/electron-main/lifecycle.ts b/src/vs/code/electron-main/lifecycle.ts index cc2b48a12ff..60b0170039c 100644 --- a/src/vs/code/electron-main/lifecycle.ts +++ b/src/vs/code/electron-main/lifecycle.ts @@ -5,12 +5,14 @@ 'use strict'; +import Uri from 'vs/base/common/uri'; import { EventEmitter } from 'events'; import { ipcMain as ipc, app } from 'electron'; import { TPromise, TValueCallback } from 'vs/base/common/winjs.base'; import { ReadyState, VSCodeWindow } from 'vs/code/electron-main/window'; import { IEnvironmentService } from 'vs/platform/environment/common/environment'; import { createDecorator } from 'vs/platform/instantiation/common/instantiation'; +import { IBackupService } from 'vs/code/electron-main/backup'; import { ILogService } from 'vs/code/electron-main/log'; import { IStorageService } from 'vs/code/electron-main/storage'; @@ -52,7 +54,8 @@ export class LifecycleService implements ILifecycleService { constructor( @IEnvironmentService private environmentService: IEnvironmentService, @ILogService private logService: ILogService, - @IStorageService private storageService: IStorageService + @IStorageService private storageService: IStorageService, + @IBackupService private backupService: IBackupService ) { this.windowToCloseRequest = Object.create(null); this.quitRequested = false; @@ -121,6 +124,7 @@ export class LifecycleService implements ILifecycleService { vscodeWindow.win.on('close', (e) => { const windowId = vscodeWindow.id; this.logService.log('Lifecycle#window-before-close', windowId); + this.logService.log('this.quitRequested: ' + this.quitRequested); // The window already acknowledged to be closed if (this.windowToCloseRequest[windowId]) { @@ -135,6 +139,12 @@ export class LifecycleService implements ILifecycleService { e.preventDefault(); this.unload(vscodeWindow).done(veto => { if (!veto) { + // Clear out any workspace backups from workspaces.json that don't have any backups + const workspaceResource = Uri.file(vscodeWindow.openedWorkspacePath); + if (!this.backupService.doesWorkspaceHaveBackups(workspaceResource)) { + this.backupService.removeWorkspaceBackupPathSync(workspaceResource); + } + this.windowToCloseRequest[windowId] = true; vscodeWindow.win.close(); } else { diff --git a/src/vs/code/test/electron-main/backup.test.ts b/src/vs/code/test/electron-main/backup.test.ts index 52e0f3efc9e..51d09302e80 100644 --- a/src/vs/code/test/electron-main/backup.test.ts +++ b/src/vs/code/test/electron-main/backup.test.ts @@ -38,6 +38,8 @@ suite('BackupService', () => { const barFile = Uri.file(platform.isWindows ? 'C:\\bar' : '/bar'); const bazFile = Uri.file(platform.isWindows ? 'C:\\baz' : '/baz'); + const fooWorkspaceBackupDir = path.join(backupHome, crypto.createHash('md5').update(fooFile.fsPath).digest('hex')); + let backupService: BackupService; setup(done => { @@ -94,16 +96,14 @@ suite('BackupService', () => { }); test('getWorkspaceUntitledFileBackupsSync should return untitled file backup resources', done => { - const workspaceResource = fooFile; - const workspaceHash = crypto.createHash('md5').update(workspaceResource.fsPath).digest('hex'); - const untitledBackupDir = path.join(backupHome, workspaceHash, 'untitled'); + const untitledBackupDir = path.join(fooWorkspaceBackupDir, 'untitled'); const untitledBackup1 = path.join(untitledBackupDir, 'bar'); const untitledBackup2 = path.join(untitledBackupDir, 'foo'); pfs.mkdirp(untitledBackupDir).then(() => { pfs.writeFile(untitledBackup1, 'test').then(() => { - assert.deepEqual(backupService.getWorkspaceUntitledFileBackupsSync(workspaceResource), [untitledBackup1]); + assert.deepEqual(backupService.getWorkspaceUntitledFileBackupsSync(fooFile), [untitledBackup1]); pfs.writeFile(untitledBackup2, 'test').then(() => { - assert.deepEqual(backupService.getWorkspaceUntitledFileBackupsSync(workspaceResource), [untitledBackup1, untitledBackup2]); + assert.deepEqual(backupService.getWorkspaceUntitledFileBackupsSync(fooFile), [untitledBackup1, untitledBackup2]); done(); }); }); @@ -138,4 +138,10 @@ suite('BackupService', () => { }); }); }); + + test('doesWorkspaceHaveBackups should return whether the workspace\'s backup exists', () => { + assert.equal(backupService.doesWorkspaceHaveBackups(fooFile), false); + fs.mkdirSync(fooWorkspaceBackupDir); + assert.equal(backupService.doesWorkspaceHaveBackups(fooFile), true); + }); }); \ No newline at end of file diff --git a/src/vs/test/utils/servicesTestUtils.ts b/src/vs/test/utils/servicesTestUtils.ts index 17c48c30993..9336da01dfd 100644 --- a/src/vs/test/utils/servicesTestUtils.ts +++ b/src/vs/test/utils/servicesTestUtils.ts @@ -641,10 +641,6 @@ export class TestBackupFileService implements IBackupFileService { return null; } - public removeWorkspaceBackupPath(workspace: URI): TPromise { - return TPromise.as(void 0); - } - public getWorkspaceTextFilesWithBackupsSync(workspace: URI): string[] { return []; } diff --git a/src/vs/workbench/services/backup/common/backup.ts b/src/vs/workbench/services/backup/common/backup.ts index d16df4d4b99..2cfde98db27 100644 --- a/src/vs/workbench/services/backup/common/backup.ts +++ b/src/vs/workbench/services/backup/common/backup.ts @@ -41,14 +41,6 @@ export interface IBackupFileService { */ getWorkspaceBackupPaths(): TPromise; - /** - * Removes a workspace backup path being tracked for restoration, deregistering all associated - * resources for backup. - * - * @param workspace The absolute workspace path being removed. - */ - removeWorkspaceBackupPath(workspace: Uri): TPromise; - /** * Gets whether a text file has a backup to restore. * diff --git a/src/vs/workbench/services/backup/node/backupFileService.ts b/src/vs/workbench/services/backup/node/backupFileService.ts index 3bd89bbb0dc..fc7200f6189 100644 --- a/src/vs/workbench/services/backup/node/backupFileService.ts +++ b/src/vs/workbench/services/backup/node/backupFileService.ts @@ -22,8 +22,6 @@ export class BackupFileService implements IBackupFileService { protected backupHome: string; protected workspacesJsonPath: string; - private workspacesJsonContent: IBackupWorkspacesFormat; - constructor( private currentWorkspace: Uri, @IEnvironmentService environmentService: IEnvironmentService, @@ -34,22 +32,8 @@ export class BackupFileService implements IBackupFileService { } public getWorkspaceBackupPaths(): TPromise { - return this.loadWorkspaces().then(() => { - return this.workspacesJsonContent.folderWorkspaces; - }); - } - - public removeWorkspaceBackupPath(workspace: Uri): TPromise { - return this.loadWorkspaces().then(() => { - if (!this.workspacesJsonContent.folderWorkspaces) { - return TPromise.as(void 0); - } - const index = this.workspacesJsonContent.folderWorkspaces.indexOf(workspace.fsPath); - if (index === -1) { - return TPromise.as(void 0); - } - this.workspacesJsonContent.folderWorkspaces.splice(index, 1); - return this.saveWorkspaces(); + return this.loadWorkspaces().then(workspacesJsonContent => { + return workspacesJsonContent.folderWorkspaces; }); } @@ -104,13 +88,10 @@ export class BackupFileService implements IBackupFileService { return this.fileService.del(Uri.file(this.getWorkspaceBackupDirectory())); } - private loadWorkspaces(): TPromise { + private loadWorkspaces(): TPromise { return pfs.fileExists(this.workspacesJsonPath).then(exists => { if (!exists) { - this.workspacesJsonContent = { - folderWorkspaces: [] - }; - return TPromise.as(void 0); + return { folderWorkspaces: [] }; } return pfs.readFile(this.workspacesJsonPath, 'utf8').then(content => { @@ -120,19 +101,12 @@ export class BackupFileService implements IBackupFileService { return []; } }).then(content => { - this.workspacesJsonContent = content; - if (!this.workspacesJsonContent.folderWorkspaces) { - this.workspacesJsonContent.folderWorkspaces = []; + let result = content; + if (!result.folderWorkspaces) { + result.folderWorkspaces = []; } - return TPromise.as(void 0); + return result; }); }); } - - private saveWorkspaces(): TPromise { - const data = JSON.stringify(this.workspacesJsonContent); - return pfs.mkdirp(this.backupHome).then(() => { - return pfs.writeFile(this.workspacesJsonPath, data); - }); - } } \ No newline at end of file diff --git a/src/vs/workbench/services/backup/node/backupService.ts b/src/vs/workbench/services/backup/node/backupService.ts index 361370f1bad..73922b82052 100644 --- a/src/vs/workbench/services/backup/node/backupService.ts +++ b/src/vs/workbench/services/backup/node/backupService.ts @@ -158,10 +158,8 @@ export class BackupService implements IBackupService { if (!workspace) { return false; // no backups to cleanup, no veto } - return this.backupFileService.removeWorkspaceBackupPath(workspace.resource).then(() => { - return this.backupFileService.discardAllWorkspaceBackups().then(() => { - return false; // no veto - }); + return this.backupFileService.discardAllWorkspaceBackups().then(() => { + return false; // no veto }); } diff --git a/src/vs/workbench/services/backup/test/backupFileService.test.ts b/src/vs/workbench/services/backup/test/backupFileService.test.ts index 7e1bd45bbeb..5ac2c9157e9 100644 --- a/src/vs/workbench/services/backup/test/backupFileService.test.ts +++ b/src/vs/workbench/services/backup/test/backupFileService.test.ts @@ -66,38 +66,6 @@ suite('BackupFileService', () => { extfs.del(backupHome, os.tmpdir(), done); }); - test('removeWorkspaceBackupPath should remove workspaces from workspaces.json', done => { - const workspacesJson: IBackupWorkspacesFormat = { folderWorkspaces: [fooFile.fsPath, barFile.fsPath] }; - pfs.writeFileAndFlush(workspacesJsonPath, JSON.stringify(workspacesJson)).then(() => { - service.removeWorkspaceBackupPath(fooFile).then(() => { - pfs.readFile(workspacesJsonPath, 'utf-8').then(buffer => { - const json = JSON.parse(buffer); - assert.deepEqual(json.folderWorkspaces, [barFile.fsPath]); - service.removeWorkspaceBackupPath(barFile).then(() => { - pfs.readFile(workspacesJsonPath, 'utf-8').then(content => { - const json2 = JSON.parse(content); - assert.deepEqual(json2.folderWorkspaces, []); - done(); - }); - }); - }); - }); - }); - }); - - test('removeWorkspaceBackupPath should fail gracefully when removing a path that doesn\'t exist', done => { - const workspacesJson: IBackupWorkspacesFormat = { folderWorkspaces: [fooFile.fsPath] }; - pfs.writeFileAndFlush(workspacesJsonPath, JSON.stringify(workspacesJson)).then(() => { - service.removeWorkspaceBackupPath(barFile).then(() => { - pfs.readFile(workspacesJsonPath, 'utf-8').then(content => { - const json = JSON.parse(content); - assert.deepEqual(json.folderWorkspaces, [fooFile.fsPath]); - done(); - }); - }); - }); - }); - test('getBackupResource should get the correct backup path for text files', () => { // Format should be: /// const backupResource = fooFile;