Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for transient workspaces that go away after restart or window reload #119695

Closed
alyssajotice opened this issue Mar 23, 2021 · 59 comments
Closed
Assignees
Labels
feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan workbench-multiroot Multi-root (multiple folders) issues
Milestone

Comments

@alyssajotice
Copy link

Version: 1.55.0-insider (user setup)
Commit: 111a6ce
Date: 2021-03-18T05:14:49.087Z
Electron: 11.3.0
Chrome: 87.0.4280.141
Node.js: 12.18.3
V8: 8.7.220.31-electron.0
OS: Windows_NT x64 10.0.19042

Possibly related to #110982

I have been working on finding a way to leave a Live Share session upon a user reload. Our current strategy, based on the discussion in #110982 was to delete the Live Share workspace files during deactivate. I have built out this approach but after the workspace has been deleted, VS Code has a workspace open with the title Visual Studio Live Share (Workspace). I made the following test extension to demonstrate the behavior we are seeing. Can you explain why this behavior is happening? Is there a way to close the current workspace during the reloading process to achieve our desired behavior?

Steps to Reproduce:

  1. Run the attached extension
  2. Open the folder C:\Users\UserProfile\AppData\Local\Temp\vsliveshare
  3. From the extension development host, join a Live Share session
  4. Observe the workspace be added to the vsliveshare folder
  5. From the command palette, run the command Delete temp workspace files upon reload
  6. Reload the window
  7. Observe workspace folder being deleted via the logs and the vsliveshare folder

Expected Behavior: VS Code will go back to being in a fresh state with no workspaces open
Actual Behavior: VS Code has the deleted workspace open and is in a bad state

extension.ts

import * as vscode from 'vscode';
import * as path from 'path';
import * as os from 'os';
import * as fs from 'fs';

export var shouldDeleteUponReload: boolean;

export function activate(context: vscode.ExtensionContext) {
	console.log('Congratulations, your extension "delete-workspace" is now active!');
	let disposable = vscode.commands.registerCommand('delete-workspace.shouldDeleteUponReload', () => {
		shouldDeleteUponReload = true;
	});
	context.subscriptions.push(disposable);
}

export async function deactivate() {
	const LIVESHARE_WORKSPACE_TEMP_DIR = path.join(os.tmpdir(), 'vsliveshare');
	console.log(shouldDeleteUponReload);
	if (shouldDeleteUponReload) {
		await fs.readdir(LIVESHARE_WORKSPACE_TEMP_DIR, (err, files) => {
			files.forEach(file => {
				let dir = LIVESHARE_WORKSPACE_TEMP_DIR + '\\' + file;
				fs.rmdir(dir, { recursive: true }, (err) =>{ console.log(err?.message); });
				console.log("Removed: " + dir);
			});
		});
	}
}

delete-workspace.zip

cc @bpasero

@bpasero
Copy link
Member

bpasero commented Mar 24, 2021

@alyssajotice I am very confused now because in #110982 (comment) you indicate that the workspace is properly left but you briefly see a file opened and I commented in #110982 (comment) that we preserve the state at least but the workspace should be empty.

So what is this issue about then? I just did a quick test to see if reload brings me back into a deleted workspace and could not reproduce:

  • open a workspace
  • delete workspace from disk
  • reload window
  • = window is empty

@bpasero bpasero added the info-needed Issue requires more information from poster label Mar 24, 2021
@alyssajotice
Copy link
Author

You are correct, when you manually delete the workspace from disk and reload, the window is empty and works as expected.

After establishing this proof of concept, I began adapting it for the Live Share extension scenario. Our goal is to leave a Live Share session when reloading by deleting the workspace files while deactivating.

When we do this (during deactivate), VS Code reopens the workspace. The end state shows the workspace is deleted but VS Code still has it open. Is there a reason why this would work properly when deleted manually but not when deleted in the extension's deactivate function?

Or, is there another approach we can take to achieve our goal, which is to close the workspace upon reloading?

@bpasero
Copy link
Member

bpasero commented Mar 24, 2021

@alyssajotice are you sure the delete actually happens? Can you try to replace that code with sync variants, I wonder if you simply do not get enough time in the deactivate to perform the delete.

I actually think we give the "old" extension host some time to shutdown and allow the window to reload so it is well possible that the file is not deleted when the window reloads. In other words, the window reload will not await the extension host from having finished to shutdown iirc, but maybe @alexdima can chime in on that.

@alyssajotice
Copy link
Author

I am able to see the workspace disappear from the file explorer, so I had assumed it had time to complete. I will try the sync code too.

@bpasero
Copy link
Member

bpasero commented Mar 24, 2021

@alyssajotice but it probably disappears after the window has already reloaded. The check happens very early now here:

configuration.workspace = await this.validateWorkspace(configuration);

Previously it happened inside the workbench itself quite late, so that is a possible explanation for the difference we see here.

@alexdima
Copy link
Member

In other words, the window reload will not await the extension host from having finished to shutdown iirc, but maybe @alexdima can chime in on that.

Yes, I confirm. The window (renderer process) does not wait in any way for the extension host process. So it is possible that reloading (which leads at this time to a new renderer process being created) will show the new window when the previous extension host is still executing. It is even possible to have two extension hosts (the old and the new) running at the same time if the old one takes 5s to shutdown and the new one is up and running within 5s.

Regarding the shutdown of the extension host, basically all active extensions are looped over and deactivate is called on them. The extensions can return a promise from deactivate. All the promises are joined and if they take under 5s, the process exits when those promises complete. If the returned promises do not resolve in 5s, then the process exits regardless.

@bpasero
Copy link
Member

bpasero commented Mar 25, 2021

@alexdima would it make sense to revisit that decision and participate in the workbench lifecycle? I think today we are actually just lucky that nothing serious is happening but there is a race condition here:

  • user has VSCode running with an extension that connects to a local DB with write lock (only 1 connection allowed)
  • the extension releases this lock from the deactivate method
  • user reloads the window
  • this will trigger the deactivate method in the extension
  • meanwhile the window continues to load, a new extension host is spawned and the extension again connects to the DB

=> at this point, if the deactivate took longer than the window managing to load, the extension would basically fail to connect to that DB

We already have code in place that allows to join a shutdown/reload with a Promise, for example here:

private _onWillShutdown(event: WillShutdownEvent): void {
// If the extension development host was started without debugger attached we need
// to communicate this back to the main side to terminate the debug session
if (this._isExtensionDevHost && !this._isExtensionDevTestFromCli && !this._isExtensionDevDebug && this._environmentService.debugExtensionHost.debugId) {
this._extensionHostDebugService.terminateSession(this._environmentService.debugExtensionHost.debugId);
event.join(timeout(100 /* wait a bit for IPC to get delivered */), 'join.extensionDevelopment');
}
}

The downside of this approach is that it would obviously make shutdown/reload slower so if we want to go down this path, we would need to ensure that we don't wait for any longer then maybe 100ms, which again does not guarantee that we are not racing again...

A very alternate solution with sandbox in mind would be to allow a window to reconnect to the same extension host after a reload, but that is probably wishful thinking...

@alexdima
Copy link
Member

@bpasero I think this particular use-case (of locking a database) has a lot more problematic cases. If an extension locks a database in the folder, there are multiple ways this can fail today. It is possible that two extension hosts will overlap in case of a window reload (like discussed in this thread), but it is also possible to open the same folder twice: once as a folder, and again as a folder part of a multi-folder workspace, so the locking case can also occur in other scenarios. If this turns up in a real-world scenario, maybe we can tackle this by introducing new vscode API such that specific extensions can signal that they don't want this overlap. i.e. and the implementation can be that they will not be activated in the new extension host until the old extension host has quit.

IIRC the reason why we don't block the lifecycle of vscode on the extension host is that we want when the user presses x that the window closes ASAP. We don't want that the window lingers for 5s before going down. I think we compared this to Windows shutting down and none of us liked how on Windows the shutdown is delayed by running programs. Even rendering the blocking programs doesn't improve the UX, it still is annoying that the machine does not shut down. I think we decided we don't want VS Code's UI to be delayed by extension shutdown, especially since a single misbehaving extension could cause a 5s stall. If you think we should delay the vscode UI and block on the extension host, we can discuss this in the team. But I personally think the current behavior is good.

To be honest, I think this issue is about LiveShare wanting to have a "temporary" workspace, which is a feature VS Code does not support. Maybe it would be easier to implement this feature (e.g. temporary workspaces do not appear in the recent list, they don't get restored after an update or after quitting and restarting vscode, etc.). If we would support the feature that LiveShare wants, we wouldn't have to change the shutdown behavior of vscode to give the extension sufficient time to delete a file on disk which is, to be honest, a workaround for a missing VS Code feature.

@bpasero bpasero added feature-request Request for new features or functionality workbench-multiroot Multi-root (multiple folders) issues and removed info-needed Issue requires more information from poster labels Mar 26, 2021
@bpasero bpasero removed their assignment Mar 26, 2021
@bpasero bpasero added this to the Backlog milestone Mar 26, 2021
@bpasero bpasero changed the title VS Code keeps deleted workspaces open Allow for temporary workspaces that go away after restart or window reload Mar 26, 2021
@alyssajotice
Copy link
Author

I looped in @fubaduba about our conversation and we agree that this is the optimal direction for our Live Share scenario. Do you have any information about a timeline for this feature?

1 similar comment
@alyssajotice
Copy link
Author

I looped in @fubaduba about our conversation and we agree that this is the optimal direction for our Live Share scenario. Do you have any information about a timeline for this feature?

@alyssajotice
Copy link
Author

Hey @bpasero, any updates about this?

@bpasero
Copy link
Member

bpasero commented Apr 27, 2021

Spend some more time thinking about this. I wonder if maybe VSCode core could register a file system provider (transient:/) that will be valid only once in a window but anytime you try to reload or restore that same URL in the window, it will instead fallback to an emtpy window.

It would then be LiveShare's responsibility to open such a workspace. Instead of creating a temporary file on disk to open, it would instruct VSCode to open this transient form.

There is some questions though:

  • how do we make sure the contents of the workspace properly reach VSCode (maybe encode as part of the URL?)
  • how can VSCode actually invalidate the URL when the window reloads?

For the latter, maybe a timestamp could be part of the URL and the URL only stays valid for a few seconds? In other words, if that URL is not opened in VSCode in 10-20s, it would simply be an empty window?

Other things that would need to happen is that transient URLs do not appear in the list of recently opened workspaces, etc.

Thoughts?

@lostintangent
Copy link
Member

lostintangent commented Apr 27, 2021

I like the transient FS idea! Another random thought: what about adding a vscode.workspace.openWorkspace API, that took a WorkspaceConfiguration, which was equivalent to the contents of a .code-workspace file? This would kind of mirror the vscode.debug.startDebugging API, which lets you launch the debugger with an in-memory definition, as opposed to relying on a launch.json file.

Behind the scenes, that could store/read the file using the transient:/ file system provider, but it could abstract away that detail, and provide a super-simple solution for programmatically launching workspaces. That API might also be useful to other extensions besides just Live Share?

@daytonellwanger
Copy link

daytonellwanger commented Apr 27, 2021

how do we make sure the contents of the workspace properly reach VSCode (maybe encode as part of the URL?)

Today we write a lot of info to mementos for things like this. Would that work?

edit: no, this doesn't work for scenarios where we're opening the workspace from outside our extension. In that case, I like the URL idea. This same concern applies for the API proposed by @lostintangent

One more consideration is that there are instances where we do want to be able to reload and open the same transient workspace. But I guess it's not much effort to go through the process of re-registering it in those scenarios.

Also +1 to @lostintangent 's suggestions.

@bpasero
Copy link
Member

bpasero commented Apr 27, 2021

what about adding a vscode.workspace.openWorkspace API

Well, we already have a vscode.open command which typically is to be used to open workspaces so maybe it could be part of that command to pass over the contents of the file and then VSCode has to somehow ensure these contents are registered to the transient file system provider.

To clarify what this means:

  • a transient workspace URL can only ever be opened once, any subsequent loads will result in empty window (either from a reload operation or by simply restarting VSCode)
  • the entire contents of the workspace file can be passed over from the extension but nothing else (e.g. no other file, the provider is essentially a 1 file provider)
  • transient workspace URIs never appear in the history (recently opened, OS dock) and run with an in-memory storage service so that no workspace state is accumulated

I think if we end up with this approach it would require that LiveShare can only use these kind of workspaces from an extension and nothing else. I.e. it would not be possible to open such a workspace from a link handler e.g. from the browser that brings up VSCode.

Another thought is whether we can support this in web too, but I would think that if we register such a file system provider in web it might work. Though arguable it will be harder to preserve the contents for the workspace.

Maybe if the URL itself had all the configuration data as query param, this would be easier to transport the data into the provider.

@alyssajotice
Copy link
Author

Hey @bpasero, this sounds great. The only other criteria we have is that we need to be able to launch VS Code from the browser and open a temp workspace where we can specify settings. Perhaps we could encode the info in the URL? Our workspace records are small.

@alyssajotice
Copy link
Author

I saw that researching this feature was on the Iteration Plan for May, do you have any updates?

@bpasero
Copy link
Member

bpasero commented Jun 10, 2021

@alyssajotice this will slip into the next milestone for sure

Some more questions maybe you can answer:

  • is this a requirement for desktop only or web as well (i.e. are guests having the choice of joining with desktop and web or only desktop)
  • how does a guest join the session, is it always through an action triggered by the LiveShare extension or are there other ways of joining, can you list the ways?
  • is the workspace that is used for the guest changed after it has been opened or does it contain all the data upfront (settings, folders)
  • are the folders that are opened in this case always virtual folders (i.e. with a file system registered by LiveShare extension)
  • is there ever a situation where the guest needs to reload the window
  • can the guest edit data in the session or is it entirely readonly? and if not, what would you expect to happen with dirty files (or untitled files) that have not been saved at the moment the user closes the session

@alyssajotice
Copy link
Author

is this a requirement for desktop only or web as well (i.e. are guests having the choice of joining with desktop and web or only desktop)

This is a requirement for web. You can join a Live Share session in the browser and also from VS Code.

how does a guest join the session, is it always through an action triggered by the Live Share extension or are there other ways of joining, can you list the ways?

I believe that guests can only join through an action by the Live Share extension. Deferring to @daytonellwanger to clarify this.

is the workspace that is used for the guest changed after it has been opened or does it contain all the data upfront (settings, folders)

It contains all of the data upfront.

are the folders that are opened in this case always virtual folders (i.e. with a file system registered by LiveShare extension)

Yes, we create our own Live Share workspace for this purpose.

is there ever a situation where the guest needs to reload the window

Yes. Right now we have two situations where this happens. The first is when an anonymous guest signs in once already in a session. In order to complete the sign in process, we must reload. The second is when we detect a desync. This triggers a reload and reconnection.

can the guest edit data in the session or is it entirely readonly? and if not, what would you expect to happen with dirty files (or untitled files) that have not been saved at the moment the user closes the session

We offer both read/write and read-only sessions for guests. For the read/write sessions, we allow them to edit files. If they edit a file and leave the session, the unsaved changes are still present on the host's machine and the host can decide how to proceed. The same thing happens for untitled files.

@bpasero
Copy link
Member

bpasero commented Jun 16, 2021

@alyssajotice thanks, will follow up with the answers shortly, but quick question: back to the original issue that after a window reload, the workspace restores, I was wondering: maybe for this particular case we could do a one off and still validate the workspace file exists only for window reloads and a bit delayed after the workbench has started.

I wonder what kind of user experience that should be because a user might briefly see a workspace (though it will appear almost empty) and then I am wondering if we should popup a modal dialog telling that the workspace is gone with the only option to close that workspace and thus return to an empty window? Does that make sense?

I bring this up because the solution for transient workspaces might be more involved and I might not be able to work on that task short term.

@alyssajotice
Copy link
Author

I tried this out by first setting the transient workspace setting to 'true' for the temp Live Share workspace and reloading while in the session. Unfortunately, the workspace was reloaded.

Here are the steps I did:

  1. Share a workspace
  2. Join the session from an extension development host that had code to set the transient setting
  3. Check that the transient property was set to 'true' in the workspace file
  4. Reload the window
  5. Observe the session being rejoined

transient-setting

Please let me know if I did something incorrectly.

Another thought about this strategy: Could we apply this same behavior to closing and reopening VS Code, rather than reloading? Because you were able to implement this, it seems that you are able to determine whether a VS Code window is a result of a reload versus a reopen. Our priority for this feature is to not rejoin workspaces when reopening. Not rejoining when reloading is a secondary goal.

Since this property will not be read when initially opening or restoring, you would still have to continue to delete the temporary workspace when your extension is deactivated.

Before we move forward with this, the one concern I have is ensuring we have enough time to delete the workspace when deactivating. Can we guarantee this?

@bpasero
Copy link
Member

bpasero commented Jun 24, 2021

@alyssajotice please try with "transient": true and not "transient": "true"

@bpasero
Copy link
Member

bpasero commented Jun 24, 2021

Another thought about this strategy: Could we apply this same behavior to closing and reopening VS Code, rather than reloading? Because you were able to implement this, it seems that you are able to determine whether a VS Code window is a result of a reload versus a reopen. Our priority for this feature is to not rejoin workspaces when reopening. Not rejoining when reloading is a secondary goal.
Before we move forward with this, the one concern I have is ensuring we have enough time to delete the workspace when deactivating. Can we guarantee this?

Yeah that makes sense to me. To clarify:

  • explicitly asking VSCode to open a transient workspace always works
  • VSCode would however never restore such a workspace (e.g. when restarting)
  • VSCode would never allow to reload such a window (what we have today already)

If we were to implement such a strategy, I think it would be safe for you to not delete the workspace on deactivate (though arguably you should continue to do so to clean up the disk).

PS: are there more things you currently do with these transient workspaces that maybe we should fold into this new property? I think we added a command to explicitly remove a workspace from the list of recently opened workspaces, but I would argue a transient workspace should never be added in the first place.

@bpasero bpasero self-assigned this Jun 24, 2021
@bpasero bpasero modified the milestones: Backlog, June 2021 Jun 24, 2021
@bpasero bpasero changed the title Allow for temporary workspaces that go away after restart or window reload Allow for transient workspaces that go away after restart or window reload Jun 24, 2021
@alyssajotice
Copy link
Author

Thank you for the clarification.

VSCode would however never restore such a workspace (e.g. when restarting)

I was previously confused about when this would take effect, but I understand now it is in all reload, and restart scenarios.

Let me try the property value without the quotes and let you know if there are any other properties we will need.

@bpasero
Copy link
Member

bpasero commented Jun 24, 2021

@alyssajotice I pushed a change that will not attempt to restore windows from a previous session that are for transient workspaces. In addition, the history entry will also be removed in that case. Let me know how this ends up working for you in tomorrows insider build.

// If the workspace is transient and we are to ignore
// transient workspaces, reject it. Also remove traces
// in the history if any.
if (workspace.transient && options.rejectTransientWorkspaces) {
this.workspacesHistoryMainService.removeRecentlyOpened([URI.file(path)]);
return undefined;
}

@bpasero
Copy link
Member

bpasero commented Jun 24, 2021

Pushed another change to not add transient workspaces to the list of recently opened 👍

@alyssajotice
Copy link
Author

alyssajotice commented Jun 24, 2021

These new changes sound great and I'll try them out tomorrow. However, I am experiencing the same behavior as shown in my last gif when setting transient to true rather than "true". When I reload the workspace, it reopens the workspace and rejoins the Live Share session. Do you think the problem could be related to me using the extension development host for testing? Sometimes I am unsure of how it manages changes when reloading/reopening.

I updated yesterday to this version:

Version: 1.58.0-insider (user setup)
Commit: 9744231
Date: 2021-06-23T05:13:08.071Z
OS: Windows_NT x64 10.0.19043

@bpasero
Copy link
Member

bpasero commented Jun 25, 2021

@alyssajotice can you distill a minimal repro from a small extension that I can run to test this? Running from the extension host should not make a difference imho.

@alyssajotice
Copy link
Author

alyssajotice commented Jun 25, 2021

I think the easiest repro is simply opening a workspace with the transient property manually set and trying to reload and see what happens.

Here is the workspace file I used:

{
    "folders": [
        {

        }
    ],
    "settings": {
        "transient": true
    }
}

Steps:

  1. Create an empty workspace file with the transient property set to true
  2. Open the workspace in VS Code Insiders
  3. Reload the window

Expected: The reloaded window is an empty window.
Actual: The window reopens the workspace.

transient-test

Please let me know if I am expecting the right behavior here. I can also make a repro extension if that will help.

@bpasero
Copy link
Member

bpasero commented Jun 25, 2021

@alyssajotice wow this is an expensive misunderstanding, try with:

{
    "folders": [
        {

        }
    ],
    "transient": true
}

@alyssajotice
Copy link
Author

That worked as expected 👍 Sorry about the misunderstanding!

@bpasero
Copy link
Member

bpasero commented Jun 25, 2021

Np, I felt a setting is not ideal because it may appear to the user as something that can be changed, when it actually cannot be changed as a setting.

@bpasero
Copy link
Member

bpasero commented Jun 25, 2021

Marking as closed and will be covered in next weeks testing week.

@bpasero bpasero closed this as completed Jun 25, 2021
@bpasero bpasero added on-release-notes Issue/pull request mentioned in release notes on-testplan labels Jun 25, 2021
@alyssajotice
Copy link
Author

I noticed a few unexpected behaviors, so I added comments on the testing issue.

@alyssajotice
Copy link
Author

@bpasero do you have any advice for how to change this value while the extension is running and the workspace is open? Is there a built-in method you can use to update workspace settings?

Otherwise I can just read and rewrite the setting to the file, but I wanted to check first.

@bpasero
Copy link
Member

bpasero commented Jun 30, 2021

@alyssajotice there is no API currently to change values in the workspace file, you need to edit it raw and write it back. The file is accessible via workspace.workspaceFile API.

I believe we might have utilities in NPM for writing JSON without breaking the formatting. @aeschli can you maybe comment whether your JSON editing code is reusable or is it not?

@alyssajotice
Copy link
Author

I was able to write to the JSON pretty easily, so I don't need the editing code. Thanks though!

@aeschli
Copy link
Contributor

aeschli commented Jul 1, 2021

The jsonc-parser node module has the capability to modify JSON with minimal edits.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan workbench-multiroot Multi-root (multiple folders) issues
Projects
None yet
Development

No branches or pull requests

6 participants