From 0fa9792718d77e153020e4be982db279094644a5 Mon Sep 17 00:00:00 2001 From: rc-swag <58423624+rc-swag@users.noreply.github.com> Date: Wed, 5 Feb 2025 15:26:13 +1000 Subject: [PATCH 1/5] feat(windows): mutex used to check download process Using the KeymanMutex wrapper to check if a download process is occuring if it isn't and we are in the downloading state this means the download process exited early. We can then clean up any downloaded files and reset the statemachine and check for updates again. --- .../main/Keyman.System.UpdateStateMachine.pas | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas index a09a9e5068..5f8d91a959 100644 --- a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas +++ b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas @@ -118,6 +118,7 @@ uses GlobalProxySettings, kmint, keymanapi_TLB, + KeymanMutex, Keyman.System.KeymanSentryClient, Keyman.System.DownloadUpdate, Keyman.System.RemoteUpdateCheck, @@ -753,12 +754,21 @@ procedure DownloadingState.Enter; var DownloadResult: Boolean; RetryCount: Integer; + FMutex: TKeymanMutex; begin // Enter DownloadingState bucStateContext.SetRegistryState(usDownloading); RetryCount := 0; DownloadResult := False; + FMutex := TKeymanMutex.Create('KeymanDownloading'); + // Should be impossible but just exit anyway and let the process current + // downloading process finish. + if not FMutex.MutexOwned then + begin + FreeAndNil(FMutex); + Exit; + end; while (not DownloadResult) and (RetryCount < 3) do begin @@ -767,6 +777,8 @@ begin Inc(RetryCount); end; + FreeAndNil(FMutex); + if (not DownloadResult) then begin // Failed three times in this process; return to the @@ -807,14 +819,38 @@ begin end; function DownloadingState.HandleKmShell; +var + FMutex: TKeymanMutex; begin - // Downloading state, in other process, so continue + // Check to ensure a download process is running if not + // clean up return to the the idle state and check for updates + FMutex := TKeymanMutex.Create('KeymanDownloading'); + if FMutex.MutexOwned then + begin + bucStateContext.RemoveCachedFiles; + FreeAndNil(FMutex); + ChangeState(IdleState); + bucStateContext.CurrentState.HandleCheck; + end; + FreeAndNil(FMutex); + // Downloading state, in another process, so continue to execute kmshell Result := kmShellContinue; end; procedure DownloadingState.HandleDownload; +var + FMutex: TKeymanMutex; begin - // Enter Already Downloading + // If downloading process is not running clean files and return to idle + FMutex := TKeymanMutex.Create('KeymanDownloading'); + if FMutex.MutexOwned then + begin + bucStateContext.RemoveCachedFiles; + FreeAndNil(FMutex); + ChangeState(IdleState); + bucStateContext.CurrentState.HandleCheck; + end; + FreeAndNil(FMutex); end; procedure DownloadingState.HandleAbort; From 01690ed483f248595c7ff457fc7f4d95f3b05562 Mon Sep 17 00:00:00 2001 From: rc-swag <58423624+rc-swag@users.noreply.github.com> Date: Mon, 10 Feb 2025 10:05:14 +1000 Subject: [PATCH 2/5] feat(windows): remove redunant FreeAndNil --- .../main/Keyman.System.UpdateStateMachine.pas | 77 +++++++++++-------- 1 file changed, 46 insertions(+), 31 deletions(-) diff --git a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas index 5f8d91a959..b1d4193a35 100644 --- a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas +++ b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas @@ -761,24 +761,28 @@ begin RetryCount := 0; DownloadResult := False; - FMutex := TKeymanMutex.Create('KeymanDownloading'); - // Should be impossible but just exit anyway and let the process current - // downloading process finish. - if not FMutex.MutexOwned then - begin + FMutex := nil; + + try + FMutex := TKeymanMutex.Create('KeymanDownloading'); + // Should be impossible but just exit anyway and let the process current + // downloading process finish. + if not FMutex.MutexOwned then + begin + Exit; + end; + + while (not DownloadResult) and (RetryCount < 3) do + begin + DownloadResult := DownloadUpdatesBackground; + if not DownloadResult then + Inc(RetryCount); + end; + + finally FreeAndNil(FMutex); - Exit; end; - while (not DownloadResult) and (RetryCount < 3) do - begin - DownloadResult := DownloadUpdatesBackground; - if not DownloadResult then - Inc(RetryCount); - end; - - FreeAndNil(FMutex); - if (not DownloadResult) then begin // Failed three times in this process; return to the @@ -822,35 +826,46 @@ function DownloadingState.HandleKmShell; var FMutex: TKeymanMutex; begin + FMutex := nil; + // Whether already downloading in another process or download has failed and + // this function has clean up and force a restart, kmshell should continue processing + Result := kmShellContinue; // Check to ensure a download process is running if not // clean up return to the the idle state and check for updates - FMutex := TKeymanMutex.Create('KeymanDownloading'); - if FMutex.MutexOwned then - begin - bucStateContext.RemoveCachedFiles; + try + FMutex := TKeymanMutex.Create('KeymanDownloading'); + if FMutex.MutexOwned then + begin + bucStateContext.RemoveCachedFiles; + FreeAndNil(FMutex); // Mutex must be freed before changing state + ChangeState(IdleState); + bucStateContext.CurrentState.HandleCheck; + Exit; + end; + finally FreeAndNil(FMutex); - ChangeState(IdleState); - bucStateContext.CurrentState.HandleCheck; end; - FreeAndNil(FMutex); - // Downloading state, in another process, so continue to execute kmshell - Result := kmShellContinue; end; procedure DownloadingState.HandleDownload; var FMutex: TKeymanMutex; begin + FMutex := nil; // If downloading process is not running clean files and return to idle - FMutex := TKeymanMutex.Create('KeymanDownloading'); - if FMutex.MutexOwned then - begin - bucStateContext.RemoveCachedFiles; + try + FMutex := TKeymanMutex.Create('KeymanDownloading'); + if FMutex.MutexOwned then + begin + bucStateContext.RemoveCachedFiles; + FreeAndNil(FMutex); // Mutex must be freed before changing state + ChangeState(IdleState); + bucStateContext.CurrentState.HandleCheck; + Exit; + end; + finally FreeAndNil(FMutex); - ChangeState(IdleState); - bucStateContext.CurrentState.HandleCheck; end; - FreeAndNil(FMutex); end; procedure DownloadingState.HandleAbort; From 8281e328c9a9cfe6abbb1d5bd000aa0eca113194 Mon Sep 17 00:00:00 2001 From: rc-swag <58423624+rc-swag@users.noreply.github.com> Date: Mon, 10 Feb 2025 14:37:59 +1000 Subject: [PATCH 3/5] feat(windows): review changes --- .../main/Keyman.System.UpdateStateMachine.pas | 33 ++++++++----------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas index 53c9e71b19..e79dd299c4 100644 --- a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas +++ b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas @@ -132,6 +132,7 @@ const SPackageUpgradeFilename = 'upgrade_packages.inf'; kmShellContinue = 0; kmShellExit = 1; + KeymanDownloadMutexName = 'KeymanDownloading'; { State Class Memebers } @@ -759,25 +760,21 @@ begin // Enter DownloadingState bucStateContext.SetRegistryState(usDownloading); - RetryCount := 0; - DownloadResult := False; - FMutex := nil; + FMutex := TKeymanMutex.Create(KeymanDownloadMutexName); try - FMutex := TKeymanMutex.Create('KeymanDownloading'); // Should be impossible but just exit anyway and let the process current // downloading process finish. - if not FMutex.MutexOwned then + if not FMutex.TakeOwnership then begin Exit; end; - while (not DownloadResult) and (RetryCount < 3) do - begin + RetryCount := 0; + repeat DownloadResult := DownloadUpdatesBackground; - if not DownloadResult then - Inc(RetryCount); - end; + Inc(RetryCount); + until DownloadResult or (RetryCount = 3); finally FreeAndNil(FMutex); @@ -824,21 +821,19 @@ function DownloadingState.HandleKmShell; var FMutex: TKeymanMutex; begin - FMutex := nil; // Whether already downloading in another process or download has failed and // this function has clean up and force a restart, kmshell should continue processing Result := kmShellContinue; // Check to ensure a download process is running if not // clean up return to the the idle state and check for updates + FMutex := TKeymanMutex.Create(KeymanDownloadMutexName); try - FMutex := TKeymanMutex.Create('KeymanDownloading'); - if FMutex.MutexOwned then + if FMutex.TakeOwnership then begin bucStateContext.RemoveCachedFiles; - FreeAndNil(FMutex); // Mutex must be freed before changing state + FMutex.ReleaseOwnership; // Mutex must be freed before changing state ChangeState(IdleState); bucStateContext.CurrentState.HandleCheck; - Exit; end; finally FreeAndNil(FMutex); @@ -849,17 +844,15 @@ procedure DownloadingState.HandleDownload; var FMutex: TKeymanMutex; begin - FMutex := nil; // If downloading process is not running clean files and return to idle + FMutex := TKeymanMutex.Create(KeymanDownloadMutexName); try - FMutex := TKeymanMutex.Create('KeymanDownloading'); - if FMutex.MutexOwned then + if FMutex.TakeOwnership then begin bucStateContext.RemoveCachedFiles; - FreeAndNil(FMutex); // Mutex must be freed before changing state + FMutex.ReleaseOwnership; // Mutex must be freed before changing state ChangeState(IdleState); bucStateContext.CurrentState.HandleCheck; - Exit; end; finally FreeAndNil(FMutex); From 7ac190fc1be846ce0433f8bf976e01d59eeb3fb5 Mon Sep 17 00:00:00 2001 From: rc-swag <58423624+rc-swag@users.noreply.github.com> Date: Tue, 11 Feb 2025 15:47:14 +1000 Subject: [PATCH 4/5] feat(windows): commit review suggestion Co-authored-by: Eberhard Beilharz --- .../desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas index e79dd299c4..dcb6f5a6d6 100644 --- a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas +++ b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas @@ -774,7 +774,7 @@ begin repeat DownloadResult := DownloadUpdatesBackground; Inc(RetryCount); - until DownloadResult or (RetryCount = 3); + until DownloadResult or (RetryCount >= 3); finally FreeAndNil(FMutex); From fa45fd88e52e05d0de1c18dcfaf9f43bda0a5740 Mon Sep 17 00:00:00 2001 From: rc-swag <58423624+rc-swag@users.noreply.github.com> Date: Tue, 11 Feb 2025 15:54:58 +1000 Subject: [PATCH 5/5] feat(windows): release ownership of mutex successful download Added a release of ownership for the mutex download after in the successuful download case --- .../desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas index dcb6f5a6d6..afce261cfa 100644 --- a/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas +++ b/windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas @@ -775,7 +775,7 @@ begin DownloadResult := DownloadUpdatesBackground; Inc(RetryCount); until DownloadResult or (RetryCount >= 3); - + FMutex.ReleaseOwnership; finally FreeAndNil(FMutex); end;