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

Do not activate extension with workspaceContains always after installing #75284

Closed
DanTup opened this issue Jun 11, 2019 · 12 comments
Closed

Do not activate extension with workspaceContains always after installing #75284

DanTup opened this issue Jun 11, 2019 · 12 comments
Assignees
Labels
extension-host Extension host issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone

Comments

@DanTup
Copy link
Contributor

DanTup commented Jun 11, 2019

I had a report of strange behaviour when installed the Flutter extension - it prompts you to install a Dart SDK (when you probably want to use Flutter, not Dart). It seems like installing the Flutter extension is activating the Dart extension (it does depend on it) even though no activation requirements for either extension are met, and the reason in the log is incorrect ("workspaceContains:**/pubspec.yaml").

Here's what I did:

  • Open VS Code - ensure there is no open workspace/folder
  • Install the Flutter extension (which will install the Dart extension)
  • Note the Dart extension is activated, and the extension host log lists:
[2019-06-11 17:54:40.902] [exthost] [info] eager extensions activated
[2019-06-11 17:54:53.239] [exthost] [info] ExtensionService#_doActivateExtension Dart-Code.dart-code {"startup":false,"activationEvent":"workspaceContains:**/pubspec.yaml"}
[2019-06-11 17:54:53.240] [exthost] [info] ExtensionService#loadCommonJSModule /Users/dantup/.vscode/extensions/dart-code.dart-code-3.1.0/out/src/extension

Here's a screenshot showing I don't have a folder open with a pubspec.yaml in it:

Screenshot 2019-06-11 at 5 57 54 pm

Is this expected? Can I stop it?

@vscodebot vscodebot bot added the extensions Issues concerning extensions label Jun 11, 2019
@DanTup DanTup changed the title Extension is unexpectedly activated with an incorrect acitvationReason in the log Extension is unexpectedly activated with an incorrect activationReason in the log when a dependent extension is installed Jun 11, 2019
@DanTup DanTup changed the title Extension is unexpectedly activated with an incorrect activationReason in the log when a dependent extension is installed Extension is unexpectedly activated with an incorrect activationReason after install Jun 11, 2019
@DanTup
Copy link
Contributor Author

DanTup commented Jun 11, 2019

Actually, just installed Dart directly shows the same thing. What's strange is that installing the Flutter one does not activate the Flutter extension, so it's not like it's just activating whatever I install immediately 🤔

@DanTup
Copy link
Contributor Author

DanTup commented Jun 11, 2019

Hmmm, I think it's because of this:

if (/^workspaceContains/.test(activationEvent)) {
// do not trigger a search, just activate in this case...

😕

@DanTup DanTup changed the title Extension is unexpectedly activated with an incorrect activationReason after install Extension with workspaceContains is always activated after install, even when workspace does not contain the file Jun 11, 2019
@DanTup
Copy link
Contributor Author

DanTup commented Jun 13, 2019

FWIW the code above was added in 7304f38 (it's in a different file, as it was moved around a bit). I can't see a PR so not sure whether this was to fix an issue or something (cc @alexandrudima), but it's a bit undesirable for extensions that want to prompt the user to install an SDK based on the type of the open project. Currently we have no way to tell if VS Code activated this extension pre-emptively, or if the activation events were actually matched (so we can't even skip the prompt without re-checking for all the conditions ourselves).

@sandy081
Copy link
Member

It was intentional to always activate any extension that has workspace contains activation event after installing due to the performance reasons.

I would wait for @alexandrudima 's opinion on this.

@sandy081 sandy081 assigned alexdima and sandy081 and unassigned sandy081 Jun 25, 2019
@sandy081 sandy081 added extension-host Extension host issues feature-request Request for new features or functionality and removed extensions Issues concerning extensions labels Jun 25, 2019
@sandy081 sandy081 added this to the Backlog milestone Jun 25, 2019
@sandy081 sandy081 changed the title Extension with workspaceContains is always activated after install, even when workspace does not contain the file Do not activate extension with workspaceContains always after installing Jun 25, 2019
@DanTup
Copy link
Contributor Author

DanTup commented Jun 25, 2019

I suspected it was performance related, but I think it's a false economy because:

  1. You're now causing extension code to run that might not be needed, which can eat resources
  2. The extension has to choose between:
    a) starting a language server that might not be needed (eats more resources)
    b) scanning the workspace itself to see if it's needed - thus undoing the savings VS Code made by not doing it

You're doing that work on a folder open anyway, I don't see why doing it at extension install is a huge deal (you probably already have some of the tree cached in the rendering for the explorer tree at this time too).

Also - if it's not considered a bug that extensions can activate when their conditions are not met, I think it should be called out explicitly in the docs (so it's clear they should decide whether or not they really should start up language servers, show user prompts, etc.).

@alexdima
Copy link
Member

alexdima commented Aug 5, 2019

An extension can be activated outside the activationEvents. For example, another extension can be written which uses vscode.extensions API and activates any other extension programatically. Or another extension can be written which depends on the extension and its activation would lead to the activation of the original extension. So an extension should be prepared to be activated even outside the events listed in activationEvents...

That being said, the majority of extensions are not insanely expensive to activate... The entire idea of activation events is to reduce the impact extensions have on startup and to be economical, of course. But imagine the following scenario: the editorconfig extension activates on workspaceContains:**/.editorconfig and then you open a folder with hundreds of thousands of files... So many files that it would take entire minutes to go through them...

The editorconfig extension works by looking at the current opened text buffer and going up the folder hierarchy looking for .editorconfig... So once it is activated (which is cheap), its operation is also kind of cheap... For such an extension, it is much better to simply activate it if the search yields nothing after a certain amount of time. In such a case waiting for minutes to finish the search would be far more wasteful than simply giving up and activating the extension...

So IMHO it is not such a clear lack of judgement on our side to give up after some time and defer the decision of doing something to the extension. The extension has domain knowledge that we lack, an extension can implement something smarter than we can. The extension can also start searching and realising after a while that it is on a huge folder, but then it can present a popup which is specific to the extension, it can ask the user to define what exact behaviour they want...

For us, in the core, the decision is:

  • keep going with searching (which might be unbounded on some crazy network shares, might consume minutes of 100% i/o on some poor disk, etc.)
  • stop after a while and not activate the extension
  • (current behaviour) stop after a while and activate the extension

I still believe we are doing the best given the limited information we have... The only thing which is debatable is for how long should we search until we give up. I think we just chose a number which seems reasonable...

@DanTup
Copy link
Contributor Author

DanTup commented Aug 5, 2019

I still believe we are doing the best given the limited information we have...

Extensions have even more limited info though - we don't even get given the activation reason (I did ask). In fact, I recently opened a Dart project and the C# extension started downloading all sorts of .NET dependencies to my machine. The C# extension activates on tons of events (for ex. onDebugInitialConfigurations) and then just downloads all its dependencies regardless of what you have open. So the faster startup isn't really faster if extensions are doing lots of expensive work themselves.

The only thing which is debatable is for how long should we search until we give up. I think we just chose a number which seems reasonable...

There is no search. Check the code that I included above:

if (/^workspaceContains/.test(activationEvent)) {
// do not trigger a search, just activate in this case...

When you install an extension, it just pre-emptively activates it if it has an onWorkspaceContains :(

@sandy081 sandy081 removed their assignment Aug 12, 2019
@DanTup
Copy link
Contributor Author

DanTup commented Sep 4, 2019

I just saw this again. I opened VS Code and it auto-updated the C# extension (I had a Dart project open). Because of this behaviour, the C# extension was unnecessarily activated. It then started downloading dependencies:

C# extension downloading stuff

This logic doesn't seem to hold up. You're avoiding doing expensive things at startup and causing extensions to not only do expensive things, but illogical things. Fast forward a few years where I have 20 extensions that work like the C# one, and I've potentially got 20 extensions downloading SDKs when I opened VS Code with none of their projects open.

Is this really working as intended?

@alexdima alexdima modified the milestones: Backlog, September 2019 Sep 9, 2019
@alexdima alexdima modified the milestones: September 2019, October 2019 Sep 30, 2019
@alexdima alexdima modified the milestones: October 2019, November 2019 Oct 28, 2019
@alexdima alexdima removed this from the November 2019 milestone Dec 2, 2019
@alexdima
Copy link
Member

alexdima commented Jul 6, 2020

To verify: Have an extension that uses workspaceContains. Then disable it and reload. If you enable it, the extension will only be activated if there is a real match. Before, it used to always be activated.

gjsjohnmurray pushed a commit to gjsjohnmurray/vscode that referenced this issue Jul 8, 2020
gjsjohnmurray pushed a commit to gjsjohnmurray/vscode that referenced this issue Jul 9, 2020
@alexdima
Copy link
Member

alexdima commented Aug 4, 2020

@DanTup If you have a chance, could you please verify this?

@alexdima alexdima added the verification-needed Verification of issue is requested label Aug 4, 2020
@DanTup
Copy link
Contributor Author

DanTup commented Aug 4, 2020

@alexdima sorry for the delay! I tested this in latest insiders, and it works great! With a non-Dart project open, if I install Dart/Flutter extensions, there is no activation. If I repeat with a Dart/Flutter extension open, they activate.

Thanks!

@alexdima alexdima added verified Verification succeeded and removed verification-needed Verification of issue is requested labels Aug 5, 2020
@alexdima
Copy link
Member

alexdima commented Aug 5, 2020

Thank you! ❤️

@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extension-host Extension host issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants
@DanTup @alexdima @sandy081 and others