From 627fa8d42529364741784dbb71db8a07b71e5bc9 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Fri, 16 Oct 2020 13:29:14 +0200 Subject: [PATCH] Web: do not disable backups when auto save is enabled (fix #108789) --- .../contrib/backup/browser/backupTracker.ts | 39 ++++++++++++++-- .../contrib/backup/common/backupTracker.ts | 44 +++++++------------ .../backup/electron-sandbox/backupTracker.ts | 33 +++++++++++++- .../electron-browser/backupTracker.test.ts | 5 ++- 4 files changed, 84 insertions(+), 37 deletions(-) diff --git a/src/vs/workbench/contrib/backup/browser/backupTracker.ts b/src/vs/workbench/contrib/backup/browser/backupTracker.ts index 743585520e0..f4be13d34b9 100644 --- a/src/vs/workbench/contrib/backup/browser/backupTracker.ts +++ b/src/vs/workbench/contrib/backup/browser/backupTracker.ts @@ -5,22 +5,53 @@ import { IBackupFileService } from 'vs/workbench/services/backup/common/backup'; import { IWorkbenchContribution } from 'vs/workbench/common/contributions'; -import { IFilesConfigurationService } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService'; -import { IWorkingCopyService } from 'vs/workbench/services/workingCopy/common/workingCopyService'; +import { AutoSaveMode, IFilesConfigurationService } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService'; +import { IWorkingCopy, IWorkingCopyService, WorkingCopyCapabilities } from 'vs/workbench/services/workingCopy/common/workingCopyService'; import { ILifecycleService, ShutdownReason } from 'vs/platform/lifecycle/common/lifecycle'; import { ILogService } from 'vs/platform/log/common/log'; import { BackupTracker } from 'vs/workbench/contrib/backup/common/backupTracker'; export class BrowserBackupTracker extends BackupTracker implements IWorkbenchContribution { + // 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 (web + // only), we have different scheduling delays based on auto save. This + // helps to avoid a race between saving (after 1s per default) and making + // a backup of the working copy. + private static readonly BACKUP_SCHEDULE_DELAYS = { + [AutoSaveMode.OFF]: 1000, + [AutoSaveMode.ON_FOCUS_CHANGE]: 1000, + [AutoSaveMode.ON_WINDOW_CHANGE]: 1000, + [AutoSaveMode.AFTER_SHORT_DELAY]: 2000, // explicitly higher to prevent races + [AutoSaveMode.AFTER_LONG_DELAY]: 1000 + }; + constructor( @IBackupFileService backupFileService: IBackupFileService, - @IFilesConfigurationService filesConfigurationService: IFilesConfigurationService, + @IFilesConfigurationService private readonly filesConfigurationService: IFilesConfigurationService, @IWorkingCopyService workingCopyService: IWorkingCopyService, @ILifecycleService lifecycleService: ILifecycleService, @ILogService logService: ILogService ) { - super(backupFileService, filesConfigurationService, workingCopyService, logService, lifecycleService); + super(backupFileService, workingCopyService, logService, lifecycleService); + } + + protected shouldScheduleBackup(workingCopy: IWorkingCopy): boolean { + // Web: we always want to schedule a backup, even if auto save + // is enabled because in web there is no handler on shutdown + // to trigger saving so there is a higher chance of dataloss. + // See https://github.com/microsoft/vscode/issues/108789 + return true; + } + + protected getBackupScheduleDelay(workingCopy: IWorkingCopy): number { + let autoSaveMode = this.filesConfigurationService.getAutoSaveMode(); + if (workingCopy.capabilities & WorkingCopyCapabilities.Untitled) { + autoSaveMode = AutoSaveMode.OFF; // auto-save is never on for untitled working copies + } + + return BrowserBackupTracker.BACKUP_SCHEDULE_DELAYS[autoSaveMode]; } protected onBeforeShutdown(reason: ShutdownReason): boolean | Promise { diff --git a/src/vs/workbench/contrib/backup/common/backupTracker.ts b/src/vs/workbench/contrib/backup/common/backupTracker.ts index a854320c20b..9658fda1ad0 100644 --- a/src/vs/workbench/contrib/backup/common/backupTracker.ts +++ b/src/vs/workbench/contrib/backup/common/backupTracker.ts @@ -5,47 +5,28 @@ import { IBackupFileService } from 'vs/workbench/services/backup/common/backup'; import { Disposable, IDisposable, dispose, toDisposable } from 'vs/base/common/lifecycle'; -import { IFilesConfigurationService, IAutoSaveConfiguration } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService'; -import { IWorkingCopyService, IWorkingCopy, WorkingCopyCapabilities } from 'vs/workbench/services/workingCopy/common/workingCopyService'; +import { IWorkingCopyService, IWorkingCopy } from 'vs/workbench/services/workingCopy/common/workingCopyService'; import { ILogService } from 'vs/platform/log/common/log'; import { ShutdownReason, ILifecycleService } from 'vs/platform/lifecycle/common/lifecycle'; export abstract class BackupTracker extends Disposable { - // Disable backup for when a short auto-save delay is configured with - // the rationale that the auto save will trigger a save periodically - // anway and thus creating frequent backups is not useful - // - // This will only apply to working copies that are not untitled where - // auto save is actually saving. - private static DISABLE_BACKUP_AUTO_SAVE_THRESHOLD = 1500; - - // Delay creation of backups when content changes to avoid too much - // load on the backup service when the user is typing into the editor - protected static BACKUP_FROM_CONTENT_CHANGE_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(); - private backupsDisabledForAutoSaveables = false; - // A map of scheduled pending backups for working copies private readonly pendingBackups = new Map(); constructor( protected readonly backupFileService: IBackupFileService, - protected readonly filesConfigurationService: IFilesConfigurationService, protected readonly workingCopyService: IWorkingCopyService, protected readonly logService: ILogService, protected readonly lifecycleService: ILifecycleService ) { super(); - // Figure out initial auto save config - this.onAutoSaveConfigurationChange(filesConfigurationService.getAutoSaveConfiguration()); - // Fill in initial dirty working copies this.workingCopyService.dirtyWorkingCopies.forEach(workingCopy => this.onDidRegister(workingCopy)); @@ -60,9 +41,6 @@ export abstract class BackupTracker extends Disposable { this._register(this.workingCopyService.onDidChangeDirty(workingCopy => this.onDidChangeDirty(workingCopy))); this._register(this.workingCopyService.onDidChangeContent(workingCopy => this.onDidChangeContent(workingCopy))); - // Listen to auto save config changes - this._register(this.filesConfigurationService.onAutoSaveConfigurationChange(c => this.onAutoSaveConfigurationChange(c))); - // Lifecycle (handled in subclasses) this.lifecycleService.onBeforeShutdown(event => event.veto(this.onBeforeShutdown(event.reason))); } @@ -105,13 +83,21 @@ export abstract class BackupTracker extends Disposable { } } - private onAutoSaveConfigurationChange(configuration: IAutoSaveConfiguration): void { - this.backupsDisabledForAutoSaveables = typeof configuration.autoSaveDelay === 'number' && configuration.autoSaveDelay < BackupTracker.DISABLE_BACKUP_AUTO_SAVE_THRESHOLD; - } + /** + * Allows subclasses to conditionally opt-out of doing a backup, e.g. if + * auto save is enabled. + */ + protected abstract shouldScheduleBackup(workingCopy: IWorkingCopy): boolean; + + /** + * Allows subclasses to control the delay before performing a backup from + * working copy content changes. + */ + protected abstract getBackupScheduleDelay(workingCopy: IWorkingCopy): number; private scheduleBackup(workingCopy: IWorkingCopy): void { - if (this.backupsDisabledForAutoSaveables && !(workingCopy.capabilities & WorkingCopyCapabilities.Untitled)) { - return; // skip if auto save is enabled with a short delay + if (!this.shouldScheduleBackup(workingCopy)) { + return; // subclass prevented backup for working copy } // Clear any running backup operation @@ -137,7 +123,7 @@ export abstract class BackupTracker extends Disposable { this.logService.error(error); } } - }, BackupTracker.BACKUP_FROM_CONTENT_CHANGE_DELAY); + }, this.getBackupScheduleDelay(workingCopy)); // Keep in map for disposal as needed this.pendingBackups.set(workingCopy, toDisposable(() => { diff --git a/src/vs/workbench/contrib/backup/electron-sandbox/backupTracker.ts b/src/vs/workbench/contrib/backup/electron-sandbox/backupTracker.ts index 403322f11e2..f47ccd5545b 100644 --- a/src/vs/workbench/contrib/backup/electron-sandbox/backupTracker.ts +++ b/src/vs/workbench/contrib/backup/electron-sandbox/backupTracker.ts @@ -23,9 +23,21 @@ import { IEnvironmentService } from 'vs/platform/environment/common/environment' export class NativeBackupTracker extends BackupTracker implements IWorkbenchContribution { + // Delay creation of backups when working copy changes to avoid too much + // load on the backup service when the user is typing into the editor + private static readonly BACKUP_SCHEDULE_DELAY = 1000; + + // Disable backup for when a short auto-save delay is configured with + // the rationale that the auto save will trigger a save periodically + // anway and thus creating frequent backups is not useful + // + // This will only apply to working copies that are not untitled where + // auto save is actually saving. + private static readonly DISABLE_BACKUP_AUTO_SAVE_THRESHOLD = 1500; + constructor( @IBackupFileService backupFileService: IBackupFileService, - @IFilesConfigurationService filesConfigurationService: IFilesConfigurationService, + @IFilesConfigurationService private readonly filesConfigurationService: IFilesConfigurationService, @IWorkingCopyService workingCopyService: IWorkingCopyService, @ILifecycleService lifecycleService: ILifecycleService, @IFileDialogService private readonly fileDialogService: IFileDialogService, @@ -36,7 +48,24 @@ export class NativeBackupTracker extends BackupTracker implements IWorkbenchCont @IEditorService private readonly editorService: IEditorService, @IEnvironmentService private readonly environmentService: IEnvironmentService ) { - super(backupFileService, filesConfigurationService, workingCopyService, logService, lifecycleService); + super(backupFileService, workingCopyService, logService, lifecycleService); + } + + protected shouldScheduleBackup(workingCopy: IWorkingCopy): boolean { + if (workingCopy.capabilities & WorkingCopyCapabilities.Untitled) { + return true; // always backup untitled + } + + const autoSaveConfiguration = this.filesConfigurationService.getAutoSaveConfiguration(); + if (typeof autoSaveConfiguration.autoSaveDelay === 'number' && autoSaveConfiguration.autoSaveDelay < NativeBackupTracker.DISABLE_BACKUP_AUTO_SAVE_THRESHOLD) { + return false; // skip backup when auto save is already enabled with a low delay + } + + return true; + } + + protected getBackupScheduleDelay(): number { + return NativeBackupTracker.BACKUP_SCHEDULE_DELAY; } protected onBeforeShutdown(reason: ShutdownReason): boolean | Promise { 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 5e77da98cba..beb7ff089b0 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 @@ -69,9 +69,10 @@ class TestBackupTracker extends NativeBackupTracker { @IEnvironmentService environmentService: IEnvironmentService ) { super(backupFileService, filesConfigurationService, workingCopyService, lifecycleService, fileDialogService, dialogService, contextService, nativeHostService, logService, editorService, environmentService); + } - // Reduce timeout for tests - BackupTracker.BACKUP_FROM_CONTENT_CHANGE_DELAY = 10; + protected getBackupScheduleDelay(): number { + return 10; // Reduce timeout for tests } } -- GitLab