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

Extension: Getting config setting after waiting for update doesn't provide updated value #25508

Closed
eamodio opened this issue Apr 27, 2017 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug config VS Code configuration, set up issues important Issue identified as high-priority verified Verification succeeded
Milestone

Comments

@eamodio
Copy link
Contributor

eamodio commented Apr 27, 2017

  • VSCode Version: Code - Insiders 1.12.0-insider (48ae6e3, 2017-04-26T06:13:08.212Z)
  • OS Version: Windows_NT ia32 10.0.16179

Steps to Reproduce:

  1. Create an extension that has a setting and does the following
// Get the current value of the setting -- lets say its 'foo'
const setting = vscode.workspace.getConfiguration('extension').get('setting');

// Wait for the update to complete (doesn't matter if its async/await or .then)
await vscode.workspace.getConfiguration('extension').update('setting', 'bar');

// Here I would expect updatedSetting to equal 'bar', but instead it will be 'foo'
const updatedSetting = vscode.workspace.getConfiguration('extension').get('setting');
  1. Observe that updatedSetting does not return the updated value

//cc @jrieken

@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug config VS Code configuration, set up issues important Issue identified as high-priority labels Apr 27, 2017
@jrieken jrieken added this to the April 2017 milestone Apr 27, 2017
@jrieken
Copy link
Member

jrieken commented Apr 27, 2017

From the extension host we call this._configurationEditingService.writeConfiguration and the expectation is that the promise it returns doesn't resolve before a configuration change event is fired. I see some queuing and it seems this behaviour has changed, effectively breaking extensions

@sandy081
Copy link
Member

I think this is the case always.. Configuration editing service was queuing always (AFIK) and never waited for event to trigger. Its the same behaviour for core and extensions. Everybody listens to onConfigurationChange event for any changes to configurations. Definitely, editing service can be improved to resolve after configuration is updated both on disk and in memory. So not sure if is a candidate for end game.

@eamodio
Copy link
Contributor Author

eamodio commented Apr 27, 2017

I could certainly be mistaken, but I thought at one point (quite a while ago), I would get the event before the promise resolved (which would be ideal imo) -- or at least I have some recollection of having to change my code to deal with that (though it could have just been a bug I didn't notice before). But I'm pretty sure that I used to be able to query for the value after waiting for the update to get the most recent value (even without the event firing).

And without this guarantee it makes the code an extension needs to write to wait, very error prone, since if you need to wait for the event it gets challenging -- since you aren't guaranteed to get the event (depending on what you are updating). This cause me to need to write ugly code like: https://github.com/eamodio/vscode-toggle-excluded-files/blob/master/src/excludeController.ts#L154

I believe this is also causing some intermittent timing issues I am seeing in GitLens as well

@jrieken
Copy link
Member

jrieken commented Apr 27, 2017

Yes, it should fire the event and update the state before the promise resolves. We try to figure when that got lost...

@sandy081
Copy link
Member

Sorry guys.. It was working before and it was broken by me in 1.11. Here is the change that caused this - 4c2600f

@jrieken jrieken added the verification-found Issue verification failed label Apr 28, 2017
@jrieken jrieken reopened this Apr 28, 2017
@jrieken
Copy link
Member

jrieken commented Apr 28, 2017

Still seeing this

@jrieken jrieken added verified Verification succeeded and removed verification-found Issue verification failed labels Apr 28, 2017
@jrieken
Copy link
Member

jrieken commented Apr 28, 2017

Works now

@sandy081
Copy link
Member

I actually fixed it in configuration editing service to reload configuration after editing. Reload configuration should trigger change event. Global configuration is triggering but workspace configurations is not. Refactorings from very long back (1.4) changed the behaviour of workspace configuration service. It is not triggering change event anymore.

Fixed workspace configuration to trigger event in reload.

eamodio added a commit to eamodio/vscode-toggle-excluded-files that referenced this issue May 7, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug config VS Code configuration, set up issues important Issue identified as high-priority verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants