Investigation: do not restart Extension host when first folder changes

[bpasero]

Today we restart the extension host whenever the first folder changes because workspace.rootPath is always set to that first folder and it never used to change after the extension host started once.

If we would not restart the extension host anymore an extension:

• could use workspace.rootPath as before, it would still point to the first folder always (which can be undefined if the workspace contains no folders)
• could subscribe to the onDidChangeWorkspaceFolders event to get notified about updates
• would see the value of workspace.rootPath changing during runtime whenever the first folder changes

//cc @sandy081 @jrieken

[roblourens]

This is affecting Live Share because they want to be able to track workspace changes during a live share session. If the EH reloads, then the host will have to restart their session.

If we wanted to more aggressively deprecate rootPath, we could make it undefined whenever a workspace is open. I wonder how many extensions still rely on it.

FYI @cait1inhennessy

[roblourens]

As a workaround, they can keep a dummy folder in the workspace during a session, but the user needs to know to not touch that folder, so it's not a very good workaround.

[bpasero]

@roblourens

This is affecting Live Share because they want to be able to track workspace changes during a live share session. If the EH reloads, then the host will have to restart their session.

To be clear: the transition from an empty window or single-folder workspace to a multi-root workspace will always require an extension host restart because the workspace ID changes and thus the state location for example. Please check with them if they really start from an empty workspace and then add a folder.

[roblourens]

Yeah, we realized that, and they are able to start from an actually empty workspace. I think that if you cause a window reload, it will at least be obvious why your liveshare session terminated.

[bpasero]

I am all up for making this change, but we have to decide if:

• rootPath is undefined in multi-root
• rootPath changes in multi-root based on what is the first folder

I feel the former is a change with more impact on todays extensions compared to the latter.

From the API

/**
 * ~~The folder that is open in the editor. `undefined` when no folder
 * has been opened.~~
 *
 * @deprecated Use [`workspaceFolders`](#workspace.workspaceFolders) instead.
 */
export const rootPath: string | undefined;

I think we need to bring this up in an API call to decide, but I will not have cycles. This needs a champion to drive.

[roblourens]

I think we need an idea of how many extensions this would affect. @sandy081 I am using your tool but it seems to be showing me old versions of extensions that used this api, how do I limit it to newer versions?

I think the first option would actually be less impactful. It would mean that some extensions can't work in a multiroot workspace, but they should handle the case of no folder already for an empty window. The second option would break an assumption that those extensions may be making, that rootPath will not change, and we don't know what that would do.

I can bring it up in an API call.

@cait1inhennessy we will discuss it some more because I think this is the right change to make, but don't count on us changing it this month, you will probably have to plan to use the workaround with a dummy folder in the short term.

[jrieken]

@roblourens What is your thinking when the first workspace-folder isn't a folder on disk. E.g what would rootPath be when the first folder is fooScheme://barServer/bazz/path? Do you want to go with undefined or just pretend it's /bazz/path?

[jrieken]

I think the first option would actually be less impactful.

Oh, ignore my last comment...

but they should handle the case of no folder already for an empty window.

From what I remember when we checked back then extensions often show a message like "Open a folder first"

[bpasero]

Yes, now that I think about it, I would also prefer that rootPath is undefined once you are in a multi-root workspace (independent from how many folders you configure). That is a scenario that every extension out there must gracefully handle already, so we will not break any assumptions. It may however result in certain extensions becoming useless (because maybe no longer maintained).

[jrieken]

step 1: know how many users use multi root workspaces and could be affected by this step 2: know what extensions will be affected by this and how

[bpasero]

@roblourens feel free to delete WorkspaceChangeExtHostRelauncher once decided that we go with the approach outlined.

rootPath is defined here and would need to be changed accordingly.

If you cannot reserve time this milestone, feel free to move back to me "On Deck".

[jrieken]

If you cannot reserve time this milestone,

Since this is a high risk change I would not do anything else than gathering data this milestone. Maybe show an aggressive prompt when developing extensions that use this API.

Also adding @dbaeumer since I believe that the LSP-client has a reason to use the API (maybe not anymore?)

[roblourens]

Yeah I think we shouldn't make any actual change until next month.

I didn't see .rootPath being used anywhere in an LSP extension or vscode-languageclient but maybe there is something I don't see @octref or @dbaeumer ?

Looking at a fresh copy of the 500 top extensions, I see workspace.rootPath has 573 results in 239 files. Some of these are falling back on rootPath from workspaceFolders, I guess for back-compat, and that's ok. But quite a few simply depend on it.

This even includes popular extensions and first-party extensions like the GHPR extension, some Azure extensions, Python, Go, Powershell, Material Icon theme, Ruby, settings sync, and more.

So that's worse than I was expecting and makes me think that support for multiroot workspaces in some scenarios might be worse than we realize. So I think we should start by just filing a whole lot of issues for this on extensions before we make any changes at all. Also we could print a deprecation warning in the console when rootPath is used.

[bpasero]

Also we could print a deprecation warning in the console when rootPath is used.

That should imho be something we do for every deprecated piece of API we have.

So I think we should start by just filing a whole lot of issues for this on extensions before we make any changes at all.

That sounds good to me. On top of that we can release note this and maybe even have a blog post or tweet.

[octref]

InitializeParams of language server code uses rootPath. params.rootPath code still exist in html/css server but I can remove them (#79532).

+1 for undefined rootPath in MR.

Actually vscode-languageclient doesn't depend on rootPath any more from v4.0.1: https://github.com/microsoft/vscode-languageserver-node/commit/2452184939725f7cd14d0f32457164c7c30b34b0#diff-79d1f5abc1a9f55a0e9eae588a7fdd56R2645-R2656. So we are safe to change this for any language server that uses client > v4.0.1. @roblourens would be useful if you can check top 500's package.json as well, to see what version of vscode-languageclient everyone depends upon.

[roblourens]

Added a deprecation warning for rootPath and will file a lot of issues on extensions later.

[bpasero]

@roblourens I commented on that commit and now I wonder: should we also print a link to some wiki page that explains our reasoning for deprecation? We have https://github.com/microsoft/vscode/wiki/Adopting-Multi-Root-Workspace-APIs#eliminating-rootpath but maybe we should make a new wiki page that links to that page but also explains that we plan to permanently change rootPath to be undefined in workspaces soon?

[jrieken]

This needs more tweaking and cannot ship in its current form

[dbaeumer]

Such a change would need some major code changes in the LSP client library to keep it backwards compatible. Also the library doesn't reference rootPath it emulates it using the code here: https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/client.ts#L2930 . This is to avoid unnecessary deprecation warnings for extensions were the server does support rootPath.

If a server doesn't signal workspace folder support (see https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.workspaceFolders.ts#L42) we would need to restart the server when only the first folder changes to keep the current semantic.

[bpasero]

@dbaeumer so to clarify, an extension that is using LSP today will always get the first folder as root folder?

[dbaeumer]

Yes, we sent both the rootPath and the workspace folders since on initialize we don't know what the server supports. When the server comes back an signals workspace folder support we setup things so that changes to the list of folders can be signaled to the server. If not we don't. This is why we would need to restart the server if the rootPath changes.

[bpasero]

This is why we would need to restart the server if the rootPath changes.

Not clear to me. Why can you not change everything to work with workspace.workspaceFolders. Why do you rely at all on the behaviour of workspace.rootPath?

[dbaeumer]

@bpasero when rootPath conceptually changes. It doesn't matter if we detect this using workspace folders or root path.

[razzeee]

Does this have an impact on rootUri too? https://github.com/microsoft/vscode-extension-samples/issues/207

[bpasero]

⬆️ @dbaeumer

[dbaeumer]

Yes, rootUri is the rootPath in LSP.

[akaroml]

Here's another side effect related to this scenario. When the extension host restarts, the deactivate() function is not called. So extensions don't have the chance to properly clean up.

I was debugging my extension and set a breakpoint in deactivate(), it was never hit when the extension host restarts (by adding a folder to the workspace). I expect the full lifecycle of an extension is kept in this scenario. Do we plan to also fix this?

[roblourens]

@akaroml I can reproduce that in this scenario. Since we won't be restarting the EH at all anymore, that should be fixed. Looking at the code, we also restart the EH in the same way when we upgrade from a single folder to a workspace. But in that scenario, I can't repro this, it looks like my extension is properly deactivated.

If you can repro this in a scenario besides reordering the folders in an existing workspace, could you open a new issue?

[ivankravets]

deactivate() is broken in the latest VSCode releases, or, at a minimum, support for it was removed. We also have the same problem, we can't shutdown PlatformIO Home Server when the user closes VSCode.

[roblourens]

@ivankravets please file a new issue if there isn't one already, thanks.

I have merged the change to remove the ext host restarter. rootPath will be defined for a window with a folder or a workspace with a single folder, and undefined in any other case.

[dbaeumer]

@roblourens as I was pointing out here https://github.com/microsoft/vscode/issues/69335#issuecomment-524202814 this needs some major changes in LSP.

IMO this needs some coordination and can't simply be pushed without having at least a story implemented in LSP. In addition all extension need to catchup with the new version of the LSP libraries then.

So can you please revert the changes until we have a plan how to coordinate this.

[roblourens]

@dbaeumer I guess I still don't understand the problem exactly, if the LSP library is not using the extension API's rootPath directly. Is it just that you need the behavior of your rootPath to match vscode's?

I don't have a good idea of how many language servers extensions are referencing your rootPath, do you know of any more popular ones which still rely on it?

[roblourens]

Talked to @octref about this and it sounds like the LSP world is different because language servers can just say that they don't support workspace folders, and then they rely on the InitializeParams' rootPath being valid throughout their lifetime. However, that rootPath is still marked as deprecated in the LSP, so shouldn't implementers still know not to depend on it?

Also, I don't really understand code like this, which looks like it's for backcompat, but shouldn't be necessary in a builtin extension:

https://github.com/microsoft/vscode/blob/cabcace4b5c5816c4715d6dca26edb0e6e826f8b/extensions/html-language-features/server/src/htmlServerMain.ts#L88-L93 @aeschli

[dbaeumer]

Actually LSP models the same behavior as the VS Code API does. The rootPath is set to the first workspace folder and since the extension host reloaded on reordering of the workspace folders this was always true for the language servers.

The rootPath in LSP is deprecated but this doesn't force anyone to move off of it as it didn't force anyone to move off of it in the VS Code API.

The change we want to do this is basically a semantic breakage and according to semver this would require to introduce a new major version meaning we have to move the LSP protocol to 4.x. This is something I tried to avoid so far since it will disturb the community and we should only do this if it is absolutely necessary. All LSP implementation for all the different programming languages out there have to move. Not only our TS/JS implementation.

Regarding

I guess I still don't understand the problem exactly, if the LSP library is not using the extension API's rootPath directly. Is it just that you need the behavior of your rootPath to match vscode's?

The LSP library emulates the rootPath based on the workspace folder property. The reason is that we print a warning in the console if extensions access the property. In case of LSP this is a library and a user of the library couldn't influence the access also its language server might not have used the property (for example TSLint and ESLint extensions don't use rootPath anymore but their extension would still print a warning if the LSP library would access rootPath).

Understanding what language servers do is a lot harder since they can be implemented in any programming language. The only thing I could think of is printing a warning if a server doesn't report to be multi workspace folder aware. But if it is not we still don't know if the server access the rootPath property or not.

So to not break LSP servers the LSP client library would need to do the following: if the workspace folder order changes and the server is not multi workspace folder aware and we don't have a server per workspace folder (not sure if I can detect this since this is something the extension manages today), shutdown the server and restart it (basically emulate with the server what we do now with the extension host). The complex thing is that the LSP client library needs to buffer certain VS Code events since they can't be deliver when the server is stopped and restarted.

But this still means that extension need to ship a new version since they need to pre-req a new LSP client library version to adopt to the change.

[roblourens]

Ok, now I understand the situation. Thanks for the explanation. It still seems to be like the new behavior wouldn't introduce any major new problem though.

For example, say you have workspace folders X and Y. The language server uses rootPath and only knows about X. rootPath is computed once when the language server starts. If you rearrange those folders during your session, the language server will still only give intellisense for X. If we restart the EH, it would give intellisense for Y instead. So the new behavior seems fine.

Or if you have X and Y and you delete X, and we don't restart the EH, then the language server will still only know about X and won't give intellisense for Y. But it wasn't giving intellisense for Y before you deleted X either, so nothing has changed. I can see why it would be preferable to restart the EH and get intellisense for Y instead (what would happen currently) but this seems like a very small case and it doesn't seem worth it to complicate our code to support. Does that make sense?

[roblourens]

Do language servers ever talk to each other? Maybe there would be an issue if you start language server A, it gets rootPath X, then rearrange the workspace folders, then language server B is activated and gets rootPath Y?

[octref]

@roblourens They do, see Intelliphense.

[dbaeumer]

For me the two problematic things are:

• change in user experience since in a non multi folder aware extension language smarts stick to the first folder when VS Code is open and not to the first folder used. Users might rely on that behavior.
• cases where extension start more than one language server. I need to cycle back with the Cpp team to understand again what their current solution is in terms of file not belonging to any workspace folders and to which server they will be associated.

I looked yesterday into what I can do in LSP and if we accept some diagnostic flickering and the fact that language smarts are not available for a certain period of time I think the easiest is to shutdown and restart the server without buffering any state. If extensions providers get reports against this we can then argue that they should become multi workspace folder aware.

[roblourens]

the first folder when VS Code is open and not to the first folder used

Sorry I don't understand why that would change, I don't think that will be an issue.

But if we will have to restart the language servers anywhere, it seems simplest to just leave the code alone and restart the EH.

[bpasero]

..and restart the EH.

I think there is still value in NOT restarting the EH. An extension today that shuffles workspace folders and wants to do more things after that shuffling cannot do anything about it because the EH restarts. At all times we need to make sure the EH does only restart if absolutely required (e.g. when going from no workspace => workspace because it changes state locations)

[dbaeumer]

Sorry I don't understand why that would change, I don't think that will be an issue.

To my understanding this works as following today with language smarts not being multi folder aware:

• user opens workspace with 2 folder. The 1 folder gets language smarts.
• user reorders the two folders:
  • today: the first one gets language smarts
  • in future: the second one. So language smarts stick to the folder being the first in a session.

This is a change in behavior which we might tolerate. If we don't want to tolerate this all we can do is to restart the language server.

[dbaeumer]

There is definitely value in not restarting the language server. @egamma and I looked into this and here is a query to show all language extensions that are declared to be multi root aware:

https://marketplace.visualstudio.com/search?term=tag%3Amulti-root%20ready&target=VSCode&category=Programming%20Languages&sortBy=Relevance

@roblourens you said you did a analysis which might be affected by this. So can you compare the market place query against your analysis / top n extensions. If none of the languages are affect I think we should ignore the others. If we have a strong push back then we see what we do (e.g. fix it in LSP)

[roblourens]

I didn't look at usage of the rootPath in the language server library. I guess I am not sure what you are asking for. Are you just asking me to look into how many language extensions would be affected by this?

Here's my list of extensions that appear to be using rootPath and aren't using workspaceFolders

011_octref.vetur/server/dist/services/vls.js:
057_tht13.python/out/server/src/server.js:
059_dbaeumer.jshint/jshint-server/out/server.js:
108_onecentlin.laravel-blade/server/out/htmlServerMain.js:
143_mkaufman.HTMLHint/server/server.js:
178_mrmlnc.vscode-scss/out/server.js:
179_castwide.solargraph/out/src/LanguageServer.js:
208_bungcip.better-toml/out/server/tomlServerMain.js:

Fortunately two of the top authors are already in this thread 😁 I can file issues on the others.

[dbaeumer]

The JSHint extension is now maintained by @RMacfarlane :-)

No the proposal was not to look into the code since this will be very cumbersome since it would need to read servers in many different programming languages. The idea was as follows:

• above query show all language extensions in the market place that declare to be multi root enabled.
• we could compare this list against the list of the top language extensions I was hoping you already compiled / have due to the investigations you did around rootPath usages

If there is one top extension that is not in that list from the market place we have a certain risk that something breaks or someone is unhappy. If there is none then I would simply do the change and take the risk and if something happens we fix it in the LSP client library or create a patch for the extension.

[roblourens]

I filed issues on the extensions I mentioned above, except for a couple which were false positives, and I also briefly looked through the top 50 or so most popular language extensions. Besides the ones in the list above, and some that I filed issues on earlier like Powershell, some that don't support multiroot are

• PHP Intellisense https://github.com/felixfbecker/php-language-server/issues/513
• https://marketplace.visualstudio.com/items?itemName=vscjava.vscode-maven
• https://marketplace.visualstudio.com/items?itemName=austin.code-gnu-global

[roblourens]

@dbaeumer unfortunately I haven't seen much activity on the issues I filed. I'm not sure exactly what we need to move forward

[dbaeumer]

@roblourens I think someone has to make a decision whether we want to:

• take the risk of breaking workflows for extensions that are not multi root aware
• spend the development effort to workaround this in the LSP client. It would still mean that these extensions need to ship a new version bundling the new LSP client.

@egamma opinions? We can discuss this this week.

[ivankravets]

Sorry, for the interrupt, a few cents from @PlatformIO => https://marketplace.visualstudio.com/items?itemName=platformio.platformio-ide

1) 2019 was the worse year for VSCode. I'm personally fully disappointed. We spent a ton of effort just to working on workarounds in our extension. Almost every new release brought new problems for us. The 2017-2018 - were super progressive years. New APIs, quick bug fixing, amazing communication with extension providers.

2) Do you monitor social channels? Do you see what developers now think about VSCode? Do you remember a time when each of us waits for a new VSCode release to see new features and improvements? What do we have today? I pray each day before VSCode's new release in case it will no bring more problems for us.

3. Who do need new icons? "cursor", "highlights"? https://code.visualstudio.com/updates/v1_41 ? What is it? A ton of blocker issues and we work on new hotkeys? Did Microsoft change PMs for VSCode? It looks like YES!

4. How many years do we need to instruct VSCode understanding Copy/Paste for macOS? https://github.com/platformio/platformio-vscode-ide/issues/606 . Just take a look at @mjbvz twitter account. Every week a new tweet about useless hotkeys... People have been asking for 1 year!!! please fix. No!

5. Next surprise with the latest update. Instead of thinking about the developer's workflow, VSCode thinks about machine performance. It asks developers to fully disable a task provider. How??? This is THE KEY component which drives the whole of our extension. See tons of public complaints https://community.platformio.org/t/pio-vsc-cant-build-no-tasks-found/10488 . You can't imagine how many similar letters we receive in private support.

Yes, our task provider depends on reading platformio.ini file from the user machine and provides dynamic tasks. This is a super-fast operation and is fully asynchronous https://github.com/platformio/platformio-vscode-ide/blob/develop/src/tasks.js#L41

---

a) I know, this is an open-source and everyone can contribute. But! This is not a typical open source project. This is a strategic project by @Microsoft. The "open source" just only a screen before the further strategic business model.

b) Our extension is the most rated extension in the WHOLE marketplace. You can not imagine what is the price of these efforts. Do you know what we receive for promoting VSCode to a hundred thousand developers? NOTHING! We even don't have minimal support where someone could help with fixing critical and blocker issues. I've never seen so high pride. I agree we are NOTHING in comparison with Microsoft. But you don't understand one thing... If you want to create the best editor for everyone, you MUST TALK with Developers and Community.

c) We've already looking for a replacement for VSCode. The last year showed that VSCode is fully focused on JS/HTML/Icons/Hotkey coders and no one wants to discuss other perspectives of VSCode.

P.S: In any case, thanks for your efforts, especially for the 2016-2018 years!

P.S.S: Upcoming 1.42... 2 months since the last release! The MOST BLOCKER ISSUES were fixed:

• "See pending renames in a diff... "
• "Set the maximum number of editors open at one time"
• " Move the panel (Output, Terminal) to the left, r"
• " Quickly find folded regions "
• .... etc. etc.

I don't have more comments.

[alexr00]

Regarding (5) above, I know it's another work-around and not ideal, but until the tasks disablement is improved you could have your extension set the setting "task.slowProviderWarning" to false (to prevent people from getting into this situation) and "task.autoDetect" to true (to ensure that your task provider runs).

I added the warning notification for slow task providers since it can result in a long wait that is directly shown to users when they run Tasks: Run Task. It seems this is a dangerous setting and shouldn't show in a notification :). I do want to improve this (better way of showing progress? individually cancelable task providers? stream tasks into the quick pick?) and I hope to have something better soon!

[ivankravets]

@alexr00 thanks for the comment.

I added the warning notification for slow task providers since it can result in a long wait

Task providers are dynamic components. So, as a minimum, VSCode knows which extension/component provides tasks. This warning would be more clear if you say that "these extensions depend on Tasks and they will not work". I'm sure people will not disable task autodetection.

This is the same when we see a popup with "Do you accept our license?". Only 1% of people read the full license text. There even do not suspect that they agree to kill "all elephants".