Fix for settings crash on toggling enable button for any PowerToy

[alekhyareddy28]

Summary of the Pull Request

What is this about?

This PR fixes the file IO Exception that the file is in use by another process.
This behavior is noticed when toggling the enable button on any of the pages.

PR Checklist

• Applies to [Settings app] Crash when toggling FancyZones on/off #5955, [Watson][fef87818-f2c9-063c-a2c2-f198ec356b68]STOWED_EXCEPTION_System.IO.IOException_80070020_System.IO.FileSystem.dll! System.IO.File.InternalReadAllText #6480
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

What does this include?

The general settings.json file on being accessed by settings was throwing a file in use IOException because it was being used by another process.

The way settings and runner worked before this PR is as follows: the settings on start-up checks if the general settings.json file exists and reads from it, if not it creates a new one. The runner on the other hand during start-up does not save to the file (apply_general_settings is passed with a false save argument). There is no contention during this part.

However, the issue occurs when settings tries to open the general settings.json file everytime it needs information in any viewmodel or converter. When any change is made in the settings, that information is communicated to the runner and it is the runner's responsibility to write to the settings.json file. However, since we have one process continuously trying to read from a file and the other writing to the same file, there is contention.

To fix this issue, the following changes have been made -

• Rather than adding a retry-block everywhere the settings is being accessed, the need to open the file each time has been removed by making the generalViewModel's GeneralSettingsConfig a public static variable.
• The shell page of settings (which is the base page) loads the general page on start-up.

public ShellPage()
        {
            InitializeComponent();

            DataContext = ViewModel;
            ShellHandler = this;
            ViewModel.Initialize(shellFrame, navigationView, KeyboardAccelerators);
            shellFrame.Navigate(typeof(GeneralPage));
        }

• Therefore, the general page is the first one to be loaded on start-up and the GeneralSettingsConfig variable is set in it's constructor. The generalViewModel tries to open the settings.json file and loads the information from it if it exists, if not it creates a new settings.json file. Hence, this variable is always initialized when settings is running. This is also the only time the settings.json file is accessed by the settings process (and as mentioned above on start-up runner does not write to the file so there is no contention). From now on any changes are directly accessed by using the GeneralSettingsConfig in the settings process and passed to the runner which is now the only process accessing that file.

Might possibly be related to #6527 as well (even this is a IOException in the settings hotkeycontrol).

Validation Steps Performed

How does someone test & validate?

• All tests pass (the format had to be changed for initializing generalSettings based on the code changes).
• Manually validated in release as well as in debug mode that the crash no longer occurs.
• Also validated that the converters work as expected by changing the theme.
  

[ryanbodrug-microsoft]

@alekhya. Is it possible to abstract the data for the GeneralSettingsViewModel that is shared across view models into an Implementation of an Interface that can be dependency injected instead of statically referenced. I think this would improve the code in the following ways.

1. It facilitates unit testing the various view models that need to react to a particular state of the general settings.
2. [philosophical point] A view model is responsible for providing data to a particular view. Moving to an interface avoids coupling the other viewmodels to the existence of a particular viewmodel, and can encapsulate lazy loading of the information which avoids assumptions of the sequence that other view models get loaded in.

[dsrivastavv]

@alekhyareddy28 What will happen if for some reason writing settings.json fails in runner ? The settings UI would display updated values, even though they have not been written to settings.json file or communicated to other Powertoys.

[alekhyareddy28]

@alekhyareddy28 What will happen if for some reason writing settings.json fails in runner ? The settings UI would display updated values, even though they have not been written to settings.json file or communicated to other Powertoys.

Can you please eloborate why the writing to settings.json file would fail by the runner (I verified that runner writes it as expected)? If it does, then I think it is a bug on the runner end and should be fixed rather than having the settings process try to re-open and read the settings.json file each time. @ryanbodrug-microsoft can correct me here. If we do want to avoid the situation stated by @somil55 then I can't see any other approach than to have retry statements in all the places where we try to open settings.

[dsrivastavv]

Can you please eloborate why the writing to settings.json file would fail by the runner (I verified that runner writes it as expected)? If it does, then I think it is a bug on the runner end and should be fixed rather than having the settings process try to re-open and read the settings.json file each time. @ryanbodrug-microsoft can correct me here. If we do want to avoid the situation stated by @somil55 then I can't see any other approach than to have retry statements in all the places where we try to open settings.

@alekhyareddy28 One situation would be if settings.json is being blocked from writing, say by opening in a word. Also, even if there are bugs on runners end, I feel we should not display incorrect settings and instead handle such issues. I will defer to @ryanbodrug-microsoft on this.

Also, would it be possible to move the writing logic from runner to settings ?

[alekhyareddy28]

Can you please eloborate why the writing to settings.json file would fail by the runner (I verified that runner writes it as expected)? If it does, then I think it is a bug on the runner end and should be fixed rather than having the settings process try to re-open and read the settings.json file each time. @ryanbodrug-microsoft can correct me here. If we do want to avoid the situation stated by @somil55 then I can't see any other approach than to have retry statements in all the places where we try to open settings.

@alekhyareddy28 One situation would be if settings.json is being blocked from writing, say by opening in a word. Also, even if there are bugs on runners end, I feel we should not display incorrect settings and instead handle such issues. I will defer to @ryanbodrug-microsoft on this. Also, would it be possible to move the writing logic from runner to settings ?

We wouldn't be able to move it to the settings because that wouldn't be backward compatible. Further discussion about this was done in issue #5955.

[ryanbodrug-microsoft]

Can you please eloborate why the writing to settings.json file would fail by the runner (I verified that runner writes it as expected)? If it does, then I think it is a bug on the runner end and should be fixed rather than having the settings process try to re-open and read the settings.json file each time. @ryanbodrug-microsoft can correct me here. If we do want to avoid the situation stated by @somil55 then I can't see any other approach than to have retry statements in all the places where we try to open settings.

@alekhyareddy28 One situation would be if settings.json is being blocked from writing, say by opening in a word. Also, even if there are bugs on runners end, I feel we should not display incorrect settings and instead handle such issues. I will defer to @ryanbodrug-microsoft on this. Also, would it be possible to move the writing logic from runner to settings ?

We wouldn't be able to move it to the settings because that wouldn't be backward compatible. Further discussion about this was done in issue #5955.

@somil55 IMHO potential failure on write is another reason to centralize access to the settings. Each view model shouldn't need to synchronize and ensure resiliency wrt to serialization/deserialization of the the data individually. I think that issue could be treated separately though.

[dsrivastavv]

@somil55 IMHO potential failure on write is another reason to centralize access to the settings. Each view model shouldn't need to synchronize and ensure resiliency wrt to serialization/deserialization of the the data individually. I think that issue could be treated separately though.

@ryanbodrug-microsoft I agree that centralizing settings would tackle these issue and it can be handled separately. Thanks for your clarification.

[alekhyareddy28]

@ryanbodrug-microsoft, I've made the changes as suggested.

Now we have a wrapper singleton class, namely GeneralSettingsCache which has a property CommonSettingsConfig. These configurations are shared across all the viewmodels.

I've not touched settingsutils.cs and the file handing/ipc. I was thinking if we need to make any changes to unify that, we can do so after we get your refactoring/testing PR in.

Validated the following:

• Runner and all powertoys respond correctly to changes in settings.
• Settings reads only once (on start-up) from the settings.json file. Throughout it's lifetime it continues to get information from the GeneralSettingsCache.
• Validated that the modelConverter also works as expected and that there is only one instance of the CommonSettingsConfig shared across all the viewmodels.
• All tests pass-
  

[alekhyareddy28]

@ryanbodrug-microsoft, I've made the suggested changes. There is now a generic singleton class SettingsRepository which encapsulates the disk accesses.
So any viewmodel can get the settings information using SettingsRepository<GeneralSettings>.Instance.SettingsConfig, SettingsRepository<ShortcutGuideSettings>.Instance.SettingsConfig.

Right now, FZ, shortcut guide and power preview use this interface along with GeneralSettings. I've validated the a change in the settings UI in each of these reflects in the actual powertoy (other than preview handlers which seem to be on all the time).

As suggested, I've added a lock during initialization of SettingsRepository and also added tests for the same to ensure that multiple instances are not created when accessed by multiple threads.

I shall look into migrating the other PT settings to this repository once I get a better understanding of the way in which each of them accesses the disk/passes the information to runner.

I hvn't moved the IPC delegates to settingsUtils from the view models yet. Shall do that separately as this PR is getting quite big. Thanks for patiently reviewing this PR 😄

[alekhyareddy28]

Also, to add, the GetFile function should ideally not be needed in the long run. Previously GetSettings was just reading from a file and did not have a check to see if the file existed or not.

Since GetSettings has been modified to create the file if it does not exist, GetFile does what getSettings previously used to. This is because in two cases, the default.json file of KBM and settings.json of PT Run, these two files are assumed to be created by runner (if they don't exist) and not settings. Hence, the GetFile() function was added.

[alekhyareddy28]

Pulled the changes for master, which modified the settings tests.
Made the following changes :

• Shortcutguide, FZ, powerPreview don't need settingsUtils to be passed as it is encapsulated within the settings repository's settigns config object. Hence, that has been removed.
• The tests have been refactored so that they work when settings utils is passed to the setting repository generic interface. Previously the stub of GetSettings was a generic with no type constraints, hence it could be easily mocked as <It.IsAnyType>, however since the GetSettings function must now implement the ISettingsConfig interface, the stub has been modified accordingly. There is no functionality in moq to specify a generic with a type constraint, other than using a custom matcher. Even if we go down this route, the class definitions have to be modified to implement ITypeMatcher and they must still be invoked individually by specifying the class, as GetSettings<GeneralSettings>.
• Validated that all refactored tests pass.
  
• Validated that settings and all PowerToys work as expected with and without previous settings config files.

[ryanbodrug-microsoft]

I was hoping that we were going to encapsulate this in the settings repository/settingsutils. Without this, how does someone know when it's appropriate to use IPC as in this case vs SaveSettings. Also to truly fix this issue we may need to synchronize write / reads across the IPC boundary. We would have to do this in a bunch of different places.

[alekhyareddy28]

@ryanbodrug-microsoft, these are the changes that I've made since you last reviewed-

1. unify the way the Modulename is accessed. It was redeclared in multiple places and this would cause an issue if the name is changed only in one place. All the module names are accessed using the Settings.ModuleName, eg: ShortcutGuideSettings.ModuleName.
2. Modified the settingsConfig in settingsRepository so that the we don't have to use TypeOf and categorize the objects into GeneralSettings and BasePTModuleSettings.
3. GetFile is now a private function. Modified the logic of KBM default.json access and PT Run so that we can re-use GetSettings instead of GetFile.
4. Added UpgradeSettingsConfiguration to the ISettingsConfig interface so that the settings file can be upgraded based on some condition. Presently, only the GeneralSettings file is utilizing this to change the PT Version number based on the old PT version and the current PT version that it receives from the helper function. Verified that if the PT version is lower in the general settings.json file, settings saves the file with the new version info.
5. The naming for the PowerToys was inconsistent and the variables were redeclared in multiple places. To have the settings.ModuleName as the main name, all other places should refer to that name. In the tests file the module name for ImgResizer was "ImageResizer" and not "Image Resizer".

As discussed offline, there is still some refactoring that needs to be done for settings, however it would be nice to get this in for this release as it fixes the crash and I'll make the following changes as a part of another PR -

1. Move IPC from the shell page to settingsUtils.
2. Have the viewmodels utilize the settings utils to communicate with runner rather than through having a delegate passed to each of them.
3. We should be able to make the code such that settings doesn't have to save the file directly, it can rather communicate this information to the runner and the runner can save the information.
4. As stated previously, only FZ, Shortcutguide, and powerPreview use settings repository. It should be expanded to the rest so that ideally SaveSettings is made private.
5. If we can somehow have runner tell the settings that it has received the information and has successfully stored it, then we can notify the user if something goes wrong there instead of having settings and runner running completely independently. I need to look more into the way IPC works to do this..