diff --git a/src/UniGetUI.Avalonia/Models/PackageCollections.cs b/src/UniGetUI.Avalonia/Models/PackageCollections.cs index 7c70c04cae..06dff35e4e 100644 --- a/src/UniGetUI.Avalonia/Models/PackageCollections.cs +++ b/src/UniGetUI.Avalonia/Models/PackageCollections.cs @@ -40,10 +40,13 @@ public sealed class PackageWrapper : INotifyPropertyChanged, IDisposable // reference the bitmap, so dropping it here only makes it GC-eligible. private const int MaxIconCacheEntries = 512; private static readonly object _iconCacheLock = new(); - private static readonly Dictionary> _iconCache = new(); - private static readonly LinkedList<(long Hash, Bitmap? Bitmap)> _iconCacheOrder = new(); + private static readonly Dictionary> _iconCache = new(); + private static readonly LinkedList<(long Hash, Bitmap Bitmap)> _iconCacheOrder = new(); + private static readonly Dictionary _failedIcons = new(); + private const int MaxFailedIconEntries = 512; + private static readonly TimeSpan IconRetryInterval = TimeSpan.FromMinutes(5); - private static bool TryGetCachedIcon(long hash, out Bitmap? bitmap) + private static Bitmap? GetCachedIcon(long hash) { lock (_iconCacheLock) { @@ -51,18 +54,58 @@ private static bool TryGetCachedIcon(long hash, out Bitmap? bitmap) { _iconCacheOrder.Remove(node); _iconCacheOrder.AddFirst(node); - bitmap = node.Value.Bitmap; - return true; + return node.Value.Bitmap; } } - bitmap = null; - return false; + return null; + } + + private static bool HasRecentIconFailure(long hash) + { + lock (_iconCacheLock) + { + if (!_failedIcons.TryGetValue(hash, out long failedAt)) + return false; + + if (Environment.TickCount64 - failedAt < (long)IconRetryInterval.TotalMilliseconds) + return true; + + _failedIcons.Remove(hash); + return false; + } + } + + private static void MarkIconFailed(long hash) + { + lock (_iconCacheLock) + { + _failedIcons[hash] = Environment.TickCount64; + if (_failedIcons.Count > MaxFailedIconEntries) + TrimFailedIcons(); + } } - private static void CacheIcon(long hash, Bitmap? bitmap) + private static void TrimFailedIcons() + { + long now = Environment.TickCount64; + long retryMs = (long)IconRetryInterval.TotalMilliseconds; + foreach (var expired in _failedIcons.Where(e => now - e.Value >= retryMs).ToArray()) + _failedIcons.Remove(expired.Key); + + int excess = _failedIcons.Count - MaxFailedIconEntries; + if (excess <= 0) + return; + + foreach (var oldest in _failedIcons.OrderBy(e => e.Value).Take(excess).ToArray()) + _failedIcons.Remove(oldest.Key); + } + + private static void CacheIcon(long hash, Bitmap bitmap) { lock (_iconCacheLock) { + _failedIcons.Remove(hash); + if (_iconCache.TryGetValue(hash, out var existing)) { existing.Value = (hash, bitmap); @@ -71,7 +114,7 @@ private static void CacheIcon(long hash, Bitmap? bitmap) return; } - var node = new LinkedListNode<(long, Bitmap?)>((hash, bitmap)); + var node = new LinkedListNode<(long, Bitmap)>((hash, bitmap)); _iconCache[hash] = node; _iconCacheOrder.AddFirst(node); @@ -89,6 +132,7 @@ public static void ClearIconCache() { _iconCache.Clear(); _iconCacheOrder.Clear(); + _failedIcons.Clear(); } } @@ -265,18 +309,17 @@ private async Task LoadIconAsync() { CancellationToken token = _lifetimeCts.Token; long hash = Package.GetHash(); - if (TryGetCachedIcon(hash, out Bitmap? cached)) + if (GetCachedIcon(hash) is { } cached) { - if (cached is not null) + await Dispatcher.UIThread.InvokeAsync(() => { - await Dispatcher.UIThread.InvokeAsync(() => - { - if (!token.IsCancellationRequested) IconBitmap = cached; - }); - } + if (!token.IsCancellationRequested) IconBitmap = cached; + }); return; } + if (HasRecentIconFailure(hash)) return; + try { Bitmap? bitmap = await GetSharedIconLoad(hash, Package).WaitAsync(token).ConfigureAwait(false); @@ -289,15 +332,17 @@ await Dispatcher.UIThread.InvokeAsync(() => }); } catch (OperationCanceledException) { /* row discarded before its icon finished loading */ } - catch { CacheIcon(hash, null); } + catch { MarkIconFailed(hash); } } private static Task GetSharedIconLoad(long hash, IPackage package) { lock (_inflightIconLoadsLock) { - if (TryGetCachedIcon(hash, out Bitmap? cached)) - return Task.FromResult(cached); + if (GetCachedIcon(hash) is { } cached) + return Task.FromResult(cached); + if (HasRecentIconFailure(hash)) + return Task.FromResult(null); if (_inflightIconLoads.TryGetValue(hash, out Task? existing)) return existing; @@ -328,14 +373,14 @@ private static async Task RemoveInflightIconLoadAsync(long hash, Task t try { var uri = await Task.Run(package.GetIconUrlIfAny).ConfigureAwait(false); - if (uri is null) { CacheIcon(hash, null); return null; } + if (uri is null) { MarkIconFailed(hash); return null; } Bitmap? decoded; if (uri.IsFile) { - if (!IsSkiaDecodableExtension(uri.LocalPath)) + if (IsKnownUndecodableExtension(uri.LocalPath)) { - CacheIcon(hash, null); + MarkIconFailed(hash); return null; } decoded = await Task.Run(() => TryDecodeIcon(uri.LocalPath)).ConfigureAwait(false); @@ -348,7 +393,7 @@ private static async Task RemoveInflightIconLoadAsync(long hash, Task t response.EnsureSuccessStatusCode(); if (response.Content.Headers.ContentLength > MaxIconDownloadBytes) { - CacheIcon(hash, null); + MarkIconFailed(hash); return null; } @@ -356,14 +401,20 @@ private static async Task RemoveInflightIconLoadAsync(long hash, Task t var bytes = await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false); decoded = TryDecodeIcon(bytes, uri.Host); } - else { CacheIcon(hash, null); return null; } + else { MarkIconFailed(hash); return null; } + + if (decoded is null) + { + MarkIconFailed(hash); + return null; + } CacheIcon(hash, decoded); return decoded; } catch { - CacheIcon(hash, null); + MarkIconFailed(hash); return null; } finally @@ -373,7 +424,7 @@ private static async Task RemoveInflightIconLoadAsync(long hash, Task t } // Icons come from a shared on-disk cache that can hold empty or partial entries after an - // interrupted download; decoding those throws. Skip them quietly instead of surfacing an error. + // interrupted download; decoding those throws. Skip those entries instead of failing the row. private static Bitmap? TryDecodeIcon(string filePath) { var info = new FileInfo(filePath); @@ -387,7 +438,7 @@ private static async Task RemoveInflightIconLoadAsync(long hash, Task t private static Bitmap? TryDecodeIcon(Func decode, string source) { try { return decode(); } - catch (Exception ex) { Logger.Debug($"Discarding undecodable icon '{source}': {ex.Message}"); return null; } + catch (Exception ex) { Logger.Warn($"Discarding undecodable icon '{source}': {ex.Message}"); return null; } } // Decode directly at the display-cache width. This avoids allocating a full-size bitmap first, @@ -395,15 +446,13 @@ private static async Task RemoveInflightIconLoadAsync(long hash, Task t private static Bitmap DecodeDownscaled(Stream stream) => Bitmap.DecodeToWidth(stream, MaxIconSide, BitmapInterpolationMode.HighQuality); - private static bool IsSkiaDecodableExtension(string path) + private static bool IsKnownUndecodableExtension(string path) { string ext = Path.GetExtension(path); - return ext.Equals(".png", StringComparison.OrdinalIgnoreCase) - || ext.Equals(".jpg", StringComparison.OrdinalIgnoreCase) - || ext.Equals(".jpeg", StringComparison.OrdinalIgnoreCase) - || ext.Equals(".gif", StringComparison.OrdinalIgnoreCase) - || ext.Equals(".bmp", StringComparison.OrdinalIgnoreCase) - || ext.Equals(".webp", StringComparison.OrdinalIgnoreCase); + return ext.Equals(".svg", StringComparison.OrdinalIgnoreCase) + || ext.Equals(".tif", StringComparison.OrdinalIgnoreCase) + || ext.Equals(".tiff", StringComparison.OrdinalIgnoreCase) + || ext.Equals(".avif", StringComparison.OrdinalIgnoreCase); } private void Package_PropertyChanged(object? sender, PropertyChangedEventArgs e) diff --git a/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/Interface_PViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/Interface_PViewModel.cs index bc74d87248..c1face9623 100644 --- a/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/Interface_PViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/Interface_PViewModel.cs @@ -35,6 +35,7 @@ private async Task ResetIconCache(Visual? _) try { Directory.Delete(CoreData.UniGetUICacheDirectory_Icons, true); } catch (Exception ex) { Logger.Error(ex); } global::UniGetUI.PackageEngine.PackageClasses.PackageWrapper.ClearIconCache(); + global::UniGetUI.PackageEngine.PackageClasses.Package.ResetIconCache(); RestartRequired?.Invoke(this, EventArgs.Empty); await LoadIconCacheSize(); } diff --git a/src/UniGetUI.Core.IconStore/IconCacheEngine.cs b/src/UniGetUI.Core.IconStore/IconCacheEngine.cs index 2de35b9d34..d4e439df6e 100644 --- a/src/UniGetUI.Core.IconStore/IconCacheEngine.cs +++ b/src/UniGetUI.Core.IconStore/IconCacheEngine.cs @@ -455,7 +455,7 @@ private static void DeteteCachedFiles(string iconLocation) { "image/svg+xml", "svg" }, { "image/vnd.microsoft.icon", "ico" }, { "application/octet-stream", "ico" }, - { "image/image/x-icon", "ico" }, + { "image/x-icon", "ico" }, { "image/tiff", "tif" }, } ); @@ -471,7 +471,7 @@ private static void DeteteCachedFiles(string iconLocation) { "png", "image/png" }, { "webp", "image/webp" }, { "svg", "image/svg+xml" }, - { "ico", "image/image/x-icon" }, + { "ico", "image/x-icon" }, { "tif", "image/tiff" }, } ); diff --git a/src/UniGetUI.PackageEngine.PackageManagerClasses/Packages/Package.cs b/src/UniGetUI.PackageEngine.PackageManagerClasses/Packages/Package.cs index 015dca8aa9..67fb96d52c 100644 --- a/src/UniGetUI.PackageEngine.PackageManagerClasses/Packages/Package.cs +++ b/src/UniGetUI.PackageEngine.PackageManagerClasses/Packages/Package.cs @@ -28,7 +28,14 @@ public partial class Package : IPackage private readonly string _ignoredId; private readonly string _iconId; - private static readonly ConcurrentDictionary _cachedIconPaths = new(); + private static readonly ConcurrentDictionary _cachedIconPaths = new(); + private static readonly ConcurrentDictionary _failedIconLookups = new(); + private static readonly TimeSpan _iconLookupRetryInterval = TimeSpan.FromMinutes(5); + + public static TimeSpan? TEST_IconLookupRetryIntervalOverride { private get; set; } + + private static TimeSpan IconLookupRetryInterval => + TEST_IconLookupRetryIntervalOverride ?? _iconLookupRetryInterval; private IPackageDetails? __details; public IPackageDetails Details @@ -162,12 +169,30 @@ public virtual Uri GetIconUrl() public virtual Uri? GetIconUrlIfAny() { - if (_cachedIconPaths.TryGetValue(this.GetHashCode(), out Uri? path)) + long cacheKey = _versionedHash; + if (_cachedIconPaths.TryGetValue(cacheKey, out Uri? path)) { return path; } + + if ( + _failedIconLookups.TryGetValue(cacheKey, out long failedAt) + && Environment.TickCount64 - failedAt + < (long)IconLookupRetryInterval.TotalMilliseconds + ) + { + return null; + } + var CachedIcon = LoadIconUrlIfAny(); - _cachedIconPaths.TryAdd(this.GetHashCode(), CachedIcon); + if (CachedIcon is null) + { + _failedIconLookups[cacheKey] = Environment.TickCount64; + return null; + } + + _failedIconLookups.TryRemove(cacheKey, out _); + _cachedIconPaths[cacheKey] = CachedIcon; return CachedIcon; } @@ -377,6 +402,7 @@ public SerializableIncompatiblePackage AsSerializable_Incompatible() public static void ResetIconCache() { _cachedIconPaths.Clear(); + _failedIconLookups.Clear(); } private static string GenerateIconId(Package p) diff --git a/src/UniGetUI.PackageEngine.Tests/PackageIconLookupTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageIconLookupTests.cs new file mode 100644 index 0000000000..1bf3b89436 --- /dev/null +++ b/src/UniGetUI.PackageEngine.Tests/PackageIconLookupTests.cs @@ -0,0 +1,148 @@ +using UniGetUI.Core.Data; +using UniGetUI.Core.IconEngine; +using UniGetUI.Core.SettingsEngine; +using UniGetUI.Core.SettingsEngine.SecureSettings; +using UniGetUI.PackageEngine.Interfaces; +using UniGetUI.PackageEngine.PackageClasses; +using UniGetUI.PackageEngine.Tests.Infrastructure.Builders; + +namespace UniGetUI.PackageEngine.Tests; + +public sealed class PackageIconLookupTests : IDisposable +{ + private readonly string _testRoot; + + public PackageIconLookupTests() + { + _testRoot = Path.Combine( + Path.GetTempPath(), + nameof(PackageIconLookupTests), + Guid.NewGuid().ToString("N") + ); + CoreData.TEST_DataDirectoryOverride = Path.Combine(_testRoot, "Data"); + SecureSettings.TEST_SecureSettingsRootOverride = Path.Combine(_testRoot, "SecureSettings"); + Directory.CreateDirectory(CoreData.UniGetUIUserConfigurationDirectory); + Settings.ResetSettings(); + Package.ResetIconCache(); + } + + public void Dispose() + { + Package.TEST_IconLookupRetryIntervalOverride = null; + Package.ResetIconCache(); + Settings.ResetSettings(); + CoreData.TEST_DataDirectoryOverride = null; + SecureSettings.TEST_SecureSettingsRootOverride = null; + if (Directory.Exists(_testRoot)) + Directory.Delete(_testRoot, recursive: true); + } + + [Fact] + public void GetIconUrlIfAny_DoesNotRetryFailedLookupWithinRetryInterval() + { + int lookups = 0; + var package = BuildPackage( + "Contoso.WithinInterval", + _ => + { + lookups++; + return null; + } + ); + + Assert.Null(package.GetIconUrlIfAny()); + Assert.Null(package.GetIconUrlIfAny()); + + Assert.Equal(1, lookups); + } + + [Fact] + public void GetIconUrlIfAny_RetriesFailedLookupAfterRetryInterval() + { + Package.TEST_IconLookupRetryIntervalOverride = TimeSpan.Zero; + string iconPath = CreateIconFile(); + + int lookups = 0; + var package = BuildPackage( + "Contoso.AfterInterval", + _ => + { + lookups++; + return lookups == 1 ? null : new CacheableIcon(iconPath); + } + ); + + Assert.Null(package.GetIconUrlIfAny()); + Uri? retried = package.GetIconUrlIfAny(); + + Assert.Equal(2, lookups); + Assert.NotNull(retried); + Assert.True(retried.IsFile); + Assert.Equal(iconPath, retried.LocalPath); + } + + [Fact] + public void GetIconUrlIfAny_DoesNotResolveResolvedIconAgain() + { + Package.TEST_IconLookupRetryIntervalOverride = TimeSpan.Zero; + string iconPath = CreateIconFile(); + + int lookups = 0; + var package = BuildPackage( + "Contoso.AlreadyResolved", + _ => + { + lookups++; + return new CacheableIcon(iconPath); + } + ); + + Uri? first = package.GetIconUrlIfAny(); + Uri? second = package.GetIconUrlIfAny(); + + Assert.Equal(1, lookups); + Assert.Equal(first, second); + } + + [Fact] + public void ResetIconCache_AllowsFailedLookupToBeRetriedImmediately() + { + int lookups = 0; + var package = BuildPackage( + "Contoso.AfterReset", + _ => + { + lookups++; + return null; + } + ); + + Assert.Null(package.GetIconUrlIfAny()); + Package.ResetIconCache(); + Assert.Null(package.GetIconUrlIfAny()); + + Assert.Equal(2, lookups); + } + + private string CreateIconFile() + { + Directory.CreateDirectory(_testRoot); + string iconPath = Path.Combine(_testRoot, "icon.png"); + File.WriteAllBytes(iconPath, [0x89, 0x50, 0x4E, 0x47]); + return iconPath; + } + + private static Package BuildPackage(string id, Func iconFactory) + { + var manager = new PackageManagerBuilder() + .ConfigureCapabilities(capabilities => + { + capabilities.SupportsCustomPackageIcons = true; + return capabilities; + }) + .ConfigureDetails(details => details.IconFactory = iconFactory) + .Build(); + + return new PackageBuilder().WithId(id).WithManager(manager).Build(); + } +}