PowerToys Run cache issue

[dsrivastavv]

Summary of the Pull Request

This PR fixes cache issue in PowerToys Run

References

1. https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminateprocess
2. https://stackoverflow.com/questions/2055753/how-to-gracefully-terminate-a-process

PR Checklist

• Applies to UWP and Win32 cache not loading on starting launcher #4263
• 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

Detailed Description of the Pull Request / Additional comments

Fixed Issue :

1. No cache was being written to disk on enabling/disabling powertoys run. This was because TerminateProcess was used to stop powerlauncher.exe which caused immediate termination of the process and no exit callbacks were being invoked. To fix this, we first send WM_CLOSE signal to process and give it 2sec time to stop itself, otherwise we kill it using TerminateProcess.
2. Serialization error was being thrown when writing/reading cache for Image and Pinyin. This is because ConcurrentDictionary object is not serializable in .net core (the reason it works in wox is because ConcurrentDictionary is serializable in .net framework)

Validation Steps Performed

1. Manually validated that all tests pass.
2. Manually validated from logs (%userprofile%/appdata/local/microsoft/powertoys/powertoys run/logs) that cache is being written on termination.

[alekhyareddy28]

1. The cache is not being written unless I launch Powertoys Run atleast once. You can see that there is only one exit log and multiple start-up logs. I don't think that should be a factor in deciding whether we store the cache/json files or not.
   2020-06-25.txt

2. I noticed that PT Run does not properly dispose and write the cache while exiting runner from VS. The path the code takes then is that it checks if the parent process (settings/runner) is alive or not and if not, it exits. I guess that path has not been taken into consideration.

[alekhyareddy28]

[clarification] can you specify what signal would be sent when the process is closed? Like we would get a WAIT_TIMEOUT if no signal is sent within 2 seconds.

[dsrivastavv]

I don't think we can specify the signal the object can return. WaitForSingleObject return values are limited as mentioned here : https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobject

[dsrivastavv]

1. The cache is not being written unless I launch Powertoys Run atleast once. You can see that there is only one exit log and multiple start-up logs. I don't think that should be a factor in deciding whether we store the cache/json files or not.
   2020-06-25.txt

I am not entirely sure what might be causing this. If I create a similar scenario and set launcher as startup project, cache is being written. @ryanbodrug-microsoft Will it be fine if we fix this issue later? Alekhya is blocked on #4424 because of this PR. This issue doesn't have any user-facing consequences because user preferences don't change until Powertoys run is launched atleast once.

2. I noticed that PT Run does not properly dispose and write the cache while exiting runner from VS. The path the code takes then is that it checks if the parent process (settings/runner) is alive or not and if not, it exits. I guess that path has not been taken into consideration.

This was being caused because Environment.Exit function doesn't always terminate process cleanly. So I have modified the codepath to explicitly call Dispose before Environment.Exit.

[ryanbodrug-microsoft]

1. ce. You can see that there is only one exit log and multiple start-up logs. I don't think that should be a factor in deciding whether we store the cache/json files or not.
   2020-06-25.txt

@divyansh It's okay to go forward with this, as long as you can verify verify that the behavior observed isn't a knock on of this change. Could you perhaps split this PR into seperate PRs for the same issue. @alekhyareddy28 would submitting the pinyin concurrent dictionary issue unblock you or do you need both fixes?

[alekhyareddy28]

I just noticed another issue when I pulled the changes from this branch. The SaveAppAllSettings is not called everytime when I disable PT Run from settings. It is called the first time and hits the breakpoint that i set for _mainVM.Save() but none of the other functions. From the next time onwards when i disable it, none of the functions within SaveAppAllSettings() get executed. @ryanbodrug-microsoft , the above issue does not block me, but this does. @somil55, can you look into it?

Steps to repro -

1. Launch runner from VS.
2. Attach debugger to PowerLauncher.
3. Set brakpoints in the SaveAppAllSettings() function (ie. _mainVM.Save(), PluginManager.Save() etc)
4. Launch PT Run once (using Alt+Space) because otherwise the dispose functions are not called.
5. Disable PT Run from settings.

I observe that the Exit log is logged but the Save functions are not executed.

[alekhyareddy28]

As discussed with @somil55, It seems to be happening because of timing out and calling Terminate Process. Maybe we can increase it as I see that the imagecache is not stored sometimes (even without the debugger) because of a timeout before imagecache.save().

[dsrivastavv]

I just noticed another issue when I pulled the changes from this branch. The SaveAppAllSettings is not called everytime when I disable PT Run from settings. It is called the first time and hits the breakpoint that i set for _mainVM.Save() but none of the other functions. From the next time onwards when i disable it, none of the functions within SaveAppAllSettings() get executed. @ryanbodrug-microsoft , the above issue does not block me, but this does. @somil55, can you look into it?

Steps to repro -

1. Launch runner from VS.
2. Attach debugger to PowerLauncher.
3. Set brakpoints in the SaveAppAllSettings() function (ie. _mainVM.Save(), PluginManager.Save() etc)
4. Launch PT Run once (using Alt+Space) because otherwise the dispose functions are not called.
5. Disable PT Run from settings.

I observe that the Exit log is logged but the Save functions are not executed.

The issue is because runner and powerlauncher are different processes and hitting a debugger on one process doesn't stop the execution of the other process. So when you hit a breakpoint in powerlauncher.exe, you cross 2 sec mark on the runner process and it terminates powerlauncher.exe. So when you continue from breakpoint on powerlauncher.exe, it exits and you don't hit further breakpoints.

You can set : static const int MAX_WAIT_MILLISEC = INFINITE; in dllmain.cpp for debugging purposes.