From 4717501072b30467992a6de67ac134607428bcba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Fri, 10 Jul 2026 13:39:35 -0400 Subject: [PATCH 1/2] Fix deterministic resource lifetimes Dispose HTTP, process, COM, image, task, and UI subscription resources deterministically; add cancellation and lifetime regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Classes/Helper.cs | 10 +- .../Controls/UserAvatarViewModel.cs | 79 ++++++- .../Pages/SettingsPages/BackupViewModel.cs | 102 +++++++-- .../Views/Controls/UserAvatarControl.axaml.cs | 14 +- .../Views/Pages/SettingsPages/Backup.axaml.cs | 17 +- .../SettingsPages/SettingsBasePage.axaml.cs | 33 ++- .../TaskRecyclerTests.cs | 21 ++ src/UniGetUI.Core.Classes/TaskRecycler.cs | 122 ++++------- .../IconCacheEngine.cs | 37 ++-- src/UniGetUI.Core.Tools/Tools.cs | 17 +- .../TelemetryHandlerTests.cs | 39 ++++ .../TelemetryHandler.cs | 4 +- .../Bun.cs | 2 +- .../CratesIOClient.cs | 7 +- .../Chocolatey.cs | 2 +- .../BaseNuGet.cs | 12 +- .../BaseNuGetDetailsHelper.cs | 6 +- .../Internal/NuGetManifestLoader.cs | 53 +++-- .../Npm.cs | 2 +- .../Helpers/PipPkgDetailsHelper.cs | 14 +- .../Pip.cs | 15 +- .../PowerShell.cs | 2 +- .../PowerShell7.cs | 2 +- .../Scoop.cs | 4 +- .../Helpers/VcpkgPkgDetailsHelper.cs | 16 +- .../Vcpkg.cs | 2 +- .../ClientHelpers/WinGetIconsHelper.cs | 2 +- .../WinGet.cs | 2 +- .../AbstractOperation.cs | 202 ++++++++++++++---- .../AbstractProcessOperation.cs | 150 ++++++++----- .../DownloadOperation.cs | 104 ++++----- .../KillProcessOperation.cs | 66 +++--- .../PrePostOperation.cs | 98 +++++++-- .../PackageOperationsTests.cs | 63 ++++++ 34 files changed, 920 insertions(+), 401 deletions(-) diff --git a/src/ExternalLibraries.FilePickers/Classes/Helper.cs b/src/ExternalLibraries.FilePickers/Classes/Helper.cs index bd040acba1..fe98c22494 100644 --- a/src/ExternalLibraries.FilePickers/Classes/Helper.cs +++ b/src/ExternalLibraries.FilePickers/Classes/Helper.cs @@ -17,6 +17,7 @@ internal static class Helper internal static string ShowOpen(nint windowHandle, FOS fos, List? typeFilters = null) { FileOpenDialog dialog = new(); + IShellItem item = null!; try { dialog.SetOptions(fos); @@ -36,13 +37,15 @@ internal static string ShowOpen(nint windowHandle, FOS fos, List? typeFi return string.Empty; } - dialog.GetResult(out IShellItem item); + dialog.GetResult(out item); item.GetDisplayName(SIGDN.SIGDN_FILESYSPATH, out string path); return path; } finally { #pragma warning disable CA1416 + if (item is not null) + Marshal.ReleaseComObject(item); Marshal.ReleaseComObject(dialog); #pragma warning restore CA1416 } @@ -56,6 +59,7 @@ internal static string ShowSave( ) { FileSaveDialog dialog = new(); + IShellItem item = null!; try { dialog.SetOptions(fos); @@ -79,7 +83,7 @@ internal static string ShowSave( return string.Empty; } - dialog.GetResult(out IShellItem item); + dialog.GetResult(out item); item.GetDisplayName(SIGDN.SIGDN_FILESYSPATH, out string path); dialog.GetFileTypeIndex(out uint selection); @@ -92,6 +96,8 @@ internal static string ShowSave( finally { #pragma warning disable CA1416 + if (item is not null) + Marshal.ReleaseComObject(item); Marshal.ReleaseComObject(dialog); #pragma warning restore CA1416 } diff --git a/src/UniGetUI.Avalonia/ViewModels/Controls/UserAvatarViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/Controls/UserAvatarViewModel.cs index f99ca9b1be..5e0eedeb4e 100644 --- a/src/UniGetUI.Avalonia/ViewModels/Controls/UserAvatarViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/Controls/UserAvatarViewModel.cs @@ -9,8 +9,13 @@ namespace UniGetUI.Avalonia.ViewModels.Controls; -public class UserAvatarViewModel : ViewModelBase +public class UserAvatarViewModel : ViewModelBase, IDisposable { + private readonly CancellationTokenSource _lifetimeCancellation = new(); + private readonly CancellationToken _lifetimeToken; + private int _isDisposed; + private long _refreshGeneration; + private bool _isAuthenticated; public bool IsAuthenticated { @@ -38,15 +43,32 @@ public Bitmap? AvatarBitmap public UserAvatarViewModel() { + _lifetimeToken = _lifetimeCancellation.Token; LoginCommand = new AsyncRelayCommand(LoginAsync); LogoutCommand = new MvvmRelayCommand(Logout); MoreDetailsCommand = new MvvmRelayCommand(() => CoreTools.Launch("https://devolutions.net/unigetui")); - GitHubAuthService.AuthStatusChanged += (_, _) => _ = RefreshAsync(); + GitHubAuthService.AuthStatusChanged += OnAuthStatusChanged; _ = RefreshAsync(); } + private bool IsDisposed => Volatile.Read(ref _isDisposed) != 0; + + private bool CanApplyRefresh(long generation) => + !IsDisposed + && !_lifetimeToken.IsCancellationRequested + && generation == Volatile.Read(ref _refreshGeneration); + + private void OnAuthStatusChanged(object? sender, EventArgs e) + { + if (!IsDisposed) + _ = RefreshAsync(); + } + private async Task RefreshAsync() { + if (IsDisposed) return; + + long generation = Interlocked.Increment(ref _refreshGeneration); var service = new GitHubAuthService(); bool authenticated = GitHubAuthService.IsAuthenticated(); @@ -61,6 +83,8 @@ private async Task RefreshAsync() if (client is not null) { GitHubUser user = await client.GetCurrentUserAsync(); + if (!CanApplyRefresh(generation)) return; + displayName = string.IsNullOrEmpty(user.Name) ? $"@{user.Login}" : $"{user.Name} (@{user.Login})"; @@ -68,26 +92,47 @@ private async Task RefreshAsync() if (!string.IsNullOrEmpty(user.AvatarUrl)) { using var http = new HttpClient(CoreTools.GenericHttpClientParameters); - byte[] bytes = await http.GetByteArrayAsync(user.AvatarUrl); + byte[] bytes = await http.GetByteArrayAsync(user.AvatarUrl, _lifetimeToken); + if (!CanApplyRefresh(generation)) return; + using var ms = new MemoryStream(bytes); bitmap = new Bitmap(ms); } } } + catch (OperationCanceledException) when (_lifetimeToken.IsCancellationRequested) + { + return; + } catch (Exception ex) { - Logger.Warn("UserAvatarViewModel: failed to fetch GitHub user info"); - Logger.Warn(ex); + if (!IsDisposed) + { + Logger.Warn("UserAvatarViewModel: failed to fetch GitHub user info"); + Logger.Warn(ex); + } authenticated = false; } } + if (!CanApplyRefresh(generation)) + { + bitmap?.Dispose(); + return; + } + await Dispatcher.UIThread.InvokeAsync(() => { + if (!CanApplyRefresh(generation)) return; + IsAuthenticated = authenticated; UserDisplayName = displayName; AvatarBitmap = bitmap; + bitmap = null; }); + + // A queued UI update can become obsolete while it is waiting to run. + bitmap?.Dispose(); } private async Task LoginAsync() @@ -98,8 +143,11 @@ private async Task LoginAsync() } catch (Exception ex) { - Logger.Error("UserAvatarViewModel: login failed"); - Logger.Error(ex); + if (!IsDisposed) + { + Logger.Error("UserAvatarViewModel: login failed"); + Logger.Error(ex); + } } } @@ -108,8 +156,21 @@ private void Logout() try { new GitHubAuthService().SignOut(); } catch (Exception ex) { - Logger.Error("UserAvatarViewModel: logout failed"); - Logger.Error(ex); + if (!IsDisposed) + { + Logger.Error("UserAvatarViewModel: logout failed"); + Logger.Error(ex); + } } } + + public void Dispose() + { + if (Interlocked.Exchange(ref _isDisposed, 1) != 0) return; + + GitHubAuthService.AuthStatusChanged -= OnAuthStatusChanged; + // In-flight HTTP work can still observe this token. Cancelling is sufficient; + // disposing the source here could race with that work. + _lifetimeCancellation.Cancel(); + } } diff --git a/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/BackupViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/BackupViewModel.cs index d0f3e43562..d2a79a2fa2 100644 --- a/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/BackupViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/BackupViewModel.cs @@ -17,7 +17,7 @@ namespace UniGetUI.Avalonia.ViewModels.Pages.SettingsPages; -public partial class BackupViewModel : ViewModelBase +public partial class BackupViewModel : ViewModelBase, IDisposable { public event EventHandler? RestartRequired; @@ -43,28 +43,48 @@ public partial class BackupViewModel : ViewModelBase [ObservableProperty] private IImage? _gitHubAvatarBitmap; private readonly GitHubAuthService _authService = new(); + private readonly CancellationTokenSource _lifetimeCancellation = new(); + private readonly CancellationToken _lifetimeToken; private bool _isLoading; + private int _isDisposed; + private long _statusGeneration; public BackupViewModel() { + _lifetimeToken = _lifetimeCancellation.Token; _isLocalBackupEnabled = CoreSettings.Get(CoreSettings.K.EnablePackageBackup_LOCAL); RefreshDirectoryLabel(); - GitHubAuthService.AuthStatusChanged += (_, _) => _ = UpdateGitHubLoginStatus(); + GitHubAuthService.AuthStatusChanged += OnAuthStatusChanged; _ = UpdateGitHubLoginStatus(); } + private bool IsDisposed => Volatile.Read(ref _isDisposed) != 0; + + private bool CanApplyStatus(long generation) => + !IsDisposed + && !_lifetimeToken.IsCancellationRequested + && generation == Volatile.Read(ref _statusGeneration); + + private void OnAuthStatusChanged(object? sender, EventArgs e) + { + if (!IsDisposed) + _ = UpdateGitHubLoginStatus(); + } + /* ─────────────── Local backup ─────────────── */ [RelayCommand] private void EnableLocalBackupChanged() { + if (IsDisposed) return; IsLocalBackupEnabled = CoreSettings.Get(CoreSettings.K.EnablePackageBackup_LOCAL); RestartRequired?.Invoke(this, EventArgs.Empty); } private void RefreshDirectoryLabel() { + if (IsDisposed) return; string dir = CoreSettings.GetValue(CoreSettings.K.ChangeBackupOutputDirectory); BackupDirectoryLabel = string.IsNullOrEmpty(dir) ? CoreData.UniGetUI_DefaultBackupDirectory : dir; } @@ -72,12 +92,12 @@ private void RefreshDirectoryLabel() [RelayCommand] private async Task PickBackupDirectory(Visual? visual) { - if (visual is null || TopLevel.GetTopLevel(visual) is not { } topLevel) return; + if (IsDisposed || visual is null || TopLevel.GetTopLevel(visual) is not { } topLevel) return; var folders = await topLevel.StorageProvider.OpenFolderPickerAsync(new FolderPickerOpenOptions { AllowMultiple = false, }); - if (folders is not [{ } folder]) return; + if (IsDisposed || folders is not [{ } folder]) return; var path = folder.TryGetLocalPath(); if (path is null) return; CoreSettings.SetValue(CoreSettings.K.ChangeBackupOutputDirectory, path); @@ -129,36 +149,51 @@ public static async Task DoLocalBackupStatic() private async Task UpdateGitHubLoginStatus() { + if (IsDisposed) return; + + long generation = Interlocked.Increment(ref _statusGeneration); if (GitHubAuthService.IsAuthenticated()) { - try { await GenerateLogoutState(); } + try { await GenerateLogoutState(generation); } + catch (OperationCanceledException) when (_lifetimeToken.IsCancellationRequested) + { + return; + } catch (Exception ex) { - Logger.Error("An error occurred while attempting to generate settings login UI:"); - Logger.Error(ex); - GenerateLoginState(); + if (!IsDisposed) + { + Logger.Error("An error occurred while attempting to generate settings login UI:"); + Logger.Error(ex); + } + GenerateLoginState(generation); } } else { - GenerateLoginState(); + GenerateLoginState(generation); } - UpdateCloudControlsEnabled(); + + if (CanApplyStatus(generation)) + UpdateCloudControlsEnabled(); } - private void GenerateLoginState() + private void GenerateLoginState(long generation) { + if (!CanApplyStatus(generation)) return; + IsLoggedIn = false; GitHubUserTitle = CoreTools.Translate("Current status: Not logged in"); GitHubUserSubtitle = CoreTools.Translate("Log in to enable cloud backup"); - GitHubAvatarBitmap = null; + SetGitHubAvatarBitmap(null); } - private async Task GenerateLogoutState() + private async Task GenerateLogoutState(long generation) { using var client = GitHubAuthService.CreateGitHubClient() ?? throw new InvalidOperationException("Authenticated but cannot create GitHub client."); var user = await client.GetCurrentUserAsync(); + if (!CanApplyStatus(generation)) return; IsLoggedIn = true; string displayName = string.IsNullOrWhiteSpace(user.Name) ? user.Login : user.Name; @@ -170,20 +205,36 @@ private async Task GenerateLogoutState() using var http = new HttpClient(CoreTools.GenericHttpClientParameters); if (!string.IsNullOrWhiteSpace(user.AvatarUrl)) { - var bytes = await http.GetByteArrayAsync(user.AvatarUrl); + var bytes = await http.GetByteArrayAsync(user.AvatarUrl, _lifetimeToken); + if (!CanApplyStatus(generation)) return; + using var ms = new MemoryStream(bytes); - GitHubAvatarBitmap = new Bitmap(ms); + var bitmap = new Bitmap(ms); + if (CanApplyStatus(generation)) + SetGitHubAvatarBitmap(bitmap); + else + bitmap.Dispose(); } } + catch (OperationCanceledException) when (_lifetimeToken.IsCancellationRequested) + { + } catch (Exception ex) { - Logger.Error("Failed to load GitHub avatar:"); - Logger.Error(ex); + if (!IsDisposed) + { + Logger.Error("Failed to load GitHub avatar:"); + Logger.Error(ex); + } } } + private void SetGitHubAvatarBitmap(IImage? bitmap) => GitHubAvatarBitmap = bitmap; + private void UpdateCloudControlsEnabled() { + if (IsDisposed) return; + IsLoginButtonEnabled = !_isLoading; IsCloudControlsEnabled = IsLoggedIn && !_isLoading; IsCloudBackupNowEnabled = IsLoggedIn && !_isLoading @@ -193,6 +244,7 @@ private void UpdateCloudControlsEnabled() [RelayCommand] private void EnableCloudBackupChanged() { + if (IsDisposed) return; RestartRequired?.Invoke(this, EventArgs.Empty); UpdateCloudControlsEnabled(); } @@ -200,13 +252,15 @@ private void EnableCloudBackupChanged() [RelayCommand] private async Task Login() { + if (IsDisposed) return; _isLoading = true; UpdateCloudControlsEnabled(); bool success = await _authService.SignInAsync(); - if (!success) + if (!success && !IsDisposed) Logger.Error("An error occurred while logging in to GitHub."); + if (IsDisposed) return; _isLoading = false; UpdateCloudControlsEnabled(); } @@ -214,6 +268,7 @@ private async Task Login() [RelayCommand] private void Logout() { + if (IsDisposed) return; _isLoading = true; UpdateCloudControlsEnabled(); @@ -226,6 +281,7 @@ private void Logout() [RelayCommand] private async Task DoCloudBackup() { + if (IsDisposed) return; _isLoading = true; UpdateCloudControlsEnabled(); try { await DoCloudBackupStatic(); } @@ -338,4 +394,14 @@ private static void MoreDetails() { CoreTools.Launch("https://devolutions.net/unigetui"); } + + public void Dispose() + { + if (Interlocked.Exchange(ref _isDisposed, 1) != 0) return; + + GitHubAuthService.AuthStatusChanged -= OnAuthStatusChanged; + // In-flight HTTP work can still observe this token. Cancelling is sufficient; + // disposing the source here could race with that work. + _lifetimeCancellation.Cancel(); + } } diff --git a/src/UniGetUI.Avalonia/Views/Controls/UserAvatarControl.axaml.cs b/src/UniGetUI.Avalonia/Views/Controls/UserAvatarControl.axaml.cs index 3ca931c336..83dc8fa7db 100644 --- a/src/UniGetUI.Avalonia/Views/Controls/UserAvatarControl.axaml.cs +++ b/src/UniGetUI.Avalonia/Views/Controls/UserAvatarControl.axaml.cs @@ -1,3 +1,4 @@ +using Avalonia; using Avalonia.Controls; using UniGetUI.Avalonia.ViewModels.Controls; @@ -5,9 +6,20 @@ namespace UniGetUI.Avalonia.Views.Controls; public partial class UserAvatarControl : UserControl { + private readonly UserAvatarViewModel _viewModel; + public UserAvatarControl() { - DataContext = new UserAvatarViewModel(); + _viewModel = new UserAvatarViewModel(); + DataContext = _viewModel; InitializeComponent(); } + + protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e) + { + // This control belongs to a SidebarView data template, which Avalonia can recreate as + // the navigation layout changes. Its visual lifetime—not page navigation—is the owner. + _viewModel.Dispose(); + base.OnDetachedFromVisualTree(e); + } } diff --git a/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/Backup.axaml.cs b/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/Backup.axaml.cs index be44c59b8e..e6e9a39e8e 100644 --- a/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/Backup.axaml.cs +++ b/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/Backup.axaml.cs @@ -4,8 +4,10 @@ namespace UniGetUI.Avalonia.Views.Pages.SettingsPages; -public sealed partial class Backup : UserControl, ISettingsPage +public sealed partial class Backup : UserControl, ISettingsPage, IDisposable { + private readonly BackupViewModel _viewModel; + public bool CanGoBack => true; public string ShortTitle => CoreTools.Translate("Backup and Restore"); @@ -14,9 +16,18 @@ public event EventHandler? NavigationRequested { add { } remove { } } public Backup() { - DataContext = new BackupViewModel(); + _viewModel = new BackupViewModel(); + DataContext = _viewModel; InitializeComponent(); - ((BackupViewModel)DataContext).RestartRequired += (s, e) => RestartRequired?.Invoke(s, e); + _viewModel.RestartRequired += OnRestartRequired; + } + + private void OnRestartRequired(object? sender, EventArgs e) => RestartRequired?.Invoke(sender, e); + + public void Dispose() + { + _viewModel.RestartRequired -= OnRestartRequired; + _viewModel.Dispose(); } } diff --git a/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/SettingsBasePage.axaml.cs b/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/SettingsBasePage.axaml.cs index b2a94feaeb..8657356cdb 100644 --- a/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/SettingsBasePage.axaml.cs +++ b/src/UniGetUI.Avalonia/Views/Pages/SettingsPages/SettingsBasePage.axaml.cs @@ -83,8 +83,11 @@ private void NavigateToPage(UserControl page, bool forward = true) private void NavigateBack() { if (_history.Count == 0) return; - var prev = _history.Pop(); - NavigateToPage(prev, forward: false); + + var discardedPage = _currentContent; + var previousPage = _history.Pop(); + NavigateToPage(previousPage, forward: false); + DisposePage(discardedPage); } private void Page_NavigationRequested(object? sender, Type e) @@ -159,12 +162,11 @@ public void GoBack() public void OnEnter() { - _history.Clear(); - NavigateToPage(_isManagers ? GetManagersHomepage() : GetSettingsHomepage()); + ResetToHomepage(); VM.IsRestartBannerVisible = false; } - public void OnLeave() { } + public void OnLeave() => ResetToHomepage(); // ── IInnerNavigationPage extra overloads ────────────────────────────── @@ -186,6 +188,27 @@ public void NavigateTo(Type page) // ── Helpers ─────────────────────────────────────────────────────────── + private void ResetToHomepage() + { + UserControl homepage = _isManagers ? GetManagersHomepage() : GetSettingsHomepage(); + + while (_history.TryPop(out var page)) + if (!ReferenceEquals(page, homepage)) + DisposePage(page); + + if (ReferenceEquals(_currentContent, homepage)) return; + + var discardedPage = _currentContent; + NavigateToPage(homepage); + DisposePage(discardedPage); + } + + private static void DisposePage(UserControl? page) + { + if (page is IDisposable disposable) + disposable.Dispose(); + } + private MainWindowViewModel? GetMainWindowViewModel() => (TopLevel.GetTopLevel(this) is Window { DataContext: MainWindowViewModel vm }) ? vm : null; } diff --git a/src/UniGetUI.Core.Classes.Tests/TaskRecyclerTests.cs b/src/UniGetUI.Core.Classes.Tests/TaskRecyclerTests.cs index 1d55397162..0fd14e7a87 100644 --- a/src/UniGetUI.Core.Classes.Tests/TaskRecyclerTests.cs +++ b/src/UniGetUI.Core.Classes.Tests/TaskRecyclerTests.cs @@ -129,6 +129,27 @@ public async Task TestTaskRecycler_StaticWithArgument_Int() Assert.NotEqual(result3, result5); } + [Fact] + public async Task FaultedTaskIsEvictedBeforeTheNextCall() + { + int calls = 0; + Func method = () => + { + if (Interlocked.Increment(ref calls) == 1) + throw new InvalidOperationException("Expected first attempt failure"); + return 42; + }; + + await Assert.ThrowsAsync( + async () => await TaskRecycler.RunOrAttachAsync(method) + ); + + int result = await TaskRecycler.RunOrAttachAsync(method); + + Assert.Equal(42, result); + Assert.Equal(2, calls); + } + [Fact] public async Task TestTaskRecycler_Class_String() { diff --git a/src/UniGetUI.Core.Classes/TaskRecycler.cs b/src/UniGetUI.Core.Classes/TaskRecycler.cs index 53ae2aaba0..9b87feaf5a 100644 --- a/src/UniGetUI.Core.Classes/TaskRecycler.cs +++ b/src/UniGetUI.Core.Classes/TaskRecycler.cs @@ -21,22 +21,18 @@ public static class TaskRecycler private static readonly ConcurrentDictionary> _tasks = new(); private static readonly ConcurrentDictionary _tasks_VOID = new(); - // --------------------------------------------------------------------------------------------------------------- - public static Task RunOrAttachAsync_VOID(Action method, int cacheTimeSecs = 0) { int hash = method.GetHashCode(); return _runTaskAndWait_VOID(new Task(method), hash, cacheTimeSecs); } - /// Asynchronous entry point for 0 parameters public static Task RunOrAttachAsync(Func method, int cacheTimeSecs = 0) { int hash = method.GetHashCode(); return _runTaskAndWait(new Task(method), hash, cacheTimeSecs); } - /// Asynchronous entry point for 1 parameter public static Task RunOrAttachAsync( Func method, ParamT arg1, @@ -47,7 +43,6 @@ public static Task RunOrAttachAsync( return _runTaskAndWait(new Task(() => method(arg1)), hash, cacheTimeSecs); } - /// Asynchronous entry point for 2 parameters public static Task RunOrAttachAsync( Func method, Param1T arg1, @@ -59,7 +54,6 @@ public static Task RunOrAttachAsync( return _runTaskAndWait(new Task(() => method(arg1, arg2)), hash, cacheTimeSecs); } - /// Asynchronous entry point for 3 parameters public static Task RunOrAttachAsync( Func method, Param1T arg1, @@ -80,20 +74,15 @@ public static Task RunOrAttachAsync( ); } - // --------------------------------------------------------------------------------------------------------------- - - /// Synchronous entry point for 0 parameters public static ReturnT RunOrAttach(Func method, int cacheTimeSecs = 0) => RunOrAttachAsync(method, cacheTimeSecs).GetAwaiter().GetResult(); - /// Synchronous entry point for 1 parameter1 public static ReturnT RunOrAttach( Func method, ParamT arg1, int cacheTimeSecs = 0 ) => RunOrAttachAsync(method, arg1, cacheTimeSecs).GetAwaiter().GetResult(); - /// Synchronous entry point for 2 parameters public static ReturnT RunOrAttach( Func method, Param1T arg1, @@ -101,7 +90,6 @@ public static ReturnT RunOrAttach( int cacheTimeSecs = 0 ) => RunOrAttachAsync(method, arg1, arg2, cacheTimeSecs).GetAwaiter().GetResult(); - /// Synchronous entry point for 3 parameters public static ReturnT RunOrAttach( Func method, Param1T arg1, @@ -110,104 +98,80 @@ public static ReturnT RunOrAttach( int cacheTimeSecs = 0 ) => RunOrAttachAsync(method, arg1, arg2, arg3, cacheTimeSecs).GetAwaiter().GetResult(); - // --------------------------------------------------------------------------------------------------------------- - /// /// Instantly removes a function call from the cache, even if the associated task has not - /// finished yet. Any previous call will finish as expected. New calls won't attach to any + /// finished yet. Any previous call will finish as expected. New calls will not attach to any /// preexisting Tasks, and a new Task will be created instead. - /// If the given function call is not present in the cache, nothing will be done. /// - /// public static void RemoveFromCache(Func method) => - _removeFromCache(method.GetHashCode(), 0); + _tasks.TryRemove(method.GetHashCode(), out _); - // --------------------------------------------------------------------------------------------------------------- - - /// - /// Handles running the task if no such task was found on cache, and returning the cached task if it was found. - /// - private static async Task _runTaskAndWait_VOID(Task task, int hash, int cacheTimeSecsSecs) + private static Task _runTaskAndWait_VOID(Task task, int hash, int cacheTimeSecs) { - if (_tasks_VOID.TryGetValue(hash, out Task? _task)) + Task cachedTask = _tasks_VOID.GetOrAdd(hash, task); + if (ReferenceEquals(cachedTask, task)) { - // Get the cached task, which is either running or finished - task = _task; - } - else if (!_tasks_VOID.TryAdd(hash, task)) - { - // Race condition, an equivalent task got added from another thread between the TryGetValue and TryAdd, - // so we are going to restart the call to _runTaskAndWait in order for TryGetValue to return the new task again - await _runTaskAndWait_VOID(task, hash, cacheTimeSecsSecs).ConfigureAwait(false); - return; + task.Start(); + _ = _scheduleCacheRemoval_VOID(hash, task, cacheTimeSecs); } else { - // Now that the new task is in the cache, run the task. - task.Start(); + Interlocked.Increment(ref TaskRecyclerTelemetry.DeduplicatedCalls); } - // Wait for the task to finish - await task.ConfigureAwait(false); - - // Schedule the task for removal after the cache time expires - _removeFromCache_VOID(hash, cacheTimeSecsSecs); + return cachedTask; } - /// - /// Handles running the task if no such task was found on cache, and returning the cached task if it was found. - /// - private static async Task _runTaskAndWait( - Task task, - int hash, - int cacheTimeSecsSecs - ) + private static Task _runTaskAndWait(Task task, int hash, int cacheTimeSecs) { - if (_tasks.TryGetValue(hash, out Task? _task)) - { - // Get the cached task, which is either running or finished - task = _task; - } - else if (!_tasks.TryAdd(hash, task)) + Task cachedTask = _tasks.GetOrAdd(hash, task); + if (ReferenceEquals(cachedTask, task)) { - // Race condition, an equivalent task got added from another thread between the TryGetValue and TryAdd, - // so we are going to restart the call to _runTaskAndWait in order for TryGetValue to return the new task again - return await _runTaskAndWait(task, hash, cacheTimeSecsSecs).ConfigureAwait(false); + task.Start(); + _ = _scheduleCacheRemoval(hash, task, cacheTimeSecs); } else { - // Now that the new task is in the cache, run the task. - task.Start(); + Interlocked.Increment(ref TaskRecyclerTelemetry.DeduplicatedCalls); } - // Wait for the task to finish - ReturnT result = await task.ConfigureAwait(false); - - // Schedule the task for removal after the cache time expires - _removeFromCache(hash, cacheTimeSecsSecs); - - return result; + return cachedTask; } - /// - /// Removes the task associated with the given hash from the cache after the given period of time - /// If the given hash is not present, nothing will be done. - /// - private static async void _removeFromCache(int hash, int cacheTimeSecsSecs) + private static Task _scheduleCacheRemoval(int hash, Task task, int cacheTimeSecs) => + task.ContinueWith( + completedTask => _removeFromCache(hash, completedTask, cacheTimeSecs), + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default + ) + .Unwrap(); + + private static Task _scheduleCacheRemoval_VOID(int hash, Task task, int cacheTimeSecs) => + task.ContinueWith( + completedTask => _removeFromCache_VOID(hash, completedTask, cacheTimeSecs), + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default + ) + .Unwrap(); + + private static async Task _removeFromCache(int hash, Task task, int cacheTimeSecs) { - if (cacheTimeSecsSecs > 0) - await Task.Delay(cacheTimeSecsSecs * 1000); + if (task.IsCompletedSuccessfully && cacheTimeSecs > 0) + await Task.Delay(TimeSpan.FromSeconds(cacheTimeSecs)).ConfigureAwait(false); - _tasks.Remove(hash, out _); + ((ICollection>>)_tasks).Remove(new(hash, task)); } - private static async void _removeFromCache_VOID(int hash, int cacheTimeSecsSecs) + private static async Task _removeFromCache_VOID(int hash, Task task, int cacheTimeSecs) { - if (cacheTimeSecsSecs > 0) - await Task.Delay(cacheTimeSecsSecs * 1000); + if (task.IsCompletedSuccessfully && cacheTimeSecs > 0) + await Task.Delay(TimeSpan.FromSeconds(cacheTimeSecs)).ConfigureAwait(false); - _tasks_VOID.Remove(hash, out _); + ((ICollection>)_tasks_VOID).Remove(new(hash, task)); } + } public static class TaskRecyclerTelemetry diff --git a/src/UniGetUI.Core.IconStore/IconCacheEngine.cs b/src/UniGetUI.Core.IconStore/IconCacheEngine.cs index 85112ce640..2de35b9d34 100644 --- a/src/UniGetUI.Core.IconStore/IconCacheEngine.cs +++ b/src/UniGetUI.Core.IconStore/IconCacheEngine.cs @@ -217,7 +217,8 @@ icon.ValidationMethod is IconValidationMethod.UriSource using HttpClient client = new(CoreTools.GenericHttpClientParameters); client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); - HttpResponseMessage response = client.GetAsync(icon.Url).GetAwaiter().GetResult(); + using var request = new HttpRequestMessage(HttpMethod.Get, icon.Url); + using HttpResponseMessage response = client.Send(request); if (!response.IsSuccessStatusCode) { Logger.Warn( @@ -309,25 +310,27 @@ private static void DownsizeImage(string cachedIconFile) if (width > MAX_SIDE || height > MAX_SIDE) { File.Move(cachedIconFile, $"{cachedIconFile}.copy"); - var image = MagicImageProcessor.BuildPipeline( - $"{cachedIconFile}.copy", - new ProcessImageSettings + using ( + var image = MagicImageProcessor.BuildPipeline( + $"{cachedIconFile}.copy", + new ProcessImageSettings + { + Width = MAX_SIDE, + Height = MAX_SIDE, + ResizeMode = CropScaleMode.Contain, + } + ) + ) + { + // Apply changes and save the image to disk + using (FileStream fileStream = File.Create(cachedIconFile)) { - Width = MAX_SIDE, - Height = MAX_SIDE, - ResizeMode = CropScaleMode.Contain, + image.WriteOutput(fileStream); } - ); - - // Apply changes and save the image to disk - using (FileStream fileStream = File.Create(cachedIconFile)) - { - image.WriteOutput(fileStream); + Logger.Debug( + $"File {cachedIconFile} was downsized from {width}x{height} to {image.Settings.Width}x{image.Settings.Height}" + ); } - Logger.Debug( - $"File {cachedIconFile} was downsized from {width}x{height} to {image.Settings.Width}x{image.Settings.Height}" - ); - image.Dispose(); File.Delete($"{cachedIconFile}.copy"); } else diff --git a/src/UniGetUI.Core.Tools/Tools.cs b/src/UniGetUI.Core.Tools/Tools.cs index 43f6cc4c61..05ac720f70 100644 --- a/src/UniGetUI.Core.Tools/Tools.cs +++ b/src/UniGetUI.Core.Tools/Tools.cs @@ -149,7 +149,7 @@ public static void ScheduleRelaunchAfterExit(string? executablePath = null) string command = $"Wait-Process -Id {currentProcessId}; Start-Process -FilePath '{escapedExecutablePath}'"; - Process.Start( + using var process = Process.Start( new ProcessStartInfo { FileName = "powershell.exe", @@ -356,9 +356,8 @@ public static long GetFileSizeAsLong(Uri? url) try { using HttpClient client = new(CoreTools.GenericHttpClientParameters); - HttpResponseMessage response = client.Send( - new HttpRequestMessage(HttpMethod.Head, url) - ); + using var request = new HttpRequestMessage(HttpMethod.Head, url); + using HttpResponseMessage response = client.Send(request); return response.Content.Headers.ContentLength ?? 0; } catch (Exception e) @@ -377,9 +376,8 @@ public static string GetFileName(Uri url) var handler = CoreTools.GenericHttpClientParameters; handler.AllowAutoRedirect = false; using HttpClient client = new(handler); - HttpResponseMessage response = client.Send( - new HttpRequestMessage(HttpMethod.Head, url) - ); + using var request = new HttpRequestMessage(HttpMethod.Head, url); + using HttpResponseMessage response = client.Send(request); if ( response.StatusCode @@ -935,7 +933,7 @@ public static async Task ShowFileOnExplorer(string path) return; } - Process p = new() + using Process p = new() { StartInfo = new() { @@ -947,7 +945,6 @@ public static async Task ShowFileOnExplorer(string path) }; p.Start(); await p.WaitForExitAsync(); - p.Dispose(); } catch (Exception ex) { @@ -962,7 +959,7 @@ public static void Launch(string? path) if (path is null) return; - var p = new Process() + using var p = new Process() { StartInfo = new() { FileName = path, UseShellExecute = true, CreateNoWindow = true, }, }; diff --git a/src/UniGetUI.Interface.Telemetry.Tests/TelemetryHandlerTests.cs b/src/UniGetUI.Interface.Telemetry.Tests/TelemetryHandlerTests.cs index 4b70fcfabb..0d041c25b8 100644 --- a/src/UniGetUI.Interface.Telemetry.Tests/TelemetryHandlerTests.cs +++ b/src/UniGetUI.Interface.Telemetry.Tests/TelemetryHandlerTests.cs @@ -89,6 +89,33 @@ public async Task InitializeAsync_WithoutCredentials_DoesNotSendAndLogsOnce() Assert.Equal(warningCountBefore + 1, warningCountAfter); } + [Fact] + public async Task Posts_DisposeResponsesReturnedByInjectionSeam() + { + TelemetryHandler.Configure("telemetry-user", "telemetry-pass"); + var activityResponse = new TrackingHttpResponseMessage(HttpStatusCode.OK); + TelemetryHandler.TestSendAsyncOverride = _ => Task.FromResult(activityResponse); + + await TelemetryHandler.InitializeAsync(); + + Assert.True(activityResponse.IsDisposed); + + var packageResponse = new TrackingHttpResponseMessage(HttpStatusCode.BadRequest); + TelemetryHandler.TestSendAsyncOverride = _ => Task.FromResult(packageResponse); + var package = new Package( + "Telemetry Package", + "Telemetry.Package", + "1.0.0", + new NullSource("Telemetry Source"), + NullPackageManager.Instance + ); + TelemetryHandler.UpdatePackage(package, TEL_OP_RESULT.SUCCESS); + + await TelemetryHandler.FlushPackageEventsAsync(); + + Assert.True(packageResponse.IsDisposed); + } + [Fact] public void ComputeActiveSettingsBitmask_IncludesDeterministicSettingsAndSpecialPaths() { @@ -353,6 +380,18 @@ private static void ClearDictionaryField(string fieldName) dictionary.GetType().GetMethod("Clear")!.Invoke(dictionary, null); } + private sealed class TrackingHttpResponseMessage(HttpStatusCode statusCode) + : HttpResponseMessage(statusCode) + { + public bool IsDisposed { get; private set; } + + protected override void Dispose(bool disposing) + { + IsDisposed = true; + base.Dispose(disposing); + } + } + private sealed record CapturedRequest( Uri RequestUri, string Body, diff --git a/src/UniGetUI.Interface.Telemetry/TelemetryHandler.cs b/src/UniGetUI.Interface.Telemetry/TelemetryHandler.cs index 8d85cbf806..e0ab51b55a 100644 --- a/src/UniGetUI.Interface.Telemetry/TelemetryHandler.cs +++ b/src/UniGetUI.Interface.Telemetry/TelemetryHandler.cs @@ -280,7 +280,7 @@ private static async Task PostBulkPackageEventsAsync(IReadOnlyList(string indexName, T eventData }; request.Headers.Authorization = new AuthenticationHeaderValue("Basic", credentials); - HttpResponseMessage response = TestSendAsyncOverride is { } sendAsync + using HttpResponseMessage response = TestSendAsyncOverride is { } sendAsync ? await sendAsync(request) : await _httpClient.SendAsync(request); diff --git a/src/UniGetUI.PackageEngine.Managers.Bun/Bun.cs b/src/UniGetUI.PackageEngine.Managers.Bun/Bun.cs index 24204326f6..a75a521dd0 100644 --- a/src/UniGetUI.PackageEngine.Managers.Bun/Bun.cs +++ b/src/UniGetUI.PackageEngine.Managers.Bun/Bun.cs @@ -222,7 +222,7 @@ protected override void _loadManagerExecutableFile(out bool found, out string pa protected override void _loadManagerVersion(out string version) { - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.Cargo/CratesIOClient.cs b/src/UniGetUI.PackageEngine.Managers.Cargo/CratesIOClient.cs index a03068fae5..3304ee42b1 100644 --- a/src/UniGetUI.PackageEngine.Managers.Cargo/CratesIOClient.cs +++ b/src/UniGetUI.PackageEngine.Managers.Cargo/CratesIOClient.cs @@ -91,10 +91,13 @@ public static CargoManifestVersion GetManifestVersion(string packageId, string v internal static T Fetch(Uri url) { - HttpClient client = new(CoreTools.GenericHttpClientParameters); + using HttpClient client = new(CoreTools.GenericHttpClientParameters); client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); + using var request = new HttpRequestMessage(HttpMethod.Get, url); + using HttpResponseMessage response = client.Send(request); + response.EnsureSuccessStatusCode(); - var manifestStr = client.GetStringAsync(url).GetAwaiter().GetResult(); + string manifestStr = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); var manifest = CargoJson.Deserialize(manifestStr) diff --git a/src/UniGetUI.PackageEngine.Managers.Chocolatey/Chocolatey.cs b/src/UniGetUI.PackageEngine.Managers.Chocolatey/Chocolatey.cs index 041cfcbb77..7c19d4ebc5 100644 --- a/src/UniGetUI.PackageEngine.Managers.Chocolatey/Chocolatey.cs +++ b/src/UniGetUI.PackageEngine.Managers.Chocolatey/Chocolatey.cs @@ -369,7 +369,7 @@ out string callArguments protected override void _loadManagerVersion(out string version) { - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGet.cs b/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGet.cs index 171ad30071..f64233fb4e 100644 --- a/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGet.cs +++ b/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGet.cs @@ -106,10 +106,8 @@ protected sealed override IReadOnlyList FindPackages_UnSafe(string quer while (SearchUrl is not null) { - HttpResponseMessage response = client - .GetAsync(SearchUrl) - .GetAwaiter() - .GetResult(); + using var request = new HttpRequestMessage(HttpMethod.Get, SearchUrl); + using HttpResponseMessage response = client.Send(request); if (!response.IsSuccessStatusCode) { @@ -246,10 +244,8 @@ protected override IReadOnlyList GetAvailableUpdates_UnSafe() using HttpClient client = new(CoreTools.GenericHttpClientParameters); client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); - HttpResponseMessage response = client - .GetAsync(SearchUrl) - .GetAwaiter() - .GetResult(); + using var request = new HttpRequestMessage(HttpMethod.Get, SearchUrl); + using HttpResponseMessage response = client.Send(request); if (!response.IsSuccessStatusCode) { diff --git a/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGetDetailsHelper.cs b/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGetDetailsHelper.cs index c10ebfa529..b478ba02ef 100644 --- a/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGetDetailsHelper.cs +++ b/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/BaseNuGetDetailsHelper.cs @@ -266,10 +266,10 @@ protected override IReadOnlyList GetInstallableVersions_UnSafe(IPackage List results = []; - HttpClient client = new(CoreTools.GenericHttpClientParameters); + using HttpClient client = new(CoreTools.GenericHttpClientParameters); client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); - - HttpResponseMessage response = client.GetAsync(SearchUrl).GetAwaiter().GetResult(); + using var request = new HttpRequestMessage(HttpMethod.Get, SearchUrl); + using HttpResponseMessage response = client.Send(request); if (!response.IsSuccessStatusCode) { Logger.Warn( diff --git a/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/Internal/NuGetManifestLoader.cs b/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/Internal/NuGetManifestLoader.cs index 9e3e9e6c32..d811a3aae6 100644 --- a/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/Internal/NuGetManifestLoader.cs +++ b/src/UniGetUI.PackageEngine.Managers.Generic.NuGet/Internal/NuGetManifestLoader.cs @@ -45,7 +45,6 @@ public static Uri GetNuPkgUrl(IPackage package) return manifest; } - string? PackageManifestContent = ""; string PackageManifestUrl = GetManifestUrl(package).ToString(); try @@ -53,33 +52,24 @@ public static Uri GetNuPkgUrl(IPackage package) using (HttpClient client = new(CoreTools.GenericHttpClientParameters)) { client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); - HttpResponseMessage response = client - .GetAsync(PackageManifestUrl) - .GetAwaiter() - .GetResult(); - if (!response.IsSuccessStatusCode && package.VersionString.EndsWith(".0")) - { - response = client - .GetAsync(new Uri(PackageManifestUrl.ToString().Replace(".0')", "')"))) - .GetAwaiter() - .GetResult(); - } + using var initialRequest = new HttpRequestMessage( + HttpMethod.Get, + PackageManifestUrl + ); + using HttpResponseMessage initialResponse = client.Send(initialRequest); - if (!response.IsSuccessStatusCode) + if (!initialResponse.IsSuccessStatusCode && package.VersionString.EndsWith(".0")) { - Logger.Warn( - $"Failed to download the {package.Manager.Name} manifest at Url={PackageManifestUrl.ToString()} with status code {response.StatusCode}" + using var fallbackRequest = new HttpRequestMessage( + HttpMethod.Get, + new Uri(PackageManifestUrl.ToString().Replace(".0')", "')")) ); - return null; + using HttpResponseMessage fallbackResponse = client.Send(fallbackRequest); + return CacheManifestContent(package, PackageManifestUrl, fallbackResponse); } - PackageManifestContent = response - .Content.ReadAsStringAsync() - .GetAwaiter() - .GetResult(); + return CacheManifestContent(package, PackageManifestUrl, initialResponse); } - BaseNuGet.Manifests[package.GetHash()] = PackageManifestContent; - return PackageManifestContent; } catch (Exception e) { @@ -90,5 +80,24 @@ public static Uri GetNuPkgUrl(IPackage package) return null; } } + + private static string? CacheManifestContent( + IPackage package, + string packageManifestUrl, + HttpResponseMessage response + ) + { + if (!response.IsSuccessStatusCode) + { + Logger.Warn( + $"Failed to download the {package.Manager.Name} manifest at Url={packageManifestUrl} with status code {response.StatusCode}" + ); + return null; + } + + string packageManifestContent = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); + BaseNuGet.Manifests[package.GetHash()] = packageManifestContent; + return packageManifestContent; + } } } diff --git a/src/UniGetUI.PackageEngine.Managers.Npm/Npm.cs b/src/UniGetUI.PackageEngine.Managers.Npm/Npm.cs index b11980316f..11430c0c6d 100644 --- a/src/UniGetUI.PackageEngine.Managers.Npm/Npm.cs +++ b/src/UniGetUI.PackageEngine.Managers.Npm/Npm.cs @@ -208,7 +208,7 @@ out string callArguments protected override void _loadManagerVersion(out string version) { - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.Pip/Helpers/PipPkgDetailsHelper.cs b/src/UniGetUI.PackageEngine.Managers.Pip/Helpers/PipPkgDetailsHelper.cs index fa8dd9da06..70b753a66a 100644 --- a/src/UniGetUI.PackageEngine.Managers.Pip/Helpers/PipPkgDetailsHelper.cs +++ b/src/UniGetUI.PackageEngine.Managers.Pip/Helpers/PipPkgDetailsHelper.cs @@ -21,13 +21,15 @@ protected override void GetDetails_UnSafe(IPackageDetails details) LoggableTaskType.LoadPackageDetails ); - string JsonString; - HttpClient client = new(CoreTools.GenericHttpClientParameters); + using HttpClient client = new(CoreTools.GenericHttpClientParameters); client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); - JsonString = client - .GetStringAsync($"https://pypi.org/pypi/{details.Package.Id}/json") - .GetAwaiter() - .GetResult(); + using var request = new HttpRequestMessage( + HttpMethod.Get, + $"https://pypi.org/pypi/{details.Package.Id}/json" + ); + using HttpResponseMessage response = client.Send(request); + response.EnsureSuccessStatusCode(); + string JsonString = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); JsonObject? contents = JsonNode.Parse(JsonString) as JsonObject; diff --git a/src/UniGetUI.PackageEngine.Managers.Pip/Pip.cs b/src/UniGetUI.PackageEngine.Managers.Pip/Pip.cs index 69a2b947d0..a1aac9e582 100644 --- a/src/UniGetUI.PackageEngine.Managers.Pip/Pip.cs +++ b/src/UniGetUI.PackageEngine.Managers.Pip/Pip.cs @@ -195,7 +195,8 @@ private static string[] GetOrRefreshIndex(INativeTaskLogger logger) client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); client.DefaultRequestHeaders.Add("Accept", "application/vnd.pypi.simple.v1+json"); - HttpResponseMessage response = client.GetAsync("https://pypi.org/simple/").GetAwaiter().GetResult(); + using var request = new HttpRequestMessage(HttpMethod.Get, "https://pypi.org/simple/"); + using HttpResponseMessage response = client.Send(request); if (!response.IsSuccessStatusCode) throw new HttpRequestException($"PyPI simple index returned {(int)response.StatusCode} {response.ReasonPhrase}"); @@ -251,9 +252,15 @@ internal static string[] SelectSearchMatches(string query, IEnumerable a await _versionFetchSemaphore.WaitAsync().ConfigureAwait(false); try { - string json = await _httpClient - .GetStringAsync($"https://pypi.org/pypi/{Uri.EscapeDataString(packageName)}/json") + using var request = new HttpRequestMessage( + HttpMethod.Get, + $"https://pypi.org/pypi/{Uri.EscapeDataString(packageName)}/json" + ); + using HttpResponseMessage response = await _httpClient + .SendAsync(request) .ConfigureAwait(false); + response.EnsureSuccessStatusCode(); + string json = await response.Content.ReadAsStringAsync().ConfigureAwait(false); return (JsonNode.Parse(json) as JsonObject)?["info"]?["version"]?.GetValue(); } catch @@ -489,7 +496,7 @@ out string callArguments protected override void _loadManagerVersion(out string version) { - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.PowerShell/PowerShell.cs b/src/UniGetUI.PackageEngine.Managers.PowerShell/PowerShell.cs index 14632242cf..e81a37f435 100644 --- a/src/UniGetUI.PackageEngine.Managers.PowerShell/PowerShell.cs +++ b/src/UniGetUI.PackageEngine.Managers.PowerShell/PowerShell.cs @@ -138,7 +138,7 @@ out string callArguments protected override void _loadManagerVersion(out string version) { - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.PowerShell7/PowerShell7.cs b/src/UniGetUI.PackageEngine.Managers.PowerShell7/PowerShell7.cs index 084561e825..85844ca1c5 100644 --- a/src/UniGetUI.PackageEngine.Managers.PowerShell7/PowerShell7.cs +++ b/src/UniGetUI.PackageEngine.Managers.PowerShell7/PowerShell7.cs @@ -181,7 +181,7 @@ out string callArguments protected override void _loadManagerVersion(out string version) { - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs b/src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs index d42384455d..b4bc191e95 100644 --- a/src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs +++ b/src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs @@ -374,7 +374,7 @@ protected override IReadOnlyList FindPackages_UnSafe(string query) var (found, path) = CoreTools.Which("scoop-search.exe"); if (!found) { - Process proc = new() + using Process proc = new() { StartInfo = new ProcessStartInfo { @@ -553,7 +553,7 @@ out string callArguments protected override void _loadManagerVersion(out string version) { - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.Vcpkg/Helpers/VcpkgPkgDetailsHelper.cs b/src/UniGetUI.PackageEngine.Managers.Vcpkg/Helpers/VcpkgPkgDetailsHelper.cs index 8662b129df..566b1c0a07 100644 --- a/src/UniGetUI.PackageEngine.Managers.Vcpkg/Helpers/VcpkgPkgDetailsHelper.cs +++ b/src/UniGetUI.PackageEngine.Managers.Vcpkg/Helpers/VcpkgPkgDetailsHelper.cs @@ -27,15 +27,15 @@ protected override void GetDetails_UnSafe(IPackageDetails details) string PackagePrefix = details.Package.Id.Split(":")[0]; string PackageName = PackagePrefix.Split("[")[0]; - string JsonString; - HttpClient client = new(CoreTools.GenericHttpClientParameters); + using HttpClient client = new(CoreTools.GenericHttpClientParameters); client.DefaultRequestHeaders.UserAgent.ParseAdd(CoreData.UserAgentString); - JsonString = client - .GetStringAsync( - $"https://raw.githubusercontent.com/{VCPKG_REPO}/refs/heads/{VCPKG_PORT_PATH}/{PackageName}/{VCPKG_PORT_FILE}" - ) - .GetAwaiter() - .GetResult(); + using var request = new HttpRequestMessage( + HttpMethod.Get, + $"https://raw.githubusercontent.com/{VCPKG_REPO}/refs/heads/{VCPKG_PORT_PATH}/{PackageName}/{VCPKG_PORT_FILE}" + ); + using HttpResponseMessage response = client.Send(request); + response.EnsureSuccessStatusCode(); + string JsonString = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); JsonObject? contents = JsonNode.Parse(JsonString) as JsonObject; diff --git a/src/UniGetUI.PackageEngine.Managers.Vcpkg/Vcpkg.cs b/src/UniGetUI.PackageEngine.Managers.Vcpkg/Vcpkg.cs index 7a051aba50..1ef3e63b07 100644 --- a/src/UniGetUI.PackageEngine.Managers.Vcpkg/Vcpkg.cs +++ b/src/UniGetUI.PackageEngine.Managers.Vcpkg/Vcpkg.cs @@ -366,7 +366,7 @@ protected override void _loadManagerVersion(out string version) { var (_, rootPath) = GetVcpkgRoot(); - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Managers.WinGet/ClientHelpers/WinGetIconsHelper.cs b/src/UniGetUI.PackageEngine.Managers.WinGet/ClientHelpers/WinGetIconsHelper.cs index 7f0ee0eb49..d6769b6960 100644 --- a/src/UniGetUI.PackageEngine.Managers.WinGet/ClientHelpers/WinGetIconsHelper.cs +++ b/src/UniGetUI.PackageEngine.Managers.WinGet/ClientHelpers/WinGetIconsHelper.cs @@ -38,7 +38,7 @@ internal static class WinGetIconsHelper using (StreamWriter streamWriter = new(httpRequest.GetRequestStream())) streamWriter.Write(data); - var httpResponse = httpRequest.GetResponse() as HttpWebResponse; + using var httpResponse = httpRequest.GetResponse() as HttpWebResponse; if (httpResponse is null) { Logger.Warn($"Null MS Store response for uri={url} and data={data}"); diff --git a/src/UniGetUI.PackageEngine.Managers.WinGet/WinGet.cs b/src/UniGetUI.PackageEngine.Managers.WinGet/WinGet.cs index f75b610f44..6d306affc8 100644 --- a/src/UniGetUI.PackageEngine.Managers.WinGet/WinGet.cs +++ b/src/UniGetUI.PackageEngine.Managers.WinGet/WinGet.cs @@ -589,7 +589,7 @@ protected override void _loadManagerVersion(out string version) bool usesCliHelper = WinGetHelper.Instance is WinGetCliHelper; bool usesPingetHelper = WinGetHelper.Instance is PingetCliHelper; - Process process = new() + using Process process = new() { StartInfo = new ProcessStartInfo { diff --git a/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs b/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs index 24bdf78b0d..81e0d97292 100644 --- a/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs +++ b/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs @@ -23,6 +23,11 @@ public abstract partial class AbstractOperation : IDisposable protected bool QUEUE_ENABLED; protected bool FORCE_HOLD_QUEUE; private bool IsInnerOperation; + private readonly object CancellationLock = new(); + private CancellationTokenSource? RunCancellationSource; + private AbstractOperation? ActiveInnerOperation; + private volatile bool IsExecutingOperation; + private bool Disposed; private readonly List<(string, LineType)> LogList = []; private OperationStatus _status = OperationStatus.InQueue; @@ -73,28 +78,84 @@ public AbstractOperation( public void Cancel() { - switch (_status) + if (_status is OperationStatus.Canceled or OperationStatus.Failed or OperationStatus.Succeeded) + return; + + bool wasRunning = _status is OperationStatus.Running; + bool hasActiveWork = IsExecutingOperation; + Status = OperationStatus.Canceled; + CancellationTokenSource? cancellationSource; + lock (CancellationLock) + cancellationSource = RunCancellationSource; + cancellationSource?.Cancel(); + AbstractOperation? activeInnerOperation; + lock (CancellationLock) + activeInnerOperation = ActiveInnerOperation; + activeInnerOperation?.Cancel(); + + if (wasRunning) + CancelRequested?.Invoke(this, EventArgs.Empty); + + // A queued operation has no active work to clean up. A running operation stays on the + // queue until MainThread has awaited its task and completed its cleanup. + if (!hasActiveWork) { - case OperationStatus.Canceled: - break; - case OperationStatus.Failed: - break; - case OperationStatus.Running: - Status = OperationStatus.Canceled; - while (OperationQueue.Remove(this)) - ; - CancelRequested?.Invoke(this, EventArgs.Empty); - Status = OperationStatus.Canceled; - break; - case OperationStatus.InQueue: - Status = OperationStatus.Canceled; - while (OperationQueue.Remove(this)) - ; - Status = OperationStatus.Canceled; - break; - case OperationStatus.Succeeded: - break; + while (OperationQueue.Remove(this)) + ; + } + } + + protected CancellationToken CancellationToken + { + get + { + lock (CancellationLock) + return RunCancellationSource?.Token ?? global::System.Threading.CancellationToken.None; + } + } + + private CancellationTokenSource StartRunCancellation() + { + var cancellationSource = new CancellationTokenSource(); + lock (CancellationLock) + RunCancellationSource = cancellationSource; + return cancellationSource; + } + + private bool TrySetActiveInnerOperation(AbstractOperation operation) + { + bool cancellationRequested; + lock (CancellationLock) + { + cancellationRequested = + RunCancellationSource?.IsCancellationRequested is true + || Status is OperationStatus.Canceled; + if (!cancellationRequested) + ActiveInnerOperation = operation; + } + + if (cancellationRequested) + operation.Cancel(); + + return !cancellationRequested; + } + + private void ClearActiveInnerOperation(AbstractOperation operation) + { + lock (CancellationLock) + if (ReferenceEquals(ActiveInnerOperation, operation)) + ActiveInnerOperation = null; + } + + private void EndRunCancellation(CancellationTokenSource cancellationSource) + { + lock (CancellationLock) + { + if (!ReferenceEquals(RunCancellationSource, cancellationSource)) + return; + RunCancellationSource = null; } + cancellationSource.Dispose(); } protected void Line(string line, LineType type) @@ -116,6 +177,7 @@ protected void Line(string line, LineType type) public async Task MainThread() { + CancellationTokenSource runCancellation = StartRunCancellation(); try { if (Metadata.Status == "") @@ -176,7 +238,16 @@ public async Task MainThread() } // END QUEUE HANDLER - var result = await _runOperation(); + IsExecutingOperation = true; + OperationVeredict result; + try + { + result = await _runOperation(); + } + finally + { + IsExecutingOperation = false; + } while (OperationQueue.Remove(this)) ; @@ -250,8 +321,16 @@ public async Task MainThread() } finally { - if (OperationQueue.Count == 0) - QueueDrained?.Invoke(null, EventArgs.Empty); + try + { + OnRunCompleted(); + } + finally + { + EndRunCancellation(runCancellation); + if (OperationQueue.Count == 0) + QueueDrained?.Invoke(null, EventArgs.Empty); + } } } @@ -266,13 +345,27 @@ private async Task _runOperation() Line("", LineType.VerboseDetails); foreach (var preReq in PreOperations) { + if (Status is OperationStatus.Canceled || CancellationToken.IsCancellationRequested) + return OperationVeredict.Canceled; + i++; Line( CoreTools.Translate("Running PreOperation ({0}/{1})...", i, count), LineType.Information ); preReq.Operation.LogLineAdded += (_, line) => Line(line.Item1, line.Item2); - await preReq.Operation.MainThread(); + if (!TrySetActiveInnerOperation(preReq.Operation)) + return OperationVeredict.Canceled; + try + { + await preReq.Operation.MainThread(); + } + finally + { + ClearActiveInnerOperation(preReq.Operation); + } + if (Status is OperationStatus.Canceled || CancellationToken.IsCancellationRequested) + return OperationVeredict.Canceled; if (preReq.Operation.Status is not OperationStatus.Succeeded && preReq.MustSucceed) { Line( @@ -305,29 +398,26 @@ private async Task _runOperation() do { + if (Status is OperationStatus.Canceled || CancellationToken.IsCancellationRequested) + { + result = OperationVeredict.Canceled; + break; + } + OperationStarting?.Invoke(this, EventArgs.Empty); try { - // Check if the operation was canceled - if (Status is OperationStatus.Canceled) - { - result = OperationVeredict.Canceled; - break; - } - Task op = PerformOperation(); - while (Status != OperationStatus.Canceled && !op.IsCompleted) - await Task.Delay(100); - - if (Status is OperationStatus.Canceled) + result = await op.ConfigureAwait(false); + if (Status is OperationStatus.Canceled || CancellationToken.IsCancellationRequested) result = OperationVeredict.Canceled; - else - result = op.GetAwaiter().GetResult(); } catch (Exception e) { - result = OperationVeredict.Failure; + result = CancellationToken.IsCancellationRequested + ? OperationVeredict.Canceled + : OperationVeredict.Failure; Logger.Error(e); foreach (string l in e.ToString().Split("\n")) { @@ -351,8 +441,22 @@ private async Task _runOperation() CoreTools.Translate("Running PostOperation ({0}/{1})...", i, count), LineType.Information ); + if (Status is OperationStatus.Canceled || CancellationToken.IsCancellationRequested) + return OperationVeredict.Canceled; + postReq.Operation.LogLineAdded += (_, line) => Line(line.Item1, line.Item2); - await postReq.Operation.MainThread(); + if (!TrySetActiveInnerOperation(postReq.Operation)) + return OperationVeredict.Canceled; + try + { + await postReq.Operation.MainThread(); + } + finally + { + ClearActiveInnerOperation(postReq.Operation); + } + if (Status is OperationStatus.Canceled || CancellationToken.IsCancellationRequested) + return OperationVeredict.Canceled; if (postReq.Operation.Status is not OperationStatus.Succeeded && postReq.MustSucceed) { Line( @@ -437,9 +541,25 @@ public void Retry(string retryMode) protected abstract Task PerformOperation(); public abstract Task GetOperationIcon(); + protected virtual void OnRunCompleted() { } + public void Dispose() { - while (OperationQueue.Remove(this)) - ; + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (!disposing || Disposed) + return; + + Disposed = true; + Cancel(); + if (!IsExecutingOperation) + { + while (OperationQueue.Remove(this)) + ; + } } } diff --git a/src/UniGetUI.PackageEngine.Operations/AbstractProcessOperation.cs b/src/UniGetUI.PackageEngine.Operations/AbstractProcessOperation.cs index 9dccbcacd8..a86613dda5 100644 --- a/src/UniGetUI.PackageEngine.Operations/AbstractProcessOperation.cs +++ b/src/UniGetUI.PackageEngine.Operations/AbstractProcessOperation.cs @@ -20,23 +20,10 @@ protected AbstractProcessOperation( : base(queue_enabled, preOps, postOps) { process = new(); - CancelRequested += (_, _) => - { - try - { - process.Kill(); - ProcessKilled = true; - } - catch (InvalidOperationException e) - { - Line( - "Attempted to cancel a process that hasn't ben created yet: " + e.Message, - LineType.Error - ); - } - }; + CancelRequested += (_, _) => StopProcess(); OperationStarting += (_, _) => { + DisposeProcess(); ProcessKilled = false; process = new(); process.StartInfo.UseShellExecute = false; @@ -56,12 +43,10 @@ protected AbstractProcessOperation( { if (e.Data is null) return; - string line = e.Data.ToString().Trim(); + string line = e.Data.Trim(); var lineType = LineType.Error; if (line.Length < 6 || line.Contains("Waiting for another install...")) - { lineType = LineType.ProgressIndicator; - } Line(line, lineType); }; @@ -98,12 +83,11 @@ protected override async Task PerformOperation() if (process.StartInfo.Arguments == "lol") throw new InvalidOperationException("StartInfo.Arguments has not been set"); - Line($"Executing process with StartInfo:", LineType.VerboseDetails); + Line("Executing process with StartInfo:", LineType.VerboseDetails); Line($" - FileName: \"{process.StartInfo.FileName.Trim()}\"", LineType.VerboseDetails); Line($" - Arguments: \"{process.StartInfo.Arguments.Trim()}\"", LineType.VerboseDetails); Line($"Start Time: \"{DateTime.Now}\"", LineType.VerboseDetails); - // An empty FileName means elevation was required but no elevator (UniGetUI Elevator/GSudo) is available if (string.IsNullOrWhiteSpace(process.StartInfo.FileName)) { Line( @@ -121,18 +105,25 @@ protected override async Task PerformOperation() await CoreTools.CacheUACForCurrentProcess(); } + CancellationToken.ThrowIfCancellationRequested(); process.Start(); + if (CancellationToken.IsCancellationRequested) + { + StopProcess(); + await process.WaitForExitAsync().ConfigureAwait(false); + return OperationVeredict.Canceled; + } + if (!Settings.Get(Settings.K.DisableNewProcessLineHandler)) { - await process.StandardInput.WriteLineAsync("\r\n\r\n\r\n\r\n"); + await process.StandardInput.WriteLineAsync("\r\n\r\n\r\n\r\n".AsMemory(), CancellationToken); process.StandardInput.Close(); } - // process.BeginOutputReadLine(); try { process.BeginErrorReadLine(); } - catch (Exception ex) + catch (InvalidOperationException ex) { Logger.Error(ex); } @@ -140,41 +131,56 @@ protected override async Task PerformOperation() StringBuilder currentLine = new(); char[] buffer = new char[1]; string? lastStringBeforeLF = null; - - while ((await process.StandardOutput.ReadBlockAsync(buffer)) > 0) + try { - char c = buffer[0]; - if (c == '\n') + while ((await process.StandardOutput.ReadAsync(buffer.AsMemory(), CancellationToken)) > 0) { - if (currentLine.Length == 0) + char c = buffer[0]; + if (c == 10) { - if (lastStringBeforeLF is not null) + if (currentLine.Length == 0) { - Line(lastStringBeforeLF, LineType.Information); - lastStringBeforeLF = null; + if (lastStringBeforeLF is not null) + { + Line(lastStringBeforeLF, LineType.Information); + lastStringBeforeLF = null; + } + continue; } - continue; - } - string line = currentLine.ToString(); - Line(line, LineType.Information); - currentLine.Clear(); - } - else if (c == '\r') - { - if (currentLine.Length == 0) - continue; - lastStringBeforeLF = currentLine.ToString(); - Line(lastStringBeforeLF, LineType.ProgressIndicator); - currentLine.Clear(); - } - else - { - currentLine.Append(c); + string line = currentLine.ToString(); + Line(line, LineType.Information); + currentLine.Clear(); + } + else if (c == 13) + { + if (currentLine.Length == 0) + continue; + lastStringBeforeLF = currentLine.ToString(); + Line(lastStringBeforeLF, LineType.ProgressIndicator); + currentLine.Clear(); + } + else + { + currentLine.Append(c); + } } } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + await process.WaitForExitAsync().ConfigureAwait(false); + return OperationVeredict.Canceled; + } - await process.WaitForExitAsync(); + try + { + await process.WaitForExitAsync(CancellationToken).ConfigureAwait(false); + } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + await process.WaitForExitAsync().ConfigureAwait(false); + return OperationVeredict.Canceled; + } Line($"End Time: \"{DateTime.Now}\"", LineType.VerboseDetails); Line( @@ -182,10 +188,9 @@ protected override async Task PerformOperation() LineType.VerboseDetails ); - if (ProcessKilled) + if (ProcessKilled || CancellationToken.IsCancellationRequested) return OperationVeredict.Canceled; - // Raw output: redacting here would corrupt the phrases result detection matches on. List output = new(); foreach (var line in GetRawOutput()) { @@ -198,6 +203,49 @@ protected override async Task PerformOperation() return await GetProcessVeredict(process.ExitCode, output); } + protected override void OnRunCompleted() + { + DisposeProcess(); + } + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + if (disposing) + DisposeProcess(); + } + + private void StopProcess() + { + try + { + if (!process.HasExited) + { + process.Kill(entireProcessTree: true); + ProcessKilled = true; + } + } + catch (InvalidOperationException) { } + } + + private void DisposeProcess() + { + try + { + if (!process.HasExited) + { + process.Kill(entireProcessTree: true); + ProcessKilled = true; + process.WaitForExit(); + } + } + catch (InvalidOperationException) { } + finally + { + process.Dispose(); + } + } + protected abstract Task GetProcessVeredict( int ReturnCode, List Output diff --git a/src/UniGetUI.PackageEngine.Operations/DownloadOperation.cs b/src/UniGetUI.PackageEngine.Operations/DownloadOperation.cs index e139f391c4..5d7734475b 100644 --- a/src/UniGetUI.PackageEngine.Operations/DownloadOperation.cs +++ b/src/UniGetUI.PackageEngine.Operations/DownloadOperation.cs @@ -10,11 +10,7 @@ public class DownloadOperation : AbstractOperation private readonly IPackage _package; public IPackage Package => _package; private string downloadLocation; - public string DownloadLocation - { - get => downloadLocation; - } - private bool canceled; + public string DownloadLocation => downloadLocation; public DownloadOperation(IPackage package, string downloadPath) : base(true, null) @@ -23,10 +19,7 @@ public DownloadOperation(IPackage package, string downloadPath) _package = package; Metadata.OperationInformation = - "Downloading installer for Package=" - + _package.Id - + " with Manager=" - + _package.Manager.Name; + "Downloading installer for Package=" + _package.Id + " with Manager=" + _package.Manager.Name; Metadata.Title = CoreTools.Translate( "{package} installer download", new Dictionary { { "package", _package.Name } } @@ -45,11 +38,6 @@ public DownloadOperation(IPackage package, string downloadPath) "{package} installer could not be downloaded", new Dictionary { { "package", _package.Name } } ); - - CancelRequested += (_, _) => - { - canceled = true; - }; } public override Task GetOperationIcon() @@ -57,27 +45,26 @@ public override Task GetOperationIcon() return Task.Run(_package.GetIconUrl); } - protected override void ApplyRetryAction(string retryMode) - { - // Do nothing - } + protected override void ApplyRetryAction(string retryMode) { } protected override async Task PerformOperation() { - canceled = false; + bool downloadFileCreated = false; try { + CancellationToken.ThrowIfCancellationRequested(); Line( $"Fetching download url for package {_package.Name} from {_package.Manager.DisplayName}...", LineType.Information ); await _package.Details.Load(); + CancellationToken.ThrowIfCancellationRequested(); Uri? downloadUrl = _package.Details.InstallerUrl; if (downloadUrl is null) { Line( $"UniGetUI was not able to find any installer for this package. " - + $"Please check that this package has an applicable installer and try again later", + + "Please check that this package has an applicable installer and try again later", LineType.Error ); return OperationVeredict.Failure; @@ -86,6 +73,7 @@ protected override async Task PerformOperation() if (Directory.Exists(downloadLocation)) { string? fileName = await _package.GetInstallerFileName(); + CancellationToken.ThrowIfCancellationRequested(); if (fileName is null) { Line( @@ -101,63 +89,65 @@ protected override async Task PerformOperation() using var httpClient = new HttpClient(CoreTools.GenericHttpClientParameters); using var response = await httpClient.GetAsync( downloadUrl, - HttpCompletionOption.ResponseHeadersRead + HttpCompletionOption.ResponseHeadersRead, + CancellationToken ); - response.EnsureSuccessStatusCode(); var totalBytes = response.Content.Headers.ContentLength ?? -1L; var canReportProgress = totalBytes > 0; - - using var contentStream = await response.Content.ReadAsStreamAsync(); - using var fileStream = new FileStream( + await using (var contentStream = await response.Content.ReadAsStreamAsync(CancellationToken)) + await using (var fileStream = new FileStream( downloadLocation, FileMode.Create, FileAccess.Write, FileShare.None, 8192, - true - ); - - var buffer = new byte[4 * 1024 * 1024]; - long totalRead = 0; - int bytesRead; - - int oldProgress = -1; - while ((bytesRead = await contentStream.ReadAsync(buffer)) > 0) + useAsync: true + )) { - await fileStream.WriteAsync(buffer.AsMemory(0, bytesRead)); - totalRead += bytesRead; - - if (canReportProgress) + downloadFileCreated = true; + var buffer = new byte[4 * 1024 * 1024]; + long totalRead = 0; + int oldProgress = -1; + int bytesRead; + while ( + (bytesRead = await contentStream.ReadAsync(buffer.AsMemory(), CancellationToken)) > 0 + ) { - var progress = (int)((totalRead * 100L) / totalBytes); - if (progress != oldProgress) + await fileStream.WriteAsync(buffer.AsMemory(0, bytesRead), CancellationToken); + totalRead += bytesRead; + + if (canReportProgress) { - oldProgress = progress; - Line( - CoreTools.TextProgressGenerator( - 30, - progress, - $"{CoreTools.FormatAsSize(totalRead)}/{CoreTools.FormatAsSize(totalBytes)}" - ), - LineType.ProgressIndicator - ); + var progress = (int)((totalRead * 100L) / totalBytes); + if (progress != oldProgress) + { + oldProgress = progress; + Line( + CoreTools.TextProgressGenerator( + 30, + progress, + $"{CoreTools.FormatAsSize(totalRead)}/{CoreTools.FormatAsSize(totalBytes)}" + ), + LineType.ProgressIndicator + ); + } } } - - if (canceled) - { - fileStream.Close(); - File.Delete(downloadLocation); - Line("User has canceled the operation", LineType.Error); - return OperationVeredict.Canceled; - } } + CancellationToken.ThrowIfCancellationRequested(); Line($"The file was saved to {downloadLocation}", LineType.Information); return OperationVeredict.Success; } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + if (downloadFileCreated) + File.Delete(downloadLocation); + Line("User has canceled the operation", LineType.Error); + return OperationVeredict.Canceled; + } catch (Exception ex) { Line($"{ex.GetType()}: {ex.Message}", LineType.Error); diff --git a/src/UniGetUI.PackageEngine.Operations/KillProcessOperation.cs b/src/UniGetUI.PackageEngine.Operations/KillProcessOperation.cs index f179d8644d..92959f07ba 100644 --- a/src/UniGetUI.PackageEngine.Operations/KillProcessOperation.cs +++ b/src/UniGetUI.PackageEngine.Operations/KillProcessOperation.cs @@ -16,9 +16,9 @@ public KillProcessOperation(string procName) Metadata.Status = $"Closing process(es) {procName}"; Metadata.Title = $"Closing process(es) {procName}"; Metadata.OperationInformation = " "; - Metadata.SuccessTitle = $"Done!"; - Metadata.SuccessMessage = $"Done!"; - Metadata.FailureTitle = $"Failed to close process"; + Metadata.SuccessTitle = "Done!"; + Metadata.SuccessMessage = "Done!"; + Metadata.FailureTitle = "Failed to close process"; Metadata.FailureMessage = $"The process(es) {procName} could not be closed"; } @@ -32,39 +32,51 @@ protected override async Task PerformOperation() $"Attempting to close all processes with name {ProcessName}...", LineType.Information ); - var procs = Process.GetProcessesByName(ProcessName.Replace(".exe", "")); - foreach (var proc in procs) + foreach (var proc in Process.GetProcessesByName(ProcessName.Replace(".exe", ""))) { - if (proc.HasExited) - continue; - Line( - $"Attempting to close process {ProcessName} with pid={proc.Id}...", - LineType.VerboseDetails - ); - proc.CloseMainWindow(); - await Task.WhenAny(proc.WaitForExitAsync(), Task.Delay(1000)); - if (!proc.HasExited) + using (proc) { - if (Settings.Get(Settings.K.KillProcessesThatRefuseToDie)) + CancellationToken.ThrowIfCancellationRequested(); + if (proc.HasExited) + continue; + Line( + $"Attempting to close process {ProcessName} with pid={proc.Id}...", + LineType.VerboseDetails + ); + proc.CloseMainWindow(); + await Task.WhenAny( + proc.WaitForExitAsync(CancellationToken), + Task.Delay(1000, CancellationToken) + ); + CancellationToken.ThrowIfCancellationRequested(); + if (!proc.HasExited) { - Line( - $"Timeout for process {ProcessName}, attempting to kill...", - LineType.Information - ); - proc.Kill(); - } - else - { - Line( - $"{ProcessName} with pid={proc.Id} did not exit and will not be killed. You can change this from UniGetUI settings.", - LineType.Error - ); + if (Settings.Get(Settings.K.KillProcessesThatRefuseToDie)) + { + Line( + $"Timeout for process {ProcessName}, attempting to kill...", + LineType.Information + ); + proc.Kill(entireProcessTree: true); + await proc.WaitForExitAsync(CancellationToken); + } + else + { + Line( + $"{ProcessName} with pid={proc.Id} did not exit and will not be killed. You can change this from UniGetUI settings.", + LineType.Error + ); + } } } } return OperationVeredict.Success; } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + return OperationVeredict.Canceled; + } catch (Exception ex) { Line(ex.ToString(), LineType.Error); diff --git a/src/UniGetUI.PackageEngine.Operations/PrePostOperation.cs b/src/UniGetUI.PackageEngine.Operations/PrePostOperation.cs index 692db4b176..b9a7f38a2d 100644 --- a/src/UniGetUI.PackageEngine.Operations/PrePostOperation.cs +++ b/src/UniGetUI.PackageEngine.Operations/PrePostOperation.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using UniGetUI.PackageEngine.Enums; using UniGetUI.PackageOperations; @@ -6,18 +7,21 @@ namespace UniGetUI.PackageEngine.Operations; public class PrePostOperation : AbstractOperation { private readonly string Payload; + private readonly object ProcessLock = new(); + private Process? ActiveProcess; public PrePostOperation(string payload) : base(true) { Payload = payload.Replace("\r", "\n").Replace("\n\n", "\n").Replace("\n", "&"); Metadata.Status = $"Running custom operation {Payload}"; - Metadata.Title = $"Custom operation"; + Metadata.Title = "Custom operation"; Metadata.OperationInformation = " "; - Metadata.SuccessTitle = $"Done!"; - Metadata.SuccessMessage = $"Done!"; - Metadata.FailureTitle = $"Custom operation failed"; + Metadata.SuccessTitle = "Done!"; + Metadata.SuccessMessage = "Done!"; + Metadata.FailureTitle = "Custom operation failed"; Metadata.FailureMessage = $"The custom operation {Payload} failed to run"; + CancelRequested += (_, _) => StopActiveProcess(); } protected override void ApplyRetryAction(string retryMode) { } @@ -25,9 +29,9 @@ protected override void ApplyRetryAction(string retryMode) { } protected override async Task PerformOperation() { Line($"Running command {Payload}", LineType.Information); - var process = new System.Diagnostics.Process + using var process = new Process { - StartInfo = new System.Diagnostics.ProcessStartInfo + StartInfo = new ProcessStartInfo { FileName = "cmd.exe", Arguments = $"/C {Payload}", @@ -37,25 +41,87 @@ protected override async Task PerformOperation() CreateNoWindow = true, }, }; - - process.Start(); - process.BeginErrorReadLine(); - process.BeginOutputReadLine(); - process.OutputDataReceived += (s, e) => + DataReceivedEventHandler outputHandler = (_, e) => { if (e.Data is not null) Line(e.Data, LineType.Information); }; - process.ErrorDataReceived += (s, e) => + DataReceivedEventHandler errorHandler = (_, e) => { if (e.Data is not null) Line(e.Data, LineType.Error); }; - await process.WaitForExitAsync(); + process.OutputDataReceived += outputHandler; + process.ErrorDataReceived += errorHandler; + lock (ProcessLock) + ActiveProcess = process; + bool processStarted = false; + + try + { + CancellationToken.ThrowIfCancellationRequested(); + process.Start(); + processStarted = true; + if (CancellationToken.IsCancellationRequested) + StopActiveProcess(); + process.BeginErrorReadLine(); + process.BeginOutputReadLine(); + + try + { + await process.WaitForExitAsync(CancellationToken).ConfigureAwait(false); + } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + await process.WaitForExitAsync().ConfigureAwait(false); + return OperationVeredict.Canceled; + } + + if (CancellationToken.IsCancellationRequested) + return OperationVeredict.Canceled; + + int exitCode = process.ExitCode; + Line($"Exit code is {exitCode}", LineType.Information); + return exitCode == 0 ? OperationVeredict.Success : OperationVeredict.Failure; + } + finally + { + if (processStarted && !process.HasExited) + { + StopActiveProcess(); + await process.WaitForExitAsync().ConfigureAwait(false); + } + process.OutputDataReceived -= outputHandler; + process.ErrorDataReceived -= errorHandler; + lock (ProcessLock) + { + if (ReferenceEquals(ActiveProcess, process)) + ActiveProcess = null; + } + } + } - int exitCode = process.ExitCode; - Line($"Exit code is {exitCode}", LineType.Information); - return (exitCode == 0 ? OperationVeredict.Success : OperationVeredict.Failure); + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + if (disposing) + StopActiveProcess(); + } + + private void StopActiveProcess() + { + Process? process; + lock (ProcessLock) + process = ActiveProcess; + if (process is null) + return; + + try + { + if (!process.HasExited) + process.Kill(entireProcessTree: true); + } + catch (InvalidOperationException) { } } public override Task GetOperationIcon() => Task.FromResult(new Uri("about:blank")); diff --git a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs index 49f2f75dd7..b900b413cc 100644 --- a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs @@ -362,6 +362,23 @@ public void UsernameRedactionAppliesToDisplayOutputButNeverToResultParsingOutput } } + [Fact] + public async Task CancelWaitsForTheActiveOperationToCompleteCleanup() + { + using var operation = new CancellationAwareStubOperation(); + Task mainThread = operation.MainThread(); + await operation.PerformStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + + operation.Cancel(); + await operation.CancellationObserved.Task.WaitAsync(TimeSpan.FromSeconds(1)); + + Assert.False(mainThread.IsCompleted); + operation.AllowCleanupToComplete.TrySetResult(true); + await mainThread; + + Assert.Equal(OperationStatus.Canceled, operation.Status); + } + private static IReadOnlyList GetInnerOperations( AbstractOperation operation, string fieldName @@ -493,6 +510,52 @@ protected override Task PerformOperation() } } + private sealed class CancellationAwareStubOperation : AbstractOperation + { + public TaskCompletionSource PerformStarted { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + public TaskCompletionSource CancellationObserved { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + public TaskCompletionSource AllowCleanupToComplete { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + + public CancellationAwareStubOperation() + : base(queue_enabled: false) + { + Metadata.Status = "Cancelable stub status"; + Metadata.Title = "Cancelable stub title"; + Metadata.OperationInformation = "Cancelable stub info"; + Metadata.SuccessTitle = "Cancelable stub success"; + Metadata.SuccessMessage = "Cancelable stub success"; + Metadata.FailureTitle = "Cancelable stub failure"; + Metadata.FailureMessage = "Cancelable stub failure"; + } + + protected override void ApplyRetryAction(string retryMode) { } + + protected override async Task PerformOperation() + { + PerformStarted.TrySetResult(true); + try + { + await Task.Delay(Timeout.InfiniteTimeSpan, CancellationToken); + } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + CancellationObserved.TrySetResult(true); + await AllowCleanupToComplete.Task; + return OperationVeredict.Canceled; + } + + return OperationVeredict.Success; + } + + public override Task GetOperationIcon() => Task.FromResult(new Uri("about:blank")); + } + private sealed class LoggingStubOperation : AbstractOperation { public LoggingStubOperation() From 29e87826ccb16a4178fa3e3e9b6eb9a32ddc9e69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Fri, 10 Jul 2026 14:17:42 -0400 Subject: [PATCH 2/2] Serialize package operation retries Prevent cancellation, retry, and disposal races from overlapping operation runs or corrupting terminal state. Add focused regression coverage for cleanup ordering, repeated retries, startup failures, queue cancellation, and disposal boundaries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../AbstractOperation.cs | 233 +++++++++++-- .../PackageOperationsTests.cs | 306 +++++++++++++++++- 2 files changed, 512 insertions(+), 27 deletions(-) diff --git a/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs b/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs index 81e0d97292..233d17a43f 100644 --- a/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs +++ b/src/UniGetUI.PackageEngine.Operations/AbstractOperation.cs @@ -26,6 +26,9 @@ public abstract partial class AbstractOperation : IDisposable private readonly object CancellationLock = new(); private CancellationTokenSource? RunCancellationSource; private AbstractOperation? ActiveInnerOperation; + private Task? ActiveRunTask; + private TaskCompletionSource? ScheduledRetryCompletionSource; + private bool ActiveRunIsStarting; private volatile bool IsExecutingOperation; private bool Disposed; @@ -78,19 +81,27 @@ public AbstractOperation( public void Cancel() { - if (_status is OperationStatus.Canceled or OperationStatus.Failed or OperationStatus.Succeeded) - return; - - bool wasRunning = _status is OperationStatus.Running; - bool hasActiveWork = IsExecutingOperation; - Status = OperationStatus.Canceled; - CancellationTokenSource? cancellationSource; - lock (CancellationLock) - cancellationSource = RunCancellationSource; - cancellationSource?.Cancel(); AbstractOperation? activeInnerOperation; + bool wasRunning; + bool hasActiveWork; lock (CancellationLock) + { + if ( + _status + is OperationStatus.Canceled + or OperationStatus.Failed + or OperationStatus.Succeeded + && !ActiveRunIsStarting + ) + return; + + wasRunning = _status is OperationStatus.Running; + hasActiveWork = IsExecutingOperation; + RunCancellationSource?.Cancel(); activeInnerOperation = ActiveInnerOperation; + } + + Status = OperationStatus.Canceled; activeInnerOperation?.Cancel(); if (wasRunning) @@ -114,14 +125,6 @@ protected CancellationToken CancellationToken } } - private CancellationTokenSource StartRunCancellation() - { - var cancellationSource = new CancellationTokenSource(); - lock (CancellationLock) - RunCancellationSource = cancellationSource; - return cancellationSource; - } - private bool TrySetActiveInnerOperation(AbstractOperation operation) { bool cancellationRequested; @@ -154,6 +157,7 @@ private void EndRunCancellation(CancellationTokenSource cancellationSource) if (!ReferenceEquals(RunCancellationSource, cancellationSource)) return; RunCancellationSource = null; + ActiveRunIsStarting = false; } cancellationSource.Dispose(); } @@ -175,9 +179,49 @@ protected void Line(string line, LineType type) return LogList.Select(l => (Logger.Redact(l.Item1), l.Item2)).ToList(); } - public async Task MainThread() + public Task MainThread() + { + TaskCompletionSource completionSource; + CancellationTokenSource cancellationSource; + lock (CancellationLock) + { + ObjectDisposedException.ThrowIf(Disposed, this); + + if (ActiveRunTask is { IsCompleted: false }) + return ActiveRunTask; + + if (ScheduledRetryCompletionSource is not null) + return ScheduledRetryCompletionSource.Task; + + completionSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + cancellationSource = new(); + ActiveRunTask = completionSource.Task; + RunCancellationSource = cancellationSource; + ActiveRunIsStarting = true; + } + + _ = ExecuteRunAsync(completionSource, cancellationSource); + return completionSource.Task; + } + + private async Task ExecuteRunAsync( + TaskCompletionSource completionSource, + CancellationTokenSource cancellationSource + ) + { + try + { + await MainThreadCore(cancellationSource).ConfigureAwait(false); + completionSource.SetResult(); + } + catch (Exception ex) + { + completionSource.SetException(ex); + } + } + + private async Task MainThreadCore(CancellationTokenSource runCancellation) { - CancellationTokenSource runCancellation = StartRunCancellation(); try { if (Metadata.Status == "") @@ -200,17 +244,44 @@ public async Task MainThread() if (OperationQueue.Contains(this)) throw new InvalidOperationException("This operation was already on the queue"); + if (runCancellation.IsCancellationRequested) + { + MarkRunAsStarted(runCancellation); + CompleteCanceledRun(); + return; + } + Status = OperationStatus.InQueue; + MarkRunAsStarted(runCancellation); + if (runCancellation.IsCancellationRequested) + { + CompleteCanceledRun(); + return; + } + Line(Metadata.OperationInformation, LineType.VerboseDetails); Line(Metadata.Status, LineType.ProgressIndicator); Enqueued?.Invoke(this, EventArgs.Empty); + if (runCancellation.IsCancellationRequested) + { + CompleteCanceledRun(); + return; + } if (QUEUE_ENABLED && !IsInnerOperation) { // QUEUE HANDLER SKIP_QUEUE = false; OperationQueue.Add(this); + if (runCancellation.IsCancellationRequested) + { + while (OperationQueue.Remove(this)) + ; + CompleteCanceledRun(); + return; + } + int lastPos = -2; while ( @@ -293,6 +364,7 @@ public async Task MainThread() while (OperationQueue.Remove(this)) ; + MarkRunAsStarted(runCancellation); Status = OperationStatus.Failed; try { @@ -334,6 +406,22 @@ public async Task MainThread() } } + private void CompleteCanceledRun() + { + Status = OperationStatus.Canceled; + OperationFinished?.Invoke(this, EventArgs.Empty); + Line(CoreTools.Translate("Operation canceled by user"), LineType.Error); + } + + private void MarkRunAsStarted(CancellationTokenSource cancellationSource) + { + lock (CancellationLock) + { + if (ReferenceEquals(RunCancellationSource, cancellationSource)) + ActiveRunIsStarting = false; + } + } + private async Task _runOperation() { OperationVeredict result; @@ -527,14 +615,98 @@ public void Retry(string retryMode) if (retryMode is RetryMode.NoRetry) throw new InvalidOperationException("We weren't supposed to reach this, weren't we?"); + Task? previousRun = null; + TaskCompletionSource? scheduledRetry = null; + bool applyImmediately = false; + lock (CancellationLock) + { + if (Disposed) + return; + + if (Status is OperationStatus.Running) + return; + + if (Status is OperationStatus.InQueue) + { + applyImmediately = true; + } + else + { + if (ScheduledRetryCompletionSource is not null) + return; + + scheduledRetry = new(TaskCreationOptions.RunContinuationsAsynchronously); + ScheduledRetryCompletionSource = scheduledRetry; + previousRun = ActiveRunTask ?? Task.CompletedTask; + } + } + + if (applyImmediately) + { + ApplyRetryActionAndLog(retryMode); + return; + } + + _ = RetryAfterRunAsync(previousRun!, scheduledRetry!, retryMode); + } + + private async Task RetryAfterRunAsync( + Task previousRun, + TaskCompletionSource scheduledRetry, + string retryMode + ) + { + CancellationTokenSource? cancellationSource = null; + try + { + await previousRun.ConfigureAwait(false); + + lock (CancellationLock) + { + if (!ReferenceEquals(ScheduledRetryCompletionSource, scheduledRetry)) + return; + + if (Disposed) + { + ScheduledRetryCompletionSource = null; + scheduledRetry.TrySetCanceled(); + return; + } + + cancellationSource = new(); + RunCancellationSource = cancellationSource; + ActiveRunTask = scheduledRetry.Task; + ScheduledRetryCompletionSource = null; + ActiveRunIsStarting = true; + } + + ApplyRetryActionAndLog(retryMode); + await ExecuteRunAsync(scheduledRetry, cancellationSource).ConfigureAwait(false); + } + catch (Exception ex) + { + if (cancellationSource is not null) + EndRunCancellation(cancellationSource); + + lock (CancellationLock) + { + if (ReferenceEquals(ScheduledRetryCompletionSource, scheduledRetry)) + ScheduledRetryCompletionSource = null; + if (ReferenceEquals(ActiveRunTask, scheduledRetry.Task)) + ActiveRunTask = null; + } + scheduledRetry.TrySetException(ex); + Logger.Error(ex); + } + } + + private void ApplyRetryActionAndLog(string retryMode) + { ApplyRetryAction(retryMode); Line($"", LineType.VerboseDetails); Line($"-----------------------", LineType.VerboseDetails); Line($"Retrying operation with RetryMode={retryMode}", LineType.VerboseDetails); Line($"", LineType.VerboseDetails); - if (Status is OperationStatus.Running or OperationStatus.InQueue) - return; - _ = MainThread(); } protected abstract void ApplyRetryAction(string retryMode); @@ -551,10 +723,21 @@ public void Dispose() protected virtual void Dispose(bool disposing) { - if (!disposing || Disposed) + if (!disposing) return; - Disposed = true; + TaskCompletionSource? scheduledRetry; + lock (CancellationLock) + { + if (Disposed) + return; + + Disposed = true; + scheduledRetry = ScheduledRetryCompletionSource; + ScheduledRetryCompletionSource = null; + } + + scheduledRetry?.TrySetCanceled(); Cancel(); if (!IsExecutingOperation) { diff --git a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs index b900b413cc..bb68cac616 100644 --- a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs @@ -379,6 +379,144 @@ public async Task CancelWaitsForTheActiveOperationToCompleteCleanup() Assert.Equal(OperationStatus.Canceled, operation.Status); } + [Fact] + public async Task RetryWaitsForCanceledRunCleanupBeforeStartingAgain() + { + using var operation = new RetryAwareStubOperation(); + Task firstRun = operation.MainThread(); + await operation.FirstRunStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + + operation.Cancel(); + await operation.CancellationObserved.Task.WaitAsync(TimeSpan.FromSeconds(1)); + operation.Retry(AbstractOperation.RetryMode.Retry); + + await Task.Delay(50); + Assert.Equal(1, operation.RunCount); + Assert.False(firstRun.IsCompleted); + + operation.AllowCleanupToComplete.TrySetResult(true); + await firstRun; + await operation.SecondRunStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + await WaitForAsync(() => operation.Status is OperationStatus.Succeeded); + + Assert.Equal(2, operation.RunCount); + Assert.Equal(1, operation.MaxConcurrentRuns); + } + + [Fact] + public async Task ConcurrentMainThreadCallsShareTheActiveRun() + { + using var operation = new CancellationAwareStubOperation(); + + Task firstCall = operation.MainThread(); + Task secondCall = operation.MainThread(); + + Assert.Same(firstCall, secondCall); + await operation.PerformStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + operation.Cancel(); + await operation.CancellationObserved.Task.WaitAsync(TimeSpan.FromSeconds(1)); + operation.AllowCleanupToComplete.TrySetResult(true); + await firstCall; + } + + [Fact] + public async Task RetryRequestedFromRetriedRunCompletionSchedulesAnotherRun() + { + using var operation = new RepeatedRetryStubOperation(); + await operation.MainThread(); + operation.OperationFailed += (_, _) => + { + if (operation.RunCount == 2) + operation.Retry(AbstractOperation.RetryMode.Retry); + }; + + operation.Retry(AbstractOperation.RetryMode.Retry); + + await operation.ThirdRunStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + await WaitForAsync(() => operation.Status is OperationStatus.Failed); + Assert.Equal(3, operation.RunCount); + } + + [Fact] + public async Task DisposePreventsADeferredRetryFromStarting() + { + var operation = new RetryAwareStubOperation(); + Task firstRun = operation.MainThread(); + await operation.FirstRunStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + operation.Cancel(); + await operation.CancellationObserved.Task.WaitAsync(TimeSpan.FromSeconds(1)); + operation.Retry(AbstractOperation.RetryMode.Retry); + + operation.Dispose(); + operation.AllowCleanupToComplete.TrySetResult(true); + await firstRun; + await Task.Delay(50); + + Assert.Equal(1, operation.RunCount); + Assert.Throws(() => + { + _ = operation.MainThread(); + }); + } + + [Fact] + public async Task DisposeFromTerminalEventPreservesCompletedStatus() + { + var operation = new RepeatedRetryStubOperation(); + operation.OperationFailed += (_, _) => operation.Dispose(); + + await operation.MainThread(); + + Assert.Equal(OperationStatus.Failed, operation.Status); + } + + [Fact] + public async Task DisposeFromStartupFailurePreservesFailedStatus() + { + var operation = new InvalidMetadataStubOperation(); + operation.OperationFailed += (_, _) => operation.Dispose(); + + await operation.MainThread(); + + Assert.Equal(OperationStatus.Failed, operation.Status); + } + + [Fact] + public async Task RetryOptionFailureDoesNotPreventALaterRetry() + { + using var operation = new ThrowingRetryStubOperation(); + await operation.MainThread(); + + operation.Retry("InvalidRetryMode"); + operation.Retry(AbstractOperation.RetryMode.Retry); + + await operation.SecondRunStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + Assert.Equal(2, operation.RunCount); + } + + [Fact] + public async Task CancellationFromEnqueuedDoesNotRemainOnAFullQueue() + { + using var firstOperation = new CancellationAwareStubOperation(queueEnabled: true); + using var canceledOperation = new CancellationAwareStubOperation(queueEnabled: true); + AbstractOperation.OperationQueue.Clear(); + AbstractOperation.MAX_OPERATIONS = 1; + + Task firstRun = firstOperation.MainThread(); + await firstOperation.PerformStarted.Task.WaitAsync(TimeSpan.FromSeconds(1)); + canceledOperation.Enqueued += (_, _) => canceledOperation.Cancel(); + + await canceledOperation.MainThread().WaitAsync(TimeSpan.FromSeconds(1)); + + Assert.Equal(OperationStatus.Canceled, canceledOperation.Status); + Assert.DoesNotContain(canceledOperation, AbstractOperation.OperationQueue); + + firstOperation.Cancel(); + await firstOperation.CancellationObserved.Task.WaitAsync(TimeSpan.FromSeconds(1)); + firstOperation.AllowCleanupToComplete.TrySetResult(true); + await firstRun; + } + private static IReadOnlyList GetInnerOperations( AbstractOperation operation, string fieldName @@ -522,8 +660,8 @@ private sealed class CancellationAwareStubOperation : AbstractOperation TaskCreationOptions.RunContinuationsAsynchronously ); - public CancellationAwareStubOperation() - : base(queue_enabled: false) + public CancellationAwareStubOperation(bool queueEnabled = false) + : base(queue_enabled: queueEnabled) { Metadata.Status = "Cancelable stub status"; Metadata.Title = "Cancelable stub title"; @@ -556,6 +694,170 @@ protected override async Task PerformOperation() public override Task GetOperationIcon() => Task.FromResult(new Uri("about:blank")); } + private sealed class RetryAwareStubOperation : AbstractOperation + { + private int _concurrentRuns; + private int _maxConcurrentRuns; + private int _runCount; + + public TaskCompletionSource FirstRunStarted { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + public TaskCompletionSource CancellationObserved { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + public TaskCompletionSource AllowCleanupToComplete { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + public TaskCompletionSource SecondRunStarted { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + + public int RunCount => Volatile.Read(ref _runCount); + public int MaxConcurrentRuns => Volatile.Read(ref _maxConcurrentRuns); + + public RetryAwareStubOperation() + : base(queue_enabled: false) + { + Metadata.Status = "Retryable stub status"; + Metadata.Title = "Retryable stub title"; + Metadata.OperationInformation = "Retryable stub info"; + Metadata.SuccessTitle = "Retryable stub success"; + Metadata.SuccessMessage = "Retryable stub success"; + Metadata.FailureTitle = "Retryable stub failure"; + Metadata.FailureMessage = "Retryable stub failure"; + } + + protected override void ApplyRetryAction(string retryMode) { } + + protected override async Task PerformOperation() + { + int concurrentRuns = Interlocked.Increment(ref _concurrentRuns); + UpdateMaximum(ref _maxConcurrentRuns, concurrentRuns); + int run = Interlocked.Increment(ref _runCount); + try + { + if (run == 1) + { + FirstRunStarted.TrySetResult(true); + try + { + await Task.Delay(Timeout.InfiniteTimeSpan, CancellationToken); + } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + CancellationObserved.TrySetResult(true); + await AllowCleanupToComplete.Task; + return OperationVeredict.Canceled; + } + } + + SecondRunStarted.TrySetResult(true); + return OperationVeredict.Success; + } + finally + { + Interlocked.Decrement(ref _concurrentRuns); + } + } + + private static void UpdateMaximum(ref int target, int value) + { + int current; + do + { + current = Volatile.Read(ref target); + if (current >= value) + return; + } while (Interlocked.CompareExchange(ref target, value, current) != current); + } + + public override Task GetOperationIcon() => Task.FromResult(new Uri("about:blank")); + } + + private sealed class RepeatedRetryStubOperation : AbstractOperation + { + private int _runCount; + + public int RunCount => Volatile.Read(ref _runCount); + public TaskCompletionSource ThirdRunStarted { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + + public RepeatedRetryStubOperation() + : base(queue_enabled: false) + { + Metadata.Status = "Repeated retry stub status"; + Metadata.Title = "Repeated retry stub title"; + Metadata.OperationInformation = "Repeated retry stub info"; + Metadata.SuccessTitle = "Repeated retry stub success"; + Metadata.SuccessMessage = "Repeated retry stub success"; + Metadata.FailureTitle = "Repeated retry stub failure"; + Metadata.FailureMessage = "Repeated retry stub failure"; + } + + protected override void ApplyRetryAction(string retryMode) { } + + protected override Task PerformOperation() + { + if (Interlocked.Increment(ref _runCount) == 3) + ThirdRunStarted.TrySetResult(true); + return Task.FromResult(OperationVeredict.Failure); + } + + public override Task GetOperationIcon() => Task.FromResult(new Uri("about:blank")); + } + + private sealed class InvalidMetadataStubOperation : AbstractOperation + { + public InvalidMetadataStubOperation() + : base(queue_enabled: false) { } + + protected override void ApplyRetryAction(string retryMode) { } + + protected override Task PerformOperation() + => Task.FromResult(OperationVeredict.Success); + + public override Task GetOperationIcon() => Task.FromResult(new Uri("about:blank")); + } + + private sealed class ThrowingRetryStubOperation : AbstractOperation + { + private int _runCount; + + public int RunCount => Volatile.Read(ref _runCount); + public TaskCompletionSource SecondRunStarted { get; } = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + + public ThrowingRetryStubOperation() + : base(queue_enabled: false) + { + Metadata.Status = "Throwing retry stub status"; + Metadata.Title = "Throwing retry stub title"; + Metadata.OperationInformation = "Throwing retry stub info"; + Metadata.SuccessTitle = "Throwing retry stub success"; + Metadata.SuccessMessage = "Throwing retry stub success"; + Metadata.FailureTitle = "Throwing retry stub failure"; + Metadata.FailureMessage = "Throwing retry stub failure"; + } + + protected override void ApplyRetryAction(string retryMode) + { + if (retryMode == "InvalidRetryMode") + throw new InvalidOperationException("Invalid retry mode"); + } + + protected override Task PerformOperation() + { + if (Interlocked.Increment(ref _runCount) == 2) + SecondRunStarted.TrySetResult(true); + return Task.FromResult(OperationVeredict.Failure); + } + + public override Task GetOperationIcon() => Task.FromResult(new Uri("about:blank")); + } + private sealed class LoggingStubOperation : AbstractOperation { public LoggingStubOperation()