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

Improve handling of extensions when workspaceContains times out #73656

Closed
DanTup opened this issue May 13, 2019 · 13 comments
Closed

Improve handling of extensions when workspaceContains times out #73656

DanTup opened this issue May 13, 2019 · 13 comments
Assignees
Labels
extensions Issues concerning extensions feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@DanTup
Copy link
Contributor

DanTup commented May 13, 2019

I had an issue raised by a user that say my extension activate spawn its language server while they were working on an unrelated (non-Dart) project (Dart-Code/Dart-Code#1699).

It turns out that there is a hard-coded timeout of 7 seconds for workspaceContains activation events and this user hit that (possibly through a combination or large folder, and machine being busy, other extensions loading, running emulators etc.):

private static readonly WORKSPACE_CONTAINS_TIMEOUT = 7000;

The behaviour when this timeout is hit is to pre-emptively activate the extensions in this case. I understand why this is done but it seems like it may trade one problem (searching the tree) for another (activating a bunch of unrelated extensions that may spawn language servers).

I think there should be more control (for users or extensions) over this behaviour, for example:

  • Being able to configure the timeout
  • Being able to change the behaviour of whether to load extensions when this happens (or be prompted and warned)
  • Showing the activation reason in the "Show Running Extensions" section
  • Allowing extensions to control whether to activate when workspaceContains times out (in my case, it's fine not to - workspaceContains is for convenience so the user doesn't need to open a .dart file before we activate, but if no Dart or pubspec.yaml was found in 7 seconds, it would be preferable to wait for the user to open one (or one be discovered in the tree) before activating)
  • Telling the extension the activation reason (Provide activation reason to extensions in activate call #44711)

Of course, the extension could scan the tree and not spawn the language server, but then it's just doing the work that VS Code specifically was trying to avoid, and if multiple extensions are all doing it, it'll be worse performing than if VS Code did it!

@DanTup
Copy link
Contributor Author

DanTup commented Oct 21, 2020

@alexdima this issue has been "on deck" for over a year but there's been no updates - might it be something that is tackled? Right now it's not clear how extensions are supposed to handle this. Users complain of language servers running when they shouldn't (although recent instances are because of #108578).

I would much rather not have my extension activate if this times out, because it would be picked up when the user opens a Dart file (since we have onLanguage too) anyway. If their workspace is so large, delaying activating until a file is opened is far better than us spawning language servers for completely unrelated projects (which then results in a lot of CPU/disk activity as it walks the entire tree that VS Code bailed out of because it's large).

Without a fix, it seems like extensions will be motivated to do their own activation - make the real activation do essentially nothing, and then scan the tree/watch open files/etc. and re-implement the activation triggers, but that really doesn't seem like a good path.

@Melvin-Abraham
Copy link

Melvin-Abraham commented May 8, 2021

Also, if the Extension Activation Events (or global settings) include an option like skipFolders that would skip checking some folders or files based on the glob pattern specified, the scan could be limited and be made performant as for the most part unnecessary extensions can activate for JS/TS workspaces containing node_modules.

@Livven
Copy link

Livven commented Jun 2, 2021

I run into this issue almost every time I start VS Code. I suspect this is mostly due to large node_modules directories. Here is a sample of the extension host log with the offending workspaceContainsTimeout entries:

[2021-06-02 11:29:35.816] [exthost] [info] ExtensionService#_doActivateExtension Dart-Code.dart-code {"startup":true,"extensionId":{"value":"Dart-Code.dart-code","_lower":"dart-code.dart-code"},"activationEvent":"workspaceContainsTimeout:*/pubspec.yaml,*/*/pubspec.yaml,*.dart,*/*.dart,*/*/*.dart"}
[2021-06-02 11:29:35.816] [exthost] [info] ExtensionService#loadCommonJSModule file:///c:/Users/z0041d8h/.vscode/extensions/dart-code.dart-code-3.23.0/out/dist/extension
[2021-06-02 11:29:36.317] [exthost] [info] ExtensionService#_doActivateExtension golang.go {"startup":true,"extensionId":{"value":"golang.go","_lower":"golang.go"},"activationEvent":"workspaceContainsTimeout:**/*.go"}
[2021-06-02 11:29:36.317] [exthost] [info] ExtensionService#loadCommonJSModule file:///c:/Users/z0041d8h/.vscode/extensions/golang.go-0.25.1/dist/goMain.js
[2021-06-02 11:29:37.178] [exthost] [info] ExtensionService#_doActivateExtension GraphQL.vscode-graphql {"startup":true,"extensionId":{"value":"GraphQL.vscode-graphql","_lower":"graphql.vscode-graphql"},"activationEvent":"workspaceContainsTimeout:**/.graphqlrc,**/.graphqlrc.{json,yaml,yml,js},**/graphql.config.{json,yaml,yml,js}"}
[2021-06-02 11:29:37.178] [exthost] [info] ExtensionService#loadCommonJSModule file:///c:/Users/z0041d8h/.vscode/extensions/graphql.vscode-graphql-0.3.16/out/extension
[2021-06-02 11:29:46.814] [exthost] [info] ExtensionService#_doActivateExtension hashicorp.terraform {"startup":true,"extensionId":{"value":"hashicorp.terraform","_lower":"hashicorp.terraform"},"activationEvent":"workspaceContainsTimeout:**/*.tf"}
[2021-06-02 11:29:46.814] [exthost] [info] ExtensionService#loadCommonJSModule file:///c:/Users/z0041d8h/.vscode/extensions/hashicorp.terraform-2.11.0/out/extension
[2021-06-02 11:29:50.463] [exthost] [info] ExtensionService#_doActivateExtension matklad.rust-analyzer {"startup":true,"extensionId":{"value":"matklad.rust-analyzer","_lower":"matklad.rust-analyzer"},"activationEvent":"workspaceContainsTimeout:**/Cargo.toml"}
[2021-06-02 11:29:50.463] [exthost] [info] ExtensionService#loadCommonJSModule file:///c:/Users/z0041d8h/.vscode/extensions/matklad.rust-analyzer-0.2.621/out/src/main
[2021-06-02 11:29:50.569] [exthost] [info] ExtensionService#_doActivateExtension ms-dotnettools.csharp {"startup":true,"extensionId":{"value":"ms-dotnettools.csharp","_lower":"ms-dotnettools.csharp"},"activationEvent":"workspaceContainsTimeout:*.csproj,*.sln,*.slnf,*.csx,*.cake,**/*.csproj,**/*.sln,**/*.slnf,**/*.csx,**/*.cake"}

Some ideas for how to fix this:

  • make the timeout configurable
  • exclude e.g. node_modules and .git directories by default

Obviously both solutions are not ideal, but they would still be an improvement over the current situation for me and I believe many others.

@alexdima alexdima removed their assignment Jun 2, 2021
@alexdima alexdima modified the milestones: On Deck, Backlog Jun 2, 2021
@DanTup
Copy link
Contributor Author

DanTup commented Jun 2, 2021

I updated the Dart extension in the last release to try and solve this - it no longer does a full workspace search but only checks the top few folders:

[2021-06-02 11:29:35.816] [exthost] [info] ExtensionService#_doActivateExtension Dart-Code.dart-code {"startup":true,"extensionId":{"value":"Dart-Code.dart-code","_lower":"dart-code.dart-code"},"activationEvent":"workspaceContainsTimeout:/pubspec.yaml,//pubspec.yaml,.dart,/.dart,//*.dart"}

This works if you only have the Dart extension, but it looks like having any other extension that does a full workspace search might still be able to trigger the timeout and then cause the Dart extension to load.

I think a simple flag on an extension that says whether or not it should be activated in the case of a timeout would work fine for most. I'd prefer Dart not to activate in this case, as it'll then activate as soon as a user opens a .dart file or similar anyway. The workspaceContains is only intended to get a head-start on starting the language server when a project is opened, I'm not really sure it's worth this drawback right now.

@alexdima
Copy link
Member

With 9fd6ee7 , I have pushed a change to no longer activate extensions if workspaceContains times out. Let's see what kind of feedback we get from Insiders.

@alexdima alexdima self-assigned this Nov 19, 2021
@alexdima alexdima modified the milestones: Backlog, November 2021 Nov 19, 2021
@DanTup
Copy link
Contributor Author

DanTup commented Nov 19, 2021

@alexdima are there any guidelines around activation and how language extensions should behave? In some issues (like this one) it seems like VS Code is trying not to activate extensions when they're not needed, which makes it seem reasonable for language servers to unconditionally start language servers at activation (to avoid repeating expensive work by implementing their own activation events), but in other issues like #70303 the suggestion is that extensions may be activated by unrelated events (eg. running any task will activate any extension that has tasks).

I've asked for some guidance many times across many issues, but there has been none. I want to do the best thing for VS Code and my users, but I genuinely don't know what the recommendations are. If I should have a low-resource mode my extension can run in and then handle its own "real" activation programatically that starts the language server, I'm happy to do that - but I don't want to put the effort into that if VS Code is trying not to put extensions in that situation.

@alexdima
Copy link
Member

I'm sorry for the lack of guidance in this area. We would love to get extensions activated precisely when they are needed, and not sooner. But once they are needed, we would love that they would activate quickly :)

Sometimes activation events have ugly corner cases like this issue. It is hard to appreciate without context what the best way forward is: what if the user is a beginner and has just opened their first project in VS Code on a really slow computer? Some of the language extensions include all kinds of getting started experiences, and not activating them might mean we place more burden on the user to get going without any help from a language extension. That was my initial thinking in this area. But there has been a recent wave of feedback from extension authors and team members that reached out to me that extensions have started activating too eagerly, maybe after a bug we fixed recently.

There is also a momentum in the team to get extensions to move off * activation and to enhance activation events to allow extensions to avoid listing all their commands one-by-one to make moving off * more appealing, e.g. #134373.

For a language server, I would personally implement it in a way where the language server process is launched when the first request comes in (i.e. register all the providers and only when they first get invoked, only then launch the server).

Again, apologies for the frustration that accumulates in this area and thank you for your feedback!

@DanTup
Copy link
Contributor Author

DanTup commented Nov 19, 2021

For a language server, I would personally implement it in a way where the language server process is launched when the first request comes in (i.e. register all the providers and only when they first get invoked, only then launch the server).

I'm not sure this is really possible. Not only are the providers created by the VS Code LSP client when using LSP, but the server initialisation is what defines which document selectors are used (it's controlled by the server), so the providers can't be registered correctly until the server is initialised.

There are certainly things we could do to delay a lot of setup, and I'm certainly happy to do that if it makes sense. But without more guidance (and while issues like this one are being addressed) it feels like we might end up building a whole parallel activation structure within our extension and if VS Code continues to improve its activation it may not be worthwhile.

That's why I'd really like to see some general guidance. If the suggestion is that language extensions don't assume when they're activated they may be required, then I'm happy to review my activation. But otherwise, if VS Code wants to avoid activating unless required, then I'd prefer not to do that work, but I think issues like #70303 should be handled better (and I think the fix for that is #126238, but that was closed and I had similar comments).

(If there's value in creating an issue speficially about having better guidance in this area, lmk and I'll move these comments to one :-))

@alexdima
Copy link
Member

cc @dbaeumer

@dbaeumer
Copy link
Member

I have a comparable problem in ESLint and it usually happens when beside onLanguage there are other kind of activation events.

In ESLint I listen to document open and postpone activating the ESLint server until a document type opens I am interested in (https://github.com/microsoft/vscode-eslint/blob/f308127cf98fc7a75528c23cbd9d8a98dbd93286/client/src/extension.ts#L419). This basically allows to be activated differently for commands or languages

@DanTup
Copy link
Contributor Author

DanTup commented Nov 22, 2021

That's what I was considering doing, although waiting for a Dart file to be opened seems too late - today a Dart user can open their project and just press F5 because we detect the pubspec.yaml in the project and we can infer a launch file without it needing to be open. So to do this, we'd end up doing more than listening for file opens, but also scanning the workspace tree.

I'm not against doing it and handling the "real" activation myself, but I'd like to know this is the recommended way before I do, since VS Code's attempts to lazily activate extensions make it seem like this shouldn't be necessary. I've made changes before based on comments in issues before which turned out not to be optimal, so some official guidance in this area to ensure everyone is on the same page would really be appreciated.

@dbaeumer
Copy link
Member

VS Code tries to activate extensions lazily. But quite some extensions have more than one activation event and hence VS Code itself can't decide which part to activate.

If an extension has n activation events then the recommendation is that the extension deals with making this as lazy as possible (e.g. only doing the work that is actually necessary to fulfil the activation)

@alexdima any additional insights.

@DanTup
Copy link
Contributor Author

DanTup commented Nov 24, 2021

If an extension has n activation events then the recommendation is that the extension deals with making this as lazy as possible

Can this recommendation be documented somewhere? It hasn't been clear to me that this was the recommendation since VS Code seems to have a lot of control over activation events that make it seem like the extension doesn't need to do this.

There's a lot of functionality in my extension I don't really want to leave until the user opens a Dart file (eg. I want a user to be able to open a project and just press F5 without any file open - and that works today), so I would end up scanning the workspace folder. However, it seems like this is duplicating a lot of work VS Code has already done as part of workspaceContains and if every extension does that, that's a lot of repeated work (and completely wasteful in the case where it didn't time out and VS Code knows there are Dart files - but doesn't tell us, because we get no activation reason).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions Issues concerning extensions feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@DanTup @Livven @jrieken @dbaeumer @alexdima @Melvin-Abraham and others