Do not sync install states across machines#2620
Draft
nagilson wants to merge 3 commits intodotnet:mainfrom
Draft
Do not sync install states across machines#2620nagilson wants to merge 3 commits intodotnet:mainfrom
nagilson wants to merge 3 commits intodotnet:mainfrom
Conversation
https://github.com/microsoft/vscode/blob/231f8d745c9e59b6a62ae33f230002d99216800e/src/vscode-dts/vscode.d.ts#L8454 Keys in the global state are synced when the user turns on configuration settings sync. I had no idea. This means we will sync file paths from different machines, even different OS's, across one another. All of our data keys are related to the specific machine so we should definitely not sync any of them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves microsoft/vscode-dotnettools#2759
There are 2 issues that compound to cause the issue above.
The issue there is caused by:
Issues addressed:
1. Some extension states should not be shared/synced:
Keys in the global extension storage state are synced (from the primary machine to the container image) when the user turns on configuration settings sync. I had no idea. This means we will sync file paths from different machines, even different OS's, across one another. All of our data keys are related to the specific machine so we should definitely not sync any of them.
https://github.com/microsoft/vscode/blob/231f8d745c9e59b6a62ae33f230002d99216800e/src/vscode-dts/vscode.d.ts#L8454
2. The cache does not get invalidated in some cases for install/uninstall.
Upon any install / uninstall operation we should clear the cache of anything with that path in it.
The clear in the invoker is not necessary, but it is a good 'defense in depth'. We should rarely get to that state if ever now?
Potential Other Approaches
Alternatively, we could just decide not to cache any failures. But this would mean no dotnet exists checks would cause a lot of wasted extra processes. I don't think we need to do this since we wipe the 'dotnet' ones after admin install / uninstall but obviously this wouldn't pick up external installers. That's... ok.
Other Considerations
It's technically a bug that in the calls where we wipe a directory, we don't enforce that we checked if the install is in use using the install tracker method.
The only place we don't do that is in the installer itself. Concurrently it needs to hold a mutex on that install for it to work, so this isn't a race condition, I don't think this can be a race condition bug actually. It can only be a sequential bug.
We could enforce the is it in use check on the wipe part of the installer, but it already tries to check if it's valid or not.
By checking if the install is valid while invalidating the cache before hand, we'd truly only invalidate an already invalid install:
install it -> invalidate it -> it will get deleted (bad, but it was invalidated)
Instead of wiping the cache we could do the is in use check:
is in use -> (if its invalid) -> we return invalid install but don't wipe the cache of everything pertaining to that install.
We only wipe the local runtime specific folders in this case (not 'dotnet') so I don't expect a large perf regression.