Implement Hot Exit by Tyriar · Pull Request #14115 · microsoft/vscode

Tyriar · 2016-10-20T17:36:11Z

Hot Exit: Support hot exit on folder workspaces #14200 Hot Exit: Support hot exit on folder workspaces
Preserve files on exit (aka hot exit) #12400 Original hot exit PR
Hot Exit: avoid direct calls to fs.readdir()/fs.readdirSync() #14055 Hot Exit: avoid direct calls to fs.readdir()/fs.readdirSync() (aab05a8)
Launch VSCode target launches multiple window #14060 Correctly clean up workspace backups when exiting (873d095)
When hot exit restores neither files or untitled files from backup, no files that were open but not dirty are being restored #14100 & Hot Exit: Restore untitled files from within UntitledEditorModel #14067 Have TextFileEditorModel handle restores (b23229f)
Hot Exit: do not load backup files to restore sync on startup in the renderer #14068 Hot Exit: do not load backup files to restore sync on startup in the renderer
Hot Exit: push renderer backup logic into new backup service #14065 Hot Exit: push renderer backup logic into new backup service
Hot Exit: backupService.ts feedback #14066 Hot Exit: backupService.ts feedback
Big refactor to throw as much as possible into the now several Backup*Services and leverage events
Hot Exit: textFileServices.ts feedback #14079 Remove public backup()
Hot Exit: Careful when you veto the shutdown from failing backups #14074 Hot Exit: Careful when you veto the shutdown from failing backups
Introduce on model dispose events to ITextFileEditorModelManager and IUntitledEditorService and track in BackupModelService #14181 Introduce on model dispose events to ITextFileEditorModelManager and IUntitledEditorService and track in BackupModelService
Don't hash untitled resource names for their backup paths #14180 Don't hash untitled resource names for their backup paths
Merge backup path logic from node/fileService.ts into backupService.ts #14037 Merge backup path logic from node/fileService.ts into backupService.ts
Hot Exit: backupService saves and loads a LOT #14064 Hot Exit: backupService saves and loads a LOT - Resources are no longer tracked at all in workspace.json and instead the actual backup path is checked
Hot Exit: deal with backup workspaces that do not exist anymore #14063 Hot Exit: deal with backup workspaces that do not exist anymore
Hot Exit: backup workspace handling should all happen on main side #14056 Hot Exit: backup workspace handling should all happen on main side
Hot exit: Reloading the window should restore all files in the window #14330 Hot exit: Reloading the window should restore all files in the window
Hot Exit: restoring file model issues #14132 Hot Exit: restoring file model issues
Hot Exit: Support multiple folder restore #13305 Hot Exit: Support multiple folder restore (5bb2cb8, 57c6e72)
Hot exit on Mac currently allows closing the final single window #15134 Hot exit on Mac currently allows closing the final single window
Cannot exit empty workspace windows on Windows when hot exit is enabled #15130 Cannot exit empty workspace windows on Windows when hot exit is enabled
Close folder does not clear the backup workspace path #15137 Close folder does not clear the backup workspace path

mention-bot · 2016-10-20T17:36:13Z

@Tyriar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero, @joaomoreno and @egamma to be potential reviewers.

…68_load_backups_on_main_process

Fixes #14066

Fixes #14053

It's no longer needed now that the workspace is restored.

bpasero · 2016-10-21T06:48:14Z

@Tyriar there are merge conflicts?

Fixes #14074

…_process

bpasero · 2016-10-21T13:40:50Z

I merged.

bpasero · 2016-10-21T13:51:08Z

 		@IEventService private eventService: IEventService,
 		@ITextFileService private textFileService: ITextFileService,
-		@IBackupService private backupService: IBackupService
+		@IBackupFileService private backupFileService: IBackupFileService


Unused member

bpasero · 2016-10-21T13:57:17Z

-					return path.workspacePath;
-				}
-				return platform.isLinux ? path.workspacePath : path.workspacePath.toLowerCase();
+			const workspacesWithBackups = this.backupService.getWorkspaceBackupPathsSync();


As reported in a different issue that code should really move out of the open() method and only be used from the first startup code. Otherwise it makes it very hard to understand what this method is actually doing (it is already quite complex).

bpasero · 2016-10-21T14:01:46Z

 	) {
 		super();
 		this.resource = resource;
+		this.restoreResource = restoreResource;


As mentioned before I think we can get rid of the restoreResource also for untitled. The untitled model should go to the backup service and ask for available backups with the resource URI, similar to how text files now ask.

bpasero · 2016-10-21T14:03:04Z

-		this.backupPromises.push(promise);

-		return promise;
+		// TODO: Introduce an event on IUntitledEditorService so this can be moved to BackupModelService?


Yes, please!

bpasero · 2016-10-21T14:03:37Z

 		@IFileService private fileService: IFileService,
-		@IConfigurationService private configurationService: IConfigurationService
+		@IConfigurationService private configurationService: IConfigurationService,
+		@IBackupService private backupService: IBackupService,


Unused service?

bpasero · 2016-10-21T14:04:44Z

-					return false; // the backup went smoothly, no veto
-				});
-			});
+		if (this.backupService.isHotExitEnabled) {


Let the backup service listen to the lifecycle method and do it in there. Here I would only check if hot exit is enabled to see if confirmation is needed.

I did this first but regardless there needs to be some communication between the 2 services. BackupService needs to essentially be run both before (check if hot exit) and after (cleanup backups) TextFileService.beforeShutdown. Also the shutdown event doesn't work as it has to be synchronous and BackupService,cleanupBackupsBeforeShutdown is async.

Ok maybe textFileServices should use eventing or similar so that we can further decouple this into the backup service.

bpasero · 2016-10-21T14:05:22Z

 					}

-					return this.cleanupBackupsBeforeShutdown();
+					return this.backupService.cleanupBackupsBeforeShutdown(); // all good, no veto


All of the cleanupBackupsBeforeShutdown should move out into the backup service that itself should listen to lifecycle events.

bpasero · 2016-10-21T14:06:28Z


-		this.fileService.discardBackup(this.resource);
+		// TODO: Can this be moved to BackupModelService?
+		this.backupFileService.discardAndDeregisterResource(this.resource);


Yes, I suggest another event on the service maybe when this model gets disposed.

bpasero · 2016-10-21T14:09:35Z

 		return true;
 	}

+	public get onModelContentChanged(): Event<TextFileModelChangeEvent> {


I see the need for an event when untitled or text files are changing so that the backup service can do its business, however I fear that this comes at the cost of spam: you will see many model content change events while the user is typing, so basically each key press will trigger this event. Performance should always be something to be concerned of, especially when it is typing performance we are talking about.

Some ideas:

buffer the model content change event and only start to emit the change after a certain delay

do not always create an event object to send around

I think buffering would be a good idea anyway because backups should also not be created without some delay.

bpasero · 2016-10-21T14:10:17Z

@Tyriar looking a lot better. I like the direction. My only concern besides the comments I added is about the amount of events that are being sent now while typing. See my comment in the review on that topic.

…ot_exit/14068_load_backups_on_main_process

…68_load_backups_on_main_process

bpasero · 2016-11-10T05:59:14Z

@Tyriar replied to your comments, let me know when I should review again.

…68_load_backups_on_main_process

Tyriar · 2016-11-11T03:39:42Z

@bpasero ready for another review! If you consider any of these issues to be must fix before merge please label them with important. I want to polish the PR then merge it so we can start getting some testing from Insiders and move to smaller changes that are easier to work with. Thanks 😃

…_process

bpasero · 2016-11-11T07:04:09Z

+				fs.mkdirSync(this.backupHome);
+			}
+			fs.writeFileSync(this.workspacesJsonPath, JSON.stringify(this.workspacesJsonContent));
+		} catch (ex) {


@Tyriar maybe add at least some logging here to be able to find out about issues more easily.

bpasero · 2016-11-11T07:13:02Z

@@ -0,0 +1,164 @@
+/*---------------------------------------------------------------------------------------------


@Tyriar there are still warnings in this file

bpasero · 2016-11-11T07:16:59Z

@Tyriar only few comments left. my assumption is that you extracted the bigger feedback I gave into issues to look into and that is fine so that we can continue on master in these.

Tyriar · 2016-11-11T16:35:39Z

Fix warnings in 97371ad (not sure why I can't reply to that comment).

Tyriar · 2016-11-11T16:39:01Z

@bpasero yes we've been tracking all the comments in issues, the important ones have are in the November milestone.

…68_load_backups_on_main_process

Tyriar · 2016-11-11T20:18:11Z

Got the go ahead from @bpasero to merge, all tests are passing and did a manual smoke test on Mac and Windows to double check nothing has regressed since our internal test the other day. Merging! 🎆

bpasero · 2016-11-15T05:56:16Z

+			const workspacesWithBackups = this.backupService.getWorkspaceBackupPaths();
+
+			workspacesWithBackups.forEach(workspacePath => {
+				if (!fs.existsSync(workspacePath)) {


@Tyriar would it be possible that we have a backup workspace without backups to restore? maybe we should add more checks here that a backup workspace path should only cause a window to open if it has backups, otherwise it gets deleted. I am thinking of situations where we the backup workspace gets out of sync with the backups within (for whatever reason - e.g. crash). This would prevent bugs where multiple windows just open even though there are no backups.

continued in #15499

Commented on #15499.

bpasero · 2016-11-15T05:58:48Z

 		delete configuration.filesToCreate;
 		delete configuration.filesToDiff;

+		// Update untitled files to restore so they come through in the reloaded window


@Tyriar I am thinking more and more that untitled editors with backups should not be loaded from the main side but rather from the window itself. Files for example restore because we store their state into local storage (via IEditorInputFactory - see https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/common/editor/editorStacksModel.ts#L621).

If we add a factory for untitled editors, we would just rely on that to bring the untitled back on reopen. It would also fix the issue that untitled would reopen at the right location where they where before.

I filed #15498 to continue the discussion

bpasero · 2016-11-15T05:59:45Z

+					return;
+				}
+
+				const untitledToRestore = this.backupService.getWorkspaceUntitledFileBackupsSync(Uri.file(workspacePath)).map(filePath => {


@Tyriar same comment here about how to restore untitled files. This would not be needed if untitled had input factory.

continued in #15498

bpasero · 2016-11-15T06:00:44Z

 		}

+		// Register new paths for backup
+		this.backupService.pushWorkspaceBackupPathsSync(iPathsToOpen.filter(p => p.workspacePath).map(p => Uri.file(p.workspacePath)));


@Tyriar should this only happen if hot exit is enabled in settings?

It should still happen so that it gets recorded and restored if there was a crash.

bpasero · 2016-11-15T06:01:40Z

 			};
 		});

+		// Add any untitled files to be restored from backup


@Tyriar again this would not be needed if we had untitled editor factory.

Load backup resources exclusively on main process

5415009

Fixes #14068

Tyriar added the workbench-hot-exit Preservation of unsaved changes across restarts label Oct 20, 2016

Tyriar added this to the October 2016 milestone Oct 20, 2016

Tyriar assigned bpasero Oct 20, 2016

msftclas added the cla-already-signed label Oct 20, 2016

Tyriar added 3 commits October 20, 2016 12:22

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/140…

99d13ab

…68_load_backups_on_main_process

Split BackupService into main/renderer process versions

c328d1b

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/140…

42215ef

…68_load_backups_on_main_process

Tyriar mentioned this pull request Oct 20, 2016

Hot Exit: push renderer backup logic into new backup service #14065

Closed

Tyriar added 3 commits October 20, 2016 15:39

Remove BackupService.setCurrentWorkspace

0db499d

Fixes #14066

Remove redundant fs.existsSync call

bd9ecb2

Fixes #14066

Remove unused imports

2bdec4d

Fixes #14053

Tyriar changed the title ~~Load backup resources exclusively on main process~~ Fix various hot exit issues Oct 20, 2016

Huge refactor to move everything into Backup*Service

ec638ab

Tyriar mentioned this pull request Oct 21, 2016

Hot Exit: textFileServices.ts feedback #14079

Closed

Tyriar added 2 commits October 20, 2016 22:59

Get rid of remnants of filesToRestore

a29cc43

It's no longer needed now that the workspace is restored.

Move untitled model backups to BackupModelService

5137550

Tyriar and others added 4 commits October 20, 2016 23:58

Remove duplicate untitled save code

105dd5b

Handle last chance backup save failure

a33a4ba

Fixes #14074

Remove setRestoreFromBackup

6a10eeb

Merge branch 'master' into tyriar/hot_exit/14068_load_backups_on_main…

f8ed5aa

…_process

bpasero requested changes Oct 21, 2016

View reviewed changes

Tyriar mentioned this pull request Oct 21, 2016

Buffer untitled/text file model changed events and make the event type consistent #14186

Closed

Tyriar force-pushed the tyriar/hot_exit/14068_load_backups_on_main_process branch from fcf9dba to f8ed5aa Compare October 21, 2016 20:19

Tyriar changed the title ~~Fix various hot exit issues~~ Implement Hot Exit Oct 21, 2016

Merge commit '2066424fdf110d3ff970f50305076e02fbe6d704' into tyriar/h…

4631368

…ot_exit/14068_load_backups_on_main_process

Tyriar added 5 commits November 9, 2016 21:09

Remove unused code

2164afd

Remove unused services from UntitledEditorModel

d10bfb8

Make TextFileService.getDirtyFileModels private again

e0b7451

Fallback to file content if restore content fails

940fc04

Remove comment

d346ed2

Tyriar mentioned this pull request Nov 10, 2016

Add test coverage for new events onModelDisposed and onModelContentChanged in textFileEditorModelManager.ts #15279

Closed

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/140…

b74fe8e

…68_load_backups_on_main_process

Tyriar mentioned this pull request Nov 10, 2016

Investigate BackupService listening to ILifecycleService.onWillShutdown instead of being called from TextFileService #15280

Closed

Tyriar added 2 commits November 10, 2016 19:00

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/140…

8261148

…68_load_backups_on_main_process

Move BackupMainService files to vs/platform

b91bde2

Tyriar and others added 2 commits November 10, 2016 19:47

Shuffle BackupMainService around

1558036

Merge branch 'master' into tyriar/hot_exit/14068_load_backups_on_main…

2748fc8

…_process

bpasero requested changes Nov 11, 2016

View reviewed changes

Fix backupFileService.test.ts warnings

97371ad

Add logs to BackupMainService.saveSync

46be928

Merge remote-tracking branch 'origin/master' into tyriar/hot_exit/140…

f670d11

…68_load_backups_on_main_process

Tyriar merged commit 022ee04 into master Nov 11, 2016

Tyriar deleted the tyriar/hot_exit/14068_load_backups_on_main_process branch November 11, 2016 20:18

bpasero reviewed Nov 15, 2016

View reviewed changes

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

		@@ -0,0 +1,164 @@
		/*---------------------------------------------------------------------------------------------

Conversation

Tyriar commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Oct 20, 2016

Uh oh!

bpasero commented Oct 21, 2016

Uh oh!

bpasero commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero commented Oct 21, 2016

Uh oh!

bpasero commented Nov 10, 2016

Uh oh!

Tyriar commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero commented Nov 11, 2016

Uh oh!

Tyriar commented Nov 11, 2016

Uh oh!

Tyriar commented Nov 11, 2016

Uh oh!

Tyriar commented Nov 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Tyriar commented Oct 20, 2016 •

edited

Loading

Tyriar commented Nov 11, 2016 •

edited

Loading