From 3dfb59ac8754b59db474415eeb2c0467d5d56ab3 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Mon, 17 Aug 2026 22:39:27 +0200 Subject: [PATCH] Resolve package entries by identity and end walks nobody reads Searching a package resolves its entries by file name against a case-insensitive cache, which is right for an assembly reference but wrong for an archive entry: two entries differing only in case are two files, and they collapsed onto one LoadedAssembly, so one was searched twice and the other never. Keying the cache by the entry itself separates them, and the entry's package-relative path becomes the assembly's file name, which is what tells the copies of one assembly in a multi-target package apart wherever a search result shows a location. Cancellation was only checked between top-level list entries, so a walk the user had already replaced by typing another character kept extracting package entries alongside the run they were waiting for. The omnibar had no way to end its run at all: its view model is per document tab and nothing cancelled it when the tab went away. Assisted-by: Claude:claude-opus-5[1m]:Claude Code --- ICSharpCode.ILSpyX/AssemblyListSnapshot.cs | 11 +-- ICSharpCode.ILSpyX/LoadedPackage.cs | 61 +++++++++++----- .../AssemblyTree/AssemblyTreeModelTests.cs | 72 +++++++++++++++++++ ILSpy/Controls/Omnibar/Omnibar.axaml.cs | 9 +++ ILSpy/Controls/Omnibar/OmnibarViewModel.cs | 14 +++- ILSpy/Search/RunningSearch.cs | 12 +--- ILSpy/TreeNodes/PackageFolderTreeNode.cs | 2 +- 7 files changed, 146 insertions(+), 35 deletions(-) diff --git a/ICSharpCode.ILSpyX/AssemblyListSnapshot.cs b/ICSharpCode.ILSpyX/AssemblyListSnapshot.cs index 5c520fc83..253c4174d 100644 --- a/ICSharpCode.ILSpyX/AssemblyListSnapshot.cs +++ b/ICSharpCode.ILSpyX/AssemblyListSnapshot.cs @@ -207,7 +207,7 @@ namespace ICSharpCode.ILSpyX } else if (result.Package != null) { - foreach (var descendant in EnumerateDescendants(result.Package.RootFolder)) + foreach (var descendant in EnumerateDescendants(result.Package.RootFolder, cancellationToken)) { yield return descendant; } @@ -218,11 +218,11 @@ namespace ICSharpCode.ILSpyX } } - static IEnumerable EnumerateDescendants(PackageFolder folder) + static IEnumerable EnumerateDescendants(PackageFolder folder, CancellationToken cancellationToken) { foreach (var subFolder in folder.Folders) { - foreach (var descendant in EnumerateDescendants(subFolder)) + foreach (var descendant in EnumerateDescendants(subFolder, cancellationToken)) { yield return descendant; } @@ -230,12 +230,15 @@ namespace ICSharpCode.ILSpyX foreach (var entry in folder.Entries) { + // Checked per entry, not per package: resolving one starts extracting it from + // the archive, so a walk the consumer has given up on must stop expanding. + cancellationToken.ThrowIfCancellationRequested(); if (!entry.Name.EndsWith(".dll", StringComparison.OrdinalIgnoreCase) && !entry.Name.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) continue; LoadedAssembly? asm; try { - asm = folder.ResolveFileName(entry.Name); + asm = folder.ResolveEntry(entry); } catch { diff --git a/ICSharpCode.ILSpyX/LoadedPackage.cs b/ICSharpCode.ILSpyX/LoadedPackage.cs index 84793c800..dfeecf797 100644 --- a/ICSharpCode.ILSpyX/LoadedPackage.cs +++ b/ICSharpCode.ILSpyX/LoadedPackage.cs @@ -349,35 +349,58 @@ namespace ICSharpCode.ILSpyX return Task.FromResult(null); } - readonly Dictionary assemblies = new Dictionary(StringComparer.OrdinalIgnoreCase); + // Keyed by entry name, ordinal: an assembly-reference lookup is case-insensitive, but two + // entries whose names differ only in case are two distinct files (archive entry names are + // case-sensitive) and must not share one LoadedAssembly. + readonly Dictionary assemblies = new Dictionary(StringComparer.Ordinal); public LoadedAssembly? ResolveFileName(string name) { + var entry = Entries.FirstOrDefault(e => string.Equals(name, e.Name, StringComparison.Ordinal)) + ?? Entries.FirstOrDefault(e => string.Equals(name, e.Name, StringComparison.OrdinalIgnoreCase)); + return entry == null ? null : ResolveEntry(entry); + } + + /// + /// The for one of this folder's entries, created on first use. + /// Creating it starts reading the entry out of the package, so call this only for entries + /// that are about to be inspected. + /// + public LoadedAssembly? ResolveEntry(PackageEntry entry) + { + ArgumentNullException.ThrowIfNull(entry); if (package.LoadedAssembly == null) return null; lock (assemblies) { - if (assemblies.TryGetValue(name, out var asm)) + if (assemblies.TryGetValue(entry.Name, out var asm)) return asm; - var entry = Entries.FirstOrDefault(e => string.Equals(name, e.Name, StringComparison.OrdinalIgnoreCase)); - if (entry != null) - { - asm = new LoadedAssembly( - package.LoadedAssembly, entry.Name, - fileLoaders: package.LoadedAssembly.AssemblyList.LoaderRegistry, - assemblyResolver: this, - stream: Task.Run(entry.TryOpenStream), - applyWinRTProjections: package.LoadedAssembly.AssemblyList.ApplyWinRTProjections, - useDebugSymbols: package.LoadedAssembly.AssemblyList.UseDebugSymbols - ); - } - else - { - asm = null; - } - assemblies.Add(name, asm); + // FullName is the package-relative path ("lib/net10.0/Foo.dll"), which is what makes + // the copies of one assembly in a multi-target package tellable apart wherever the + // file name is surfaced. ShortName/Text stay the bare file name either way. + asm = new LoadedAssembly( + package.LoadedAssembly, entry.FullName, + fileLoaders: package.LoadedAssembly.AssemblyList.LoaderRegistry, + assemblyResolver: this, + stream: Task.Run(entry.TryOpenStream), + applyWinRTProjections: package.LoadedAssembly.AssemblyList.ApplyWinRTProjections, + useDebugSymbols: package.LoadedAssembly.AssemblyList.UseDebugSymbols + ); + assemblies.Add(entry.Name, asm); return asm; } } + + /// + /// Whether this folder has already resolved from one of its + /// entries. Consults the resolution cache only -- nothing is loaded or extracted. + /// + public bool HasResolved(LoadedAssembly assembly) + { + lock (assemblies) + { + return assemblies.ContainsValue(assembly); + } + } } } diff --git a/ILSpy.Tests/AssemblyTree/AssemblyTreeModelTests.cs b/ILSpy.Tests/AssemblyTree/AssemblyTreeModelTests.cs index ceda979af..f1629c0ce 100644 --- a/ILSpy.Tests/AssemblyTree/AssemblyTreeModelTests.cs +++ b/ILSpy.Tests/AssemblyTree/AssemblyTreeModelTests.cs @@ -17,9 +17,11 @@ // DEALINGS IN THE SOFTWARE. using System; +using System.Collections.Generic; using System.IO; using System.IO.Compression; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Avalonia.Headless.NUnit; @@ -71,6 +73,76 @@ public class AssemblyTreeModelTests "Initialize selects the (Default) list so the tree has something to render at startup."); } + static readonly string TempRoot = Path.Combine(Path.GetTempPath(), "ILSpy.Tests.Packages", Guid.NewGuid().ToString("N")); + + // Two assemblies in sibling folder chains, so a walk that reaches only one of them fails. + static string CreatePackage() + { + var dir = Directory.CreateDirectory(Path.Combine(TempRoot, Guid.NewGuid().ToString("N"))).FullName; + var zipPath = Path.Combine(dir, "package.zip"); + using var zip = ZipFile.Open(zipPath, ZipArchiveMode.Create); + zip.CreateEntryFromFile(FixtureAssembly.Emit("Nested"), "lib/net10.0/Nested.dll"); + zip.CreateEntryFromFile(FixtureAssembly.Emit("Sibling"), "runtimes/win-x64/Sibling.dll"); + return zipPath; + } + + [OneTimeTearDown] + public void DeleteTempPackages() + { + if (Directory.Exists(TempRoot)) + Directory.Delete(TempRoot, recursive: true); + } + + [AvaloniaTest] + public async Task EnumerateAllAssemblies_expands_a_package_into_its_entries() + { + // The search corpus is this walk, so an assembly it does not yield is an assembly no + // search can ever match. + var (_, vm) = await TestHarness.BootAsync(); + await vm.OpenAssemblyAsync(CreatePackage()); + + var nested = new List(); + await foreach (var asm in vm.AssemblyTreeModel.AssemblyList!.EnumerateAllAssemblies()) + { + if (asm.ParentBundle != null) + nested.Add(asm); + } + + nested.Select(a => a.FileName).Should().BeEquivalentTo( + new[] { "lib/net10.0/Nested.dll", "runtimes/win-x64/Sibling.dll" }, + "every .dll in the package is searchable, and the package-relative path is what " + + "distinguishes copies of one assembly built for several targets."); + } + + [AvaloniaTest] + public async Task EnumerateAllAssemblies_stops_expanding_a_package_once_cancelled() + { + // Both search panes restart on every keystroke, so an abandoned walk that keeps + // extracting package entries competes with the run the user is waiting for. + var (_, vm) = await TestHarness.BootAsync(); + await vm.OpenAssemblyAsync(CreatePackage()); + + using var cts = new CancellationTokenSource(); + var walk = vm.AssemblyTreeModel.AssemblyList!.EnumerateAllAssemblies(cts.Token).GetAsyncEnumerator(); + try + { + while (await walk.MoveNextAsync() && walk.Current.ParentBundle == null) + { + // Skip past the list's own assemblies to the package's first entry. + } + walk.Current.ParentBundle.Should().NotBeNull("the package's entries come last on the list."); + cts.Cancel(); + + var next = async () => await walk.MoveNextAsync(); + await next.Should().ThrowAsync( + "the second entry must not be extracted after the walk was cancelled."); + } + finally + { + await walk.DisposeAsync(); + } + } + [AvaloniaTest] public async Task FindTreeNode_resolves_a_type_inside_an_unexpanded_package() { diff --git a/ILSpy/Controls/Omnibar/Omnibar.axaml.cs b/ILSpy/Controls/Omnibar/Omnibar.axaml.cs index c194d05c4..5deac32ff 100644 --- a/ILSpy/Controls/Omnibar/Omnibar.axaml.cs +++ b/ILSpy/Controls/Omnibar/Omnibar.axaml.cs @@ -21,6 +21,7 @@ using System.Collections.Specialized; using System.ComponentModel; using System.Linq; +using Avalonia; using Avalonia.Controls; using Avalonia.Input; using Avalonia.Threading; @@ -69,6 +70,14 @@ namespace ICSharpCode.ILSpy.Controls.Omnibar }); } + protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e) + { + // The hosting tab is gone (closed, or its content swapped out). Nothing will read the + // results, so end the run rather than let it walk the rest of the assembly list. + viewModel.CancelSearch(); + base.OnDetachedFromVisualTree(e); + } + void OnViewModelPropertyChanged(object? sender, PropertyChangedEventArgs e) { if (e.PropertyName == nameof(OmnibarViewModel.Mode)) diff --git a/ILSpy/Controls/Omnibar/OmnibarViewModel.cs b/ILSpy/Controls/Omnibar/OmnibarViewModel.cs index 3486429f0..33ab000cb 100644 --- a/ILSpy/Controls/Omnibar/OmnibarViewModel.cs +++ b/ILSpy/Controls/Omnibar/OmnibarViewModel.cs @@ -191,13 +191,23 @@ namespace ICSharpCode.ILSpy.Controls.Omnibar RunningSearch? currentSearch; - void RestartSearch() + /// + /// Ends the search in flight, if any. A run holds the assembly list and keeps expanding + /// packages until it finishes, so a bar that is going away has to stop its run instead of + /// leaving it to drain into a sink nobody can see. + /// + public void CancelSearch() { currentSearch?.Cancel(); currentSearch = null; + IsSearching = false; + } + + void RestartSearch() + { + CancelSearch(); searchSink.Clear(); Suggestions.Clear(); - IsSearching = false; var term = SearchText ?? string.Empty; if (string.IsNullOrWhiteSpace(term)) diff --git a/ILSpy/Search/RunningSearch.cs b/ILSpy/Search/RunningSearch.cs index 2801571fd..66cc06251 100644 --- a/ILSpy/Search/RunningSearch.cs +++ b/ILSpy/Search/RunningSearch.cs @@ -131,15 +131,9 @@ namespace ICSharpCode.ILSpy.Search var strategy = GetStrategy(request); if (strategy == null) return; - // Streaming enumeration: expanding bundles/packages into their contained - // assemblies triggers the lazy load of every entry on the list, so awaiting - // the whole set up front would mean no results at all until the last - // assembly is off disk. Enumerating starts the walk here on the worker - // thread, not at construction time on the UI thread. - // - // Serial walk: per-assembly metadata walk is allocation-dominated, and 4 - // parallel producers fighting for the ConcurrentQueue + the resulting UI - // batching jitter end up slower than serial in practice. + // The per-assembly metadata walk is allocation-dominated, and 4 parallel + // producers fighting for the ConcurrentQueue + the resulting UI batching + // jitter end up slower than walking the assemblies one at a time. await foreach (var assembly in assemblyList.EnumerateAllAssemblies(ct).ConfigureAwait(false)) { if (ct.IsCancellationRequested) diff --git a/ILSpy/TreeNodes/PackageFolderTreeNode.cs b/ILSpy/TreeNodes/PackageFolderTreeNode.cs index 08891b055..fe3b2f6d9 100644 --- a/ILSpy/TreeNodes/PackageFolderTreeNode.cs +++ b/ILSpy/TreeNodes/PackageFolderTreeNode.cs @@ -88,7 +88,7 @@ namespace ICSharpCode.ILSpy.TreeNodes if (entry.Name.EndsWith(".dll", StringComparison.OrdinalIgnoreCase) || entry.Name.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) { - var asm = root.ResolveFileName(entry.Name); + var asm = root.ResolveEntry(entry); if (asm != null) { yield return new AssemblyTreeNode(asm, entry);