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

Support configuration contribution points of resolver extensions #224236

Closed
connor4312 opened this issue Jul 29, 2024 · 15 comments · Fixed by #233721
Closed

Support configuration contribution points of resolver extensions #224236

connor4312 opened this issue Jul 29, 2024 · 15 comments · Fixed by #233721
Assignees
Labels
author-verification-requested Issues potentially verifiable by issue author config VS Code configuration, set up issues extension-host Extension host issues 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

@connor4312
Copy link
Member

connor4312 commented Jul 29, 2024

  1. Register some configuration defaults like { hello: "world" } using the web API (e.g. here for the code-server.sh script)
  2. 🐛 notice they are still undefined when read from the local extension host (at least before a remote is resolved)

It seems like it should work because the overrides are registered here

if (environmentService.options?.configurationDefaults) {
this.configurationRegistry.registerDefaultConfigurations([{ overrides: environmentService.options.configurationDefaults }]);
}

before the MainThreadConfiguration is even instantiated, so I would expect they get passed during initial hydration here

proxy.$initializeConfiguration(this._getConfigurationData());

...but they do not. I did some debugging myself but got a bit lost in config land.

Having these options is needed for a couple partner team feature asks.

@sandy081 sandy081 added bug Issue identified by VS Code Team member as probable bug config VS Code configuration, set up issues labels Aug 20, 2024
@sandy081 sandy081 added this to the August 2024 milestone Aug 20, 2024
@sandy081 sandy081 removed the bug Issue identified by VS Code Team member as probable bug label Aug 20, 2024
@sandy081
Copy link
Member

Assigning to August for investigation.

@sandy081
Copy link
Member

I changed the default of workbench.colorTheme setting and it works (github.dev also uses the same). You should have the setting registered for the default to be applied. I suspect the setting is not registered in your scenario?

@sandy081 sandy081 removed this from the August 2024 milestone Aug 26, 2024
@sandy081 sandy081 added the info-needed Issue requires more information from poster label Aug 26, 2024
@connor4312
Copy link
Member Author

connor4312 commented Aug 30, 2024

The setting is registered. Repro is a little annoying since it requires an extension doing the resolution of the remote.

  1. Run vscode.dev locally so that you can debug things (npm install && npm run build && node out/server/main.js)
  2. Clone https://github.com/microsoft/vscode-remote-tunnels. git checkout connor4312/preference-auth-provider && npm install && npm run watch in one terminal, and cd extension && npx serve -p 3001 --cors in another
  3. With devtools open, go to http://localhost:3000/+aHR0cDovL2xvY2FsaG9zdDozMDAx/some-tunnel-here (first segment is base64-encoded localhost:3001; the tunnel does not actually need to be hosted.) You can add ?vscode-quality=dev to that URL to use your local vscode build found in a sibling folder to vscode-dev.
  4. Notice that even though we set a default value for remote.tunnels.useAuthProvider, it's undefined when you hit the debugger statement.

@connor4312 connor4312 removed the info-needed Issue requires more information from poster label Aug 30, 2024
@connor4312 connor4312 added this to the September 2024 milestone Aug 30, 2024
@sandy081
Copy link
Member

sandy081 commented Sep 3, 2024

Thanks for the steps. I can confirm that the setting is not yet registered at the time when you are accessing this setting in your extension. Your extension (remote resolver) is accessing this setting while resolving the authority and I suspect, contribution points of this extension are not read/handled yet.

@alexdima Can you please confirm if this is true.

@sandy081 sandy081 assigned alexdima and unassigned rzhao271 Sep 3, 2024
@sandy081 sandy081 added the extension-host Extension host issues label Sep 3, 2024
@alexdima
Copy link
Member

alexdima commented Sep 6, 2024

Yes, this is an old issue since we first did remotes #88044 . Extension points are not handled until after the resolver resolves. In code:

  • AbstractExtensionService._initialize first waits for _resolveExtensions
  • NativeExtensionService._resolveExtensions does the remote authority resolving
  • Only then is _processExtensions invoked
  • That one eventually calls _doHandleExtensionPoints which goes through our extension points to inform them of extension contributions

There are some historical reasons behind it, I think back then we did not support disabling extensions without a window reload, so the extension points themselves were not ready for an extension point removal. It also did not seem to be so bad of an issue because the only affected extensions would be our resolver extensions and they mostly suffered when reading their own default configs, which they should normally know since they contribute their extensions. But it looks like you have a new pattern where an embedder is driving the resolver's default config value, which makes the initial issue more severe.

I don't know if a workaround can be made in the configuration service to be able to give values for settings that don't have their schema registered @sandy081 or if we now must handle at least the resolver's extension points before resolving. If it's the latter, then @connor4312 maybe you can look into a PR. I think something must be added before invoking the resolver here which would potentially call _doHandleExtensionPoints. But be aware that it would not be good to handle the extension points of all locally available extensions since many of them will be available remotely and that would just cause a lot of churn. Now that I write this, I remember the second reason why I didn't tackle this. Some extension point handlers use the fact that they are being called as a way to think of themselves as being "ready"/"finished", which in this situation is clearly not the case. I think the debugger was one of them, but maybe others, I don't remember.

@sandy081
Copy link
Member

sandy081 commented Sep 6, 2024

if a workaround can be made in the configuration service to be able to give values for settings that don't have their schema registered

I feel this allows misusing of the API for any random property.

How about handling only configuration contribution of a resolver extension? Or as a generic approach, let the contribution point define if it is ready for handling a contribution point during resolving phase?

@sandy081 sandy081 modified the milestones: September 2024, October 2024 Sep 23, 2024
@sandy081 sandy081 removed this from the October 2024 milestone Oct 21, 2024
@connor4312
Copy link
Member Author

How about handling only configuration contribution of a resolver extension? Or as a generic approach, let the contribution point define if it is ready for handling a contribution point during resolving phase?

Sure, I think either of these would work.

@connor4312
Copy link
Member Author

This remains an important scenario for our friends at Azure Functions, is there any way we can figure a solution here?

@joaomoreno joaomoreno added the feature-request Request for new features or functionality label Nov 11, 2024
@joaomoreno joaomoreno added this to the November 2024 milestone Nov 11, 2024
@joaomoreno joaomoreno changed the title Configuration defaults passed via workbench options don't work Support configuration contribution points of resolver extensions Nov 11, 2024
@sandy081
Copy link
Member

I will look into it this milestone/

@sandy081
Copy link
Member

@connor4312 This is fixed now. Please try it out in tomorrow's insiders and let me know if it works.

@connor4312
Copy link
Member Author

Thanks so much!

@vs-code-engineering vs-code-engineering bot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Nov 15, 2024
@sandy081 sandy081 added the author-verification-requested Issues potentially verifiable by issue author label Dec 2, 2024
Copy link

This bug has been fixed in the latest release of VS Code Insiders!

@connor4312, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version 2ed1e9b of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!

@sandy081 sandy081 added the verification-needed Verification of issue is requested label Dec 3, 2024
@alexr00 alexr00 added the ~verification-steps-needed Steps to verify are needed (with bot comment) label Dec 4, 2024
Copy link

Friendly ping! Looks like this issue requires some further steps to be verified. Please provide us with the steps necessary to verify this issue.

@vs-code-engineering vs-code-engineering bot added verification-steps-needed Steps to verify are needed for verification and removed ~verification-steps-needed Steps to verify are needed (with bot comment) labels Dec 4, 2024
@sandy081
Copy link
Member

sandy081 commented Dec 4, 2024

I would request @connor4312 to verify this as he has the set up and can also check if this is working for his scenario.

@sandy081 sandy081 removed the verification-steps-needed Steps to verify are needed for verification label Dec 4, 2024
@connor4312 connor4312 added the verified Verification succeeded label Dec 4, 2024
@connor4312
Copy link
Member Author

Yep it works thanks!

@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author-verification-requested Issues potentially verifiable by issue author config VS Code configuration, set up issues extension-host Extension host issues 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

Successfully merging a pull request may close this issue.

6 participants