Powertoys: ColorPicker and PT Run take 10 seconds to exit

[enricogior]

ℹ Computer information

• PowerToys version: 0.20.1
• PowerToy module: ColorPicker, PT Run

📝 Provide detailed reproduction steps (if any)

1. Turn off ColorPicker or PT Run in the Settings

✔️ Expected result

ColorPIcker.exe and PowerLauncher.exe should quit immediately

❌ Actual result

It takes 10 seconds for ColorPIcker.exe or PowerLauncher.exe to quit

Note: the terminateProcess() function:
https://github.com/microsoft/PowerToys/blob/b8b6dbe791fd3eddb25d8712ec03cf2251ed5bf2/src/modules/colorPicker/ColorPicker/dllmain.cpp#L157-L166
always ends up killing the process:
https://github.com/microsoft/PowerToys/blob/b8b6dbe791fd3eddb25d8712ec03cf2251ed5bf2/src/modules/colorPicker/ColorPicker/dllmain.cpp#L164

[somil55]

@enricogior The issue here seems to be because pt run isn't able to cleanly terminate itself. This codepath was added in #4472 because we were earlier using TerminateProcess to stop powerlauncher.exe which caused immediate termination of the process and no exit callbacks were being invoked. To fix this, we replaced it with sending WM_CLOSE signal to powerlauncher.exe process and give it 10 sec to cleanly terminate itself, otherwise, we force kill it using TerminateProcess.

[arjunbalgovind]

@enricogior I will be looking into running the PowerLauncher.exe process as admin (for #4427) and launching apps from within that non-elevated (unless the RunAsAdmin context menu button is pressed). Would this delay to exit still be problematic for Restart as admin if we make that change?

[enricogior]

@somil55
the problem here is that PowerLauncher.exe never exits by itself, and 100% of the times it gets killed, so we either fix the code to do a clean exit or we simply go back to kill it right away.

@arjunbalgovind
yes, the delay is still problematic and needs to be fixed. If it can't be fixed in PowerLauncher.exe, we need to revisit the "restart as admin" code since it will have to wait until all modules quits before starting the new process.

[arjunbalgovind]

@enricogior can you point me to the code where we terminate and relaunch PowerLauncher.exe when Restart as admin is pressed? If it is the same logic within the Launcher::enabled() method, we could look into either force terminating in that scenario or adding a second thread to wait until termination and then relaunch the process, so that we don't block the runner thread.

[enricogior]

@arjunbalgovind
the logic is in Launcher::enabled(), but the first problem to fix is that the current disable logic is not working and PT Run gets killed after the timeout expires.
There are two scenarios here:
1 - the WM_CLOSE event is not received/processed by PT Run
2 - PT Run processes the event but it takes forever to quit

Also the event should not be sent to all PT Run windows
https://github.com/microsoft/PowerToys/blob/b8b6dbe791fd3eddb25d8712ec03cf2251ed5bf2/src/modules/launcher/Microsoft.Launcher/dllmain.cpp#L222-L223
but only to the main one and then return false.
And we need to verify if that code does actually work for applications that don't have a visible window.

[yuyoyuppe]

Launcher is always terminated for me when its window is hidden. EnumWindows isn't able to communicate with the launcher process while its window is hidden. I've commented out the hiding logic when the window loses focus and couldn't reproduce the issue anymore. Otherwise, launcher never terminates for me (tried with INFINITE timout before terminate call) when disabling/exiting PT.

I suggest we use named event for that instead of relying on WM_CLOSE.

Another thing, we already react to the process powertoys.exe exit, and rely solely on that technique for ColorPicker/FZE/.../, so I wonder if we should remove the terminateProcess call there, and send event only when we disable the module.

[arjunbalgovind]

We should also make sure the approach we go for is non-blocking in runner to avoid the issue described in #6676. (Runner was getting blocked on the WaitForSingleObject call)

[enricogior]

@arjunbalgovind
thanks for info.

[crutkas]

@enricogior / @arjunbalgovind / @ivan100sic / @yuyoyuppe are we viewing this work item as done?

[yuyoyuppe]

can't repro on a recent master

[enricogior]

@crutkas
for now it's fixed with a workaround: we kill the process right away instead of waiting 10 seconds and then kill it anyway.
Clean exit in PT Run wasn't working for unknown reason and a brief investigation wasn't enough. @ivan100sic may have more details.

[crutkas]

@ivan100sic thoughts?

[ivan100sic]

It definitely has to do with C# code of PT Run, not the PowerToy interface or dllmain. When we have better understanding of PT Run itself, I guess then we'll be able to do a clean exit.

[crutkas]

so based on @yuyoyuppe the current workarounds in place help and can't be repo'ed against master. I think this can be migrated to a 0.29 December task then

[enricogior]

@crutkas
given that since day one we always killed PT Run and never exited gracefully, it exiting gracefully a must have? What are the possible problems of killing the process?

[arjunbalgovind]

@enricogior Launcher has logic for saving some caches (for example Image cache) when the process disposes https://github.com/microsoft/PowerToys/blob/f97ed9c34006d2facfa9bf58d2097fe0f0426237/src/modules/launcher/PowerLauncher/App.xaml.cs#L187-L192. This is also explicitly called when the "exit when PowerToys.exe terminates" handler is called https://github.com/microsoft/PowerToys/blob/f97ed9c34006d2facfa9bf58d2097fe0f0426237/src/modules/launcher/PowerLauncher/App.xaml.cs#L62-L73.
@somil55 and @alekhyareddy28 can add more context on this.

[crutkas]

In the 0.25 release. https://github.com/microsoft/PowerToys/releases/tag/v0.25.0