Extensions cannot get workspace configurations of their own

[spywhere]

• VSCode Version: 1.0.1-insider [85f337e]
• OS Version: OS X El Capitan 10.11.4 (15E65)

Extensions cannot get their configuration settings due to vscode.workspace.getConfiguration returns undefined.

This is the error trace from running the extension...

This is the variable dumps from debug mode of guides extension while request for less, explorer and guides configurations... (you can see that guides variable has no member while less and explorer have their members)

However, the default configurations contain those keys...

Related issue on guides repo: spywhere/vscode-guides#3

Known extensions affect by this issue (thanks to @andischerer):

• Guides
• CoffeeLint
• TSlint
• Path Intellisense

Steps to Reproduce:

1. Install one of the extension I listed (in this case I try guides)
2. Check the console...

[jrieken]

it seems like the main side doesn't send the full configuration to the extension host, esp. the default values which should have been derived from the extensions (package.json) are missing.

The snapshot shows what we send to the extension host. No mentioning of guides tho of tslint cos that is already configured for me.

[jrieken]

I believe this was introduced with 31ce12f. We now send the configuration over to the extension host before we have loaded the values/defaults from the extensions (their package.json). This happens when extensions are detected and when the configuration contributions are processed. The configuration service should either block until after that.

[bpasero]

⚡

[bpasero]

@alexandrudima @jrieken would appreciate a second look on my change (a6d7403).

Basically what we always did before was to reload the entire configuration from disk as soon as the onDidRegisterConfiguration event was fired that indicates a new configuration was provided. In this case, the extension host would trigger this event for each extension that provides configuration. Since the extension host needs some time to come up, this event fires sometime after the workbench opens.

This means, for each extension with configuration we would do a full reload of all configuration from disk even though that was not needed because the only thing that changes is the default values, not the actual values in the configuration files.

I did a bad change to buffer this event because I noticed it is fired once per extension. So I would wait for 50ms before reloading the configuration from disk on such an event and this causes a race condition, because the extension might come up before 50ms and expect the configuration settings to be there.

Now, with my new change, instead of reloading the configuration from disk each time, I just update my cached configuration with all the new default values I get from the extensions. In other words, the new cached configuration is the total set of defaults with the actual changed values mixed in.

[jrieken]

@bpasero I have a added a test which still fails. Are we sure that don't activate an extension before having send its default configuration to the extension host? This was the previous behaviour and folks have built on that.

[bpasero]

@jrieken this is a very cool test. I found an issue with the package.json and after fixing it (8e0ebc3), the test runs fine.

[shardulm94]

Did this go through in 1.1.0-insider? I am still having this issue with a few extensions.
Both of the below extensions rely on reading the configuration on the * activation event.

[spywhere]

My extension (Guides) is working properly on 1.1.0-insider (1865ad0) without any code changes

[bpasero]

@shardulm94 can you share your extensions where this still happens?

[shardulm94]

@bpasero

https://github.com/ChristianKohler/PathIntellisense/blob/master/src/PathCompletionItem.ts#L5

https://github.com/shardulm94/vscode-trailingspaces/blob/master/src/trailing-spaces/config.ts#L35
https://github.com/shardulm94/vscode-trailingspaces/blob/master/src/trailing-spaces/config.ts#L28

[bpasero]

@shardulm94 I cannot reproduce, are you sure you are on latest insider?

[shardulm94]

@bpasero Yes, I am on the latest one.
I did not encounter this when I started vscode now the first time. But after closing vscode and reopening I got it again.

[bpasero]

@alexandrudima @jrieken this is a race condition that only reproduces on Windows for me, likely because the timing for spawning the extension host is different. When I debug the onDidRegisterConfiguration() event (https://github.com/Microsoft/vscode/blob/ben/stacks/src/vs/platform/configuration/common/configurationService.ts#L82) I see that I am being called and that the default values from all extensions are being filled into the cached configuration.

Is there any chance that the onDidRegisterConfiguration() event does not have a chance to update the configuration value for the extensions because the extension host is already starting up and activating extensions at the same time?

In other words, the extension host should only start activating extensions if the configuration service had a chance to mixin the provided default configuration from all extensions and was able to notify all listeners about this change. Maybe the fact that the configuration changes is not being sent to the extension host fast enough before the extensions are starting up?

[bpasero]

@alexandrudima @jrieken I now instrumented the configurationService and the extHostConfiguration and built a Code version with it to understand the race condition a bit better. It turns out that in some cases an extension calls into getConfiguration before acceptConfiguration has been called on the extension host. There is a suspicious early return in getConfiguration() that causes the reported exceptions. Since undefined is returned in this case, any extension will fail to use the configuration at all.

When I look into the implementation in version 1.0.0, I would argue that we have been lucky that the exception did not occur, because _acceptConfigurationChanged is only being called after _configurationService.loadConfiguration(). This call can be long running if it is the first to load the configuration and only because other components have loaded the configuration earlier does it execute fast before the extensions are coming up.

I wonder if the fact that the configuration service was long running in 1.0.0 prevented the race condition because the modeServiceImpl did call loadConfiguration probably very early on and this was going to disk to look for settings. Meanwhile the extensions might have had enough time to be notified about the configuration from their package.json.

Again I would argue that no extension should get activated before all configurations have been synchronised to the extension host. We need to somehow flush the configuration to the extension host if possible.

[jrieken]

Again I would argue that no extension should get activated before all configurations have been synchronised to the extension host. We need to somehow flush the configuration to the extension host if possible.

I was always under that assumption. That's why there is the early return which should have been an assertion. Tho, I believe the first configuration has been set, but the extension specific config might be missing so that this happens

[alexdima]

To recap (am I understanding this?):

• extensions contribute to configuration new options and default values in package.json files
• extensions get scanned and their package.json gets communicated to the main process in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/node/extensionHostMain.ts#L142
• extensions (when activated) rely on their default values being present in the vscode config API

I think we need to add something around https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/node/extensionHostMain.ts#L142 where we'd call configurationService.sync() : TPromise<void> or something ?

[bpasero]

@jrieken yes, it should be an assertion at that point

@alexandrudima yes, extensions get scanned for configuration and they end up in the configuration service on the main side that merges them into the cached configuration (the one that is loaded right on startup). this works all fine, but the configuration is not sent back to the extension host in-time before the extension accesses this configuration.

extensions (when activated) rely on their default values being present in the vscode config API

Yes, however I think the issue is more generic: We see an extension accessing the configuration service before the configuration service had a chance to send its configuration over to the extension host. That results in NPEs.

+1 for having a way to flush all configuration values to the extension host and only then continue to activate extensions.

[alexdima]

@bpasero We could get it to reproduce only on the insiders build (not from dev, not from built without minification, really, only in the insiders build).

Steps to verify:

• install path intellisense extension
• reload a lot of times
• no exceptions should show, as the activation of extensions is now blocked on the main side confirming it received the extension descriptions.
• because the extension descriptions are handled sync (i.e. the configuration extension point handles the new config defaults sync, they should make it across to the extension host before the main side can confirm it received the extension descriptions).

[bpasero]

@alexandrudima thanks, I was able to reproduce from insiders on windows too.