Browse Source

Merge pull request #4099 from icsharpcode/fix/3290-lazy-children-marshalling

Fix #3290: marshal EnsureLazyChildren onto the tree's owning thread
pull/4104/head
Siegfried Pammer 2 weeks ago committed by GitHub
parent
commit
7465589836
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
  1. 15
      ICSharpCode.ILSpyX/TreeView/FlatListTreeNode.cs
  2. 33
      ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs
  3. 5
      ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs
  4. 298
      ICSharpCode.ILSpyX/TreeView/TreeThreadAffinity.cs
  5. 72
      ILSpy.Tests/Controls/FlatListTreeNodeTests.cs
  6. 121
      ILSpy.Tests/Controls/SharpTreeViewTests.cs
  7. 444
      ILSpy.Tests/Controls/TreeThreadAffinityTests.cs
  8. 67
      ILSpy.Tests/TreeThreadAffinityGuard.cs
  9. 10
      ILSpy/Controls/TreeView/SharpTreeView.cs
  10. 5
      ILSpy/TreeNodes/ReferenceFolderTreeNode.cs
  11. 4
      ILSpy/TreeNodes/ResourceListTreeNode.cs

15
ICSharpCode.ILSpyX/TreeView/FlatListTreeNode.cs

@ -39,7 +39,7 @@ namespace ICSharpCode.ILSpyX.TreeView @@ -39,7 +39,7 @@ namespace ICSharpCode.ILSpyX.TreeView
byte height = 1;
/// <summary>Length in the flat list, including children (children within the flat list). -1 = invalidated</summary>
int totalListLength = -1;
internal int totalListLength = -1;
int Balance {
get { return Height(right) - Height(left); }
@ -106,8 +106,16 @@ namespace ICSharpCode.ILSpyX.TreeView @@ -106,8 +106,16 @@ namespace ICSharpCode.ILSpyX.TreeView
root.GetTotalListLength(); // ensure all list lengths are calculated
Debug.Assert(index >= 0);
Debug.Assert(index < root.totalListLength);
int originalIndex = index;
SharpTreeNode node = root;
while (true)
// Falling out of the descent means the augmented lengths describe more visible nodes
// than the tree actually holds, which is what a structural mutation racing this walk
// leaves behind (see TreeThreadAffinity). This is a mitigation, not a fix: it turns a
// NullReferenceException with no context into a report of the state that produced it,
// and it cannot make the read correct. Note it does not detect a stale index either -
// collapsing a node restructures the flat list without changing totalListLength, so the
// assert above stays satisfied while the structure moves underneath.
while (node != null)
{
if (node.left != null && index < node.left.totalListLength)
{
@ -128,6 +136,9 @@ namespace ICSharpCode.ILSpyX.TreeView @@ -128,6 +136,9 @@ namespace ICSharpCode.ILSpyX.TreeView
node = node.right;
}
}
throw new InvalidOperationException(
$"The flat list tree ran out of nodes while looking up visible index {originalIndex}; "
+ $"totalListLength is {root.totalListLength}. The tree was restructured while it was being read.");
}
internal static int GetVisibleIndexForNode(SharpTreeNode node)

33
ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs

@ -23,6 +23,7 @@ using System.ComponentModel; @@ -23,6 +23,7 @@ using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using ICSharpCode.ILSpyX.TreeView.PlatformAbstractions;
@ -161,6 +162,7 @@ namespace ICSharpCode.ILSpyX.TreeView @@ -161,6 +162,7 @@ namespace ICSharpCode.ILSpyX.TreeView
set {
if (isHidden != value)
{
VerifyAccess(nameof(IsHidden));
isHidden = value;
if (modelParent != null)
UpdateIsVisible(modelParent.isVisible && modelParent.isExpanded, true);
@ -287,6 +289,7 @@ namespace ICSharpCode.ILSpyX.TreeView @@ -287,6 +289,7 @@ namespace ICSharpCode.ILSpyX.TreeView
set {
if (isExpanded != value)
{
VerifyAccess(nameof(IsExpanded));
isExpanded = value;
if (isExpanded)
{
@ -363,10 +366,38 @@ namespace ICSharpCode.ILSpyX.TreeView @@ -363,10 +366,38 @@ namespace ICSharpCode.ILSpyX.TreeView
}
/// <summary>
/// Ensures the children were initialized (loads children if lazy loading is enabled)
/// Ensures the children were initialized (loads children if lazy loading is enabled).
/// </summary>
/// <remarks>
/// Realizing children mutates a tree that a UI may be indexing concurrently, so on a tree
/// that has an owner with a way onto it (see <see cref="SetOwner(Thread, Action{Action})"/>)
/// this marshals itself there rather than relying on every caller to remember: the app
/// realizes children from background decompile tasks in several places, and a caller that
/// forgets corrupts the flat list.
///
/// A call that is already on the owning thread runs inline, which is what keeps a blocking
/// invoke from deadlocking on itself and keeps nesting cheap: once the outermost call has
/// marshalled, everything <see cref="LoadChildren"/> triggers below it is already on the
/// owner and hops no further.
///
/// A tree with no owner is not marshalled: building a subtree on a worker and publishing it
/// on the UI thread is a legitimate pattern, and nothing is displaying that subtree yet.
/// </remarks>
public void EnsureLazyChildren()
{
if (!LazyLoading)
return;
SharpTreeNode? ownerNode = OwnerNode;
if (ownerNode?.ownerInvoke is { } invoke && ownerNode.owner != Thread.CurrentThread)
invoke(LoadLazyChildren);
else
LoadLazyChildren();
}
void LoadLazyChildren()
{
// Re-checked after marshalling: the owning thread may have realized the children while
// the calling thread was queued behind it.
if (LazyLoading)
{
LazyLoading = false;

5
ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs

@ -45,6 +45,11 @@ namespace ICSharpCode.ILSpyX.TreeView @@ -45,6 +45,11 @@ namespace ICSharpCode.ILSpyX.TreeView
void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
{
// Checked here rather than in each mutator: every one of them funnels through this method,
// and this is the last point before OnChildrenChanged rewrites the flat-list tree and
// notifies the flattener. A violation therefore leaves the flat list untouched instead of
// half-updated; only the backing List<T> has already moved.
parent.VerifyChildrenChange(e);
Debug.Assert(!isRaisingEvent);
isRaisingEvent = true;
try

298
ICSharpCode.ILSpyX/TreeView/TreeThreadAffinity.cs

@ -0,0 +1,298 @@ @@ -0,0 +1,298 @@
// Copyright (c) 2026 Siegfried Pammer
//
// Permission is hereby granted, free of charge, to any person obtaining a copy of this
// software and associated documentation files (the "Software"), to deal in the Software
// without restriction, including without limitation the rights to use, copy, modify, merge,
// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
// to whom the Software is furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all copies or
// substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading;
namespace ICSharpCode.ILSpyX.TreeView
{
// Thread affinity of the tree model.
//
// The model is not thread-safe. TreeFlattener.Count and SharpTreeNode.GetNodeByVisibleIndex
// both read the augmented 'totalListLength' fields, so a structural mutation racing a read can
// hand out an index for a node that no longer exists; issue #3290 is a NullReferenceException
// inside GetNodeByVisibleIndex that is unreachable single-threaded. The rule is that a tree
// displayed by a UI is mutated only from that UI's thread, but ICSharpCode.ILSpyX is
// host-agnostic and must not name a dispatcher, so ownership is stated explicitly instead:
// a host calls SetOwner() on the root of a tree it takes over, passing the thread and the way
// to get onto it.
//
// Ownership carries the rule two ways. EnsureLazyChildren uses the invoke delegate to move
// itself onto the owning thread, so a caller cannot get it wrong; and, in debug builds, every
// other structural mutation is verified against the owning thread so a call site that bypasses
// the rule is named instead of silently corrupting the flat list.
partial class SharpTreeNode
{
Thread? owner;
Action<Action>? ownerInvoke;
/// <summary>
/// The nearest node on the model-parent chain that carries an explicit owner, or null when
/// nothing on the chain has been claimed.
/// </summary>
/// <remarks>
/// Resolving the owner by walking up instead of stamping every node gives the propagation
/// rules for free:
/// <list type="bullet">
/// <item>Ownership covers the whole subtree below the node it was set on, so a single call on
/// the root of a displayed tree protects everything in it.</item>
/// <item>Children added later inherit it, with no bookkeeping at insertion time.</item>
/// <item>A subtree built on a background thread carries no owner and is therefore unchecked
/// while it is being built. It inherits the owner the instant it is attached below an owned
/// parent - and that attachment is itself a mutation of the owned tree, so it is checked.</item>
/// </list>
/// </remarks>
SharpTreeNode? OwnerNode {
get {
for (SharpTreeNode? node = this; node != null; node = node.modelParent)
{
if (node.owner != null)
return node;
}
return null;
}
}
Thread? EffectiveOwner => OwnerNode?.owner;
/// <summary>
/// Declares <paramref name="owner"/> as the only thread allowed to structurally mutate this
/// node and its subtree.
/// </summary>
/// <param name="invoke">
/// Runs an action on <paramref name="owner"/> and blocks until it has completed - the host's
/// dispatcher invoke. Supplying it lets <see cref="EnsureLazyChildren"/> marshal itself
/// instead of every caller having to know it must. Null leaves the tree unmarshalled, which
/// only makes sense for a tree no UI is displaying.
/// </param>
/// <remarks>
/// Re-owning is allowed - handing a tree over is exactly what this exists for - but the
/// handoff must be performed by the thread that currently owns the tree, because a
/// background thread taking ownership of a live tree away from the UI is the very race the
/// affinity check hunts for.
/// </remarks>
public void SetOwner(Thread owner, Action<Action>? invoke = null)
{
VerifyAccess(nameof(SetOwner));
this.owner = owner;
this.ownerInvoke = invoke;
}
/// <summary>
/// Declares the calling thread as the only thread allowed to structurally mutate this node
/// and its subtree, without a way to marshal onto it.
/// </summary>
public void SetOwner() => SetOwner(Thread.CurrentThread);
/// <summary>
/// Reports a violation if the calling thread is not the effective owner of this node.
/// </summary>
[Conditional("DEBUG")]
internal void VerifyAccess(string operation)
{
#if DEBUG
Thread? expected = EffectiveOwner;
if (expected == null || expected == Thread.CurrentThread)
return;
TreeThreadAffinity.Report(this, operation, expected);
#endif
}
/// <summary>
/// Verifies a pending change to this node's <see cref="Children"/> collection.
/// </summary>
[Conditional("DEBUG")]
internal void VerifyChildrenChange(NotifyCollectionChangedEventArgs e)
{
#if DEBUG
VerifyAccess("Children." + e.Action);
if (e.NewItems == null)
return;
// An already-owned subtree being attached below a differently-owned parent would keep its
// own owner, so the two halves of one displayed tree would demand two different threads.
// That is a mistake worth naming, but only once: the incoming owner is dropped afterwards
// so the subtree inherits the parent's, instead of reporting again on every later mutation.
Thread? parentOwner = EffectiveOwner;
foreach (SharpTreeNode node in e.NewItems)
{
if (node.owner != null && node.owner != parentOwner)
{
TreeThreadAffinity.Report(node, "attach of a subtree owned by another thread", node.owner);
node.owner = null;
node.ownerInvoke = null;
}
}
#endif
}
}
/// <summary>
/// Collects <see cref="SharpTreeNode"/> thread-affinity violations. Nothing here is reached in
/// release builds: every caller is <c>[Conditional("DEBUG")]</c>.
/// </summary>
public static class TreeThreadAffinity
{
/// <summary>
/// Environment variable selecting the log file. Defaults to ILSpy.TreeAffinity.log in the
/// temp directory; set it to an empty string to disable file logging.
/// </summary>
public const string LogFileVariable = "ILSPY_TREE_AFFINITY_LOG";
/// <summary>
/// Environment variable enabling <see cref="FailFast"/> at startup (set it to 1).
/// </summary>
public const string FailFastVariable = "ILSPY_TREE_AFFINITY_FAILFAST";
static readonly ConcurrentDictionary<string, TreeThreadAffinityViolation> violations = new();
static readonly object logLock = new();
/// <summary>
/// When true, a violation throws after being recorded, so a debugger stops at the offending
/// frame. It is not what makes a violation observable - <see cref="Violations"/> is recorded
/// either way, because the throw may well be swallowed by a caller that catches Exception.
/// An exploratory session over a large corpus leaves this off so one bad call site does not
/// interfere with the rest of the run.
/// </summary>
public static bool FailFast { get; set; } = Environment.GetEnvironmentVariable(FailFastVariable) == "1";
/// <summary>
/// The file violations are appended to, or null when file logging is disabled.
/// </summary>
public static string? LogFilePath { get; set; } = ResolveLogFilePath();
static string? ResolveLogFilePath()
{
string? configured = Environment.GetEnvironmentVariable(LogFileVariable);
if (configured == null)
return Path.Combine(Path.GetTempPath(), "ILSpy.TreeAffinity.log");
return configured.Length == 0 ? null : configured;
}
/// <summary>
/// The distinct violating call sites recorded so far, most frequent first.
/// </summary>
public static IReadOnlyList<TreeThreadAffinityViolation> Violations
=> violations.Values.OrderByDescending(v => v.Count).ToList();
public static void Clear() => violations.Clear();
internal static void Report(SharpTreeNode node, string operation, Thread expected)
{
// Skip Report and the [Conditional] wrapper that called it, so frame 0 is the mutation.
string stackTrace = new StackTrace(2, fNeedFileInfo: true).ToString();
var violation = new TreeThreadAffinityViolation(Describe(node), operation, expected, Thread.CurrentThread, stackTrace);
Debug.WriteLine(violation.ToString());
// Deduplicate by call site: a mutation inside a loop must not produce one entry per
// iteration. The first occurrence is written through immediately so a long-running
// session can be inspected without stopping the process; repeats only bump the count.
var recorded = violations.GetOrAdd(stackTrace, violation);
if (recorded.Hit() == 1)
AppendToLog(recorded.ToString());
// Recorded first, thrown second. The throw is only a convenience for stopping a
// debugger at the offending frame; it is not the record. Tree mutation happens inside
// callers that catch Exception (the background decompile writes the failure into the
// text view), so a throw alone would be swallowed and a fail-fast run would come back
// green while violating. Violations is what a test or a session asserts on.
if (FailFast)
throw new InvalidOperationException(violation.ToString());
}
static string Describe(SharpTreeNode node)
{
string text;
try
{
// ToString() is what the model itself already uses for diagnostics. It can still fail
// here: this runs on a thread the node did not expect, which is the whole problem.
text = node.ToString() ?? "<null>";
}
catch (Exception ex)
{
text = "<ToString() threw " + ex.GetType().Name + ">";
}
return node.GetType().FullName + " \"" + text + "\"";
}
static void AppendToLog(string text)
{
string? path = LogFilePath;
if (path == null)
return;
try
{
lock (logLock)
{
File.AppendAllText(path, text + Environment.NewLine + Environment.NewLine);
}
}
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
{
// A diagnostic that cannot write its log must not take the session down with it.
}
}
}
/// <summary>
/// One distinct call site that mutated a tree from a thread other than its owner.
/// </summary>
public sealed class TreeThreadAffinityViolation
{
int count;
internal TreeThreadAffinityViolation(string node, string operation, Thread expected, Thread actual, string stackTrace)
{
this.Node = node;
this.Operation = operation;
this.ExpectedThread = DescribeThread(expected);
this.ActualThread = DescribeThread(actual);
this.StackTrace = stackTrace;
}
public string Node { get; }
public string Operation { get; }
public string ExpectedThread { get; }
public string ActualThread { get; }
public string StackTrace { get; }
/// <summary>Number of times this call site has been hit.</summary>
public int Count => count;
internal int Hit() => Interlocked.Increment(ref count);
static string DescribeThread(Thread thread)
=> thread.ManagedThreadId + " \"" + (thread.Name ?? "<unnamed>") + "\"";
public override string ToString()
{
var b = new StringBuilder();
b.Append("SharpTreeNode thread affinity violation: ").Append(Operation).AppendLine();
b.Append(" node: ").AppendLine(Node);
b.Append(" expected: thread ").AppendLine(ExpectedThread);
b.Append(" actual: thread ").AppendLine(ActualThread);
b.Append(StackTrace);
return b.ToString();
}
}
}

72
ILSpy.Tests/Controls/FlatListTreeNodeTests.cs

@ -0,0 +1,72 @@ @@ -0,0 +1,72 @@
// Copyright (c) 2026 Siegfried Pammer
//
// Permission is hereby granted, free of charge, to any person obtaining a copy of this
// software and associated documentation files (the "Software"), to deal in the Software
// without restriction, including without limitation the rights to use, copy, modify, merge,
// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
// to whom the Software is furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all copies or
// substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
using System;
using AwesomeAssertions;
using ICSharpCode.ILSpyX.TreeView;
using NUnit.Framework;
namespace ICSharpCode.ILSpy.Tests.Controls;
[TestFixture]
public class FlatListTreeNodeTests
{
sealed class TestNode : SharpTreeNode
{
readonly string text;
public TestNode(string text) => this.text = text;
public override object Text => text;
public override string ToString() => text;
}
[Test]
public void GetNodeByVisibleIndex_WalkingPastTheEnd_ThrowsNamingIndexAndLength()
{
var root = new TestNode("root");
root.Children.Add(new TestNode("child"));
root.IsExpanded = true;
var listRoot = root.GetListRoot();
listRoot.GetTotalListLength().Should().Be(2);
// A restructure that happened under a reader leaves the augmented length disagreeing with
// the structure it describes: the length says there is a node at this index, the descent
// runs out of nodes before reaching it.
listRoot.totalListLength = 5;
var error = Assert.Throws<InvalidOperationException>(
() => SharpTreeNode.GetNodeByVisibleIndex(listRoot, 4));
error!.Message.Should().Contain("4").And.Contain("5");
}
[Test]
public void GetNodeByVisibleIndex_WithinTheList_ReturnsTheNodeAtThatIndex()
{
var root = new TestNode("root");
var child = new TestNode("child");
root.Children.Add(child);
root.IsExpanded = true;
var listRoot = root.GetListRoot();
Assert.That(SharpTreeNode.GetNodeByVisibleIndex(listRoot, 0), Is.SameAs(root));
Assert.That(SharpTreeNode.GetNodeByVisibleIndex(listRoot, 1), Is.SameAs(child));
}
}

121
ILSpy.Tests/Controls/SharpTreeViewTests.cs

@ -151,4 +151,125 @@ public class SharpTreeViewTests @@ -151,4 +151,125 @@ public class SharpTreeViewTests
c.IsSelected.Should().BeTrue();
a.IsSelected.Should().BeFalse("moving the selection clears the old node's flag");
}
/// <summary>The number of rows the flattener should expose for a tree rooted in
/// <paramref name="root"/> when the root itself is not shown.</summary>
static int VisibleRowCount(SharpTreeNode root)
{
int count = 0;
void Walk(SharpTreeNode node)
{
foreach (SharpTreeNode child in node.Children)
{
if (child.IsHidden)
continue;
count++;
if (child.IsExpanded)
Walk(child);
}
}
Walk(root);
return count;
}
static void AssertEveryRowResolves(SharpTreeView tree, SharpTreeNode root, string because)
{
tree.UpdateLayout();
Dispatcher.UIThread.RunJobs();
int expected = VisibleRowCount(root);
tree.ItemCount.Should().Be(expected, because);
for (int i = 0; i < expected; i++)
tree.ItemsView[i].Should().NotBeNull($"row {i} must resolve after {because}");
}
/// <summary>
/// Collapsing and removing nodes above a scrolled viewport shrinks the flattened list under
/// the virtualizing panel's realized index range. The panel must not be left indexing rows
/// that no longer exist.
/// </summary>
[AvaloniaTest]
public void Shrinking_The_Tree_Above_A_Scrolled_Viewport_Keeps_Every_Row_Resolvable()
{
var groups = Enumerable.Range(0, 200)
.Select(i => new TestNode($"g{i}", Enumerable.Range(0, 5)
.Select(j => new TestNode($"g{i}_{j}")).ToArray()))
.ToArray();
var root = new TestNode("root", groups);
var tree = new SharpTreeView { ShowRoot = false, Root = root };
var window = new Window { Content = tree, Width = 300, Height = 400 };
window.Show();
Dispatcher.UIThread.RunJobs();
foreach (var group in groups)
group.IsExpanded = true;
AssertEveryRowResolves(tree, root, "expanding every group");
tree.GetRealizedContainers().Count().Should()
.BeLessThan(tree.ItemCount, "the panel must be virtualizing, or this test proves nothing");
// Park the realized range far from index 0, with a live selection inside it.
tree.SelectedItem = groups[^1].Children[^1];
tree.ScrollIntoView(tree.ItemCount - 1);
AssertEveryRowResolves(tree, root, "scrolling to the last row");
for (int i = 0; i < 190; i++)
groups[i].IsExpanded = false;
AssertEveryRowResolves(tree, root, "collapsing 190 groups above the viewport");
for (int i = 0; i < 150; i++)
root.Children.RemoveAt(0);
AssertEveryRowResolves(tree, root, "removing 150 groups above the viewport");
tree.ScrollIntoView(0);
AssertEveryRowResolves(tree, root, "scrolling back to the top");
}
/// <summary>
/// A lazily loaded node replaces its placeholder with real children while the tree is
/// scrolled: the row count grows and shrinks in the same gesture.
/// </summary>
[AvaloniaTest]
public void Lazy_Loading_Under_A_Scrolled_Viewport_Keeps_Every_Row_Resolvable()
{
var lazy = Enumerable.Range(0, 100).Select(i => new LazyNode($"l{i}", 7)).ToArray();
var root = new TestNode("root");
foreach (var node in lazy)
root.Children.Add(node);
var tree = new SharpTreeView { ShowRoot = false, Root = root };
var window = new Window { Content = tree, Width = 300, Height = 400 };
window.Show();
Dispatcher.UIThread.RunJobs();
AssertEveryRowResolves(tree, root, "the initial collapsed list");
tree.ScrollIntoView(tree.ItemCount - 1);
AssertEveryRowResolves(tree, root, "scrolling to the last row");
foreach (var node in lazy)
{
node.IsExpanded = true;
AssertEveryRowResolves(tree, root, $"lazily expanding {node.Text}");
}
foreach (var node in lazy)
node.ReloadChildren();
AssertEveryRowResolves(tree, root, "reloading every lazy subtree in place");
}
sealed class LazyNode : SharpTreeNode
{
readonly string text;
readonly int childCount;
public LazyNode(string text, int childCount)
{
this.text = text;
this.childCount = childCount;
LazyLoading = true;
}
public override object Text => text;
protected override void LoadChildren()
{
for (int i = 0; i < childCount; i++)
Children.Add(new TestNode($"{text}_{i}"));
}
}
}

444
ILSpy.Tests/Controls/TreeThreadAffinityTests.cs

@ -0,0 +1,444 @@ @@ -0,0 +1,444 @@
// Copyright (c) 2026 Siegfried Pammer
//
// Permission is hereby granted, free of charge, to any person obtaining a copy of this
// software and associated documentation files (the "Software"), to deal in the Software
// without restriction, including without limitation the rights to use, copy, modify, merge,
// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
// to whom the Software is furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all copies or
// substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
using System;
using System.Collections.Concurrent;
using System.Runtime.ExceptionServices;
using System.Threading;
using AwesomeAssertions;
using ICSharpCode.ILSpyX.TreeView;
using NUnit.Framework;
namespace ICSharpCode.ILSpy.Tests.Controls;
[TestFixture]
public class TreeThreadAffinityTests
{
sealed class TestNode : SharpTreeNode
{
readonly string text;
public TestNode(string text) => this.text = text;
public override object Text => text;
public override string ToString() => text;
}
/// <summary>
/// A lazy node that records where and how often its children were built, plus an optional hook
/// so a test can drive whatever LoadChildren would do in the app.
/// </summary>
sealed class LazyTestNode : SharpTreeNode
{
readonly string text;
public LazyTestNode(string text)
{
this.text = text;
LazyLoading = true;
}
public Action<LazyTestNode>? OnLoadChildren { get; set; }
public Thread? LoadChildrenThread { get; private set; }
public int LoadChildrenCount { get; private set; }
protected override void LoadChildren()
{
LoadChildrenThread = Thread.CurrentThread;
LoadChildrenCount++;
OnLoadChildren?.Invoke(this);
}
public override object Text => text;
public override string ToString() => text;
}
/// <summary>
/// A stand-in for the UI thread: a thread with a work queue, whose <see cref="Invoke"/> has the
/// same contract as the host dispatcher's - run the action there, block until it is done, and
/// run it inline when the caller already is that thread.
/// </summary>
sealed class OwnerThread : IDisposable
{
readonly BlockingCollection<Action> queue = new();
readonly Thread thread;
int invokeCount;
public OwnerThread()
{
thread = new Thread(() => {
foreach (var work in queue.GetConsumingEnumerable())
work();
}) { Name = "affinity-test-owner", IsBackground = true };
thread.Start();
}
public Thread Thread => thread;
/// <summary>Number of calls that actually had to be marshalled.</summary>
public int InvokeCount => invokeCount;
public void Invoke(Action action)
{
if (Thread.CurrentThread == thread)
{
action();
return;
}
Interlocked.Increment(ref invokeCount);
ExceptionDispatchInfo? failure = null;
using var done = new ManualResetEventSlim();
queue.Add(() => {
try
{
action();
}
catch (Exception ex)
{
failure = ExceptionDispatchInfo.Capture(ex);
}
finally
{
done.Set();
}
});
done.Wait();
failure?.Throw();
}
public void Dispose()
{
queue.CompleteAdding();
thread.Join();
queue.Dispose();
}
}
bool oldFailFast;
string? oldLogFilePath;
[SetUp]
public void SetUp()
{
oldFailFast = TreeThreadAffinity.FailFast;
oldLogFilePath = TreeThreadAffinity.LogFilePath;
// Tests must not append to whatever log an exploratory session is collecting, and must not
// inherit the mode from the environment: the ones that need a throw opt in individually.
TreeThreadAffinity.LogFilePath = null;
TreeThreadAffinity.FailFast = false;
TreeThreadAffinity.Clear();
}
[TearDown]
public void TearDown()
{
TreeThreadAffinity.FailFast = oldFailFast;
TreeThreadAffinity.LogFilePath = oldLogFilePath;
TreeThreadAffinity.Clear();
}
/// <summary>
/// Runs <paramref name="action"/> to completion on a dedicated thread and returns whatever it
/// threw. Join() makes this deterministic: no sleeps, no polling.
/// </summary>
static Exception? RunOnOtherThread(Action action)
{
Exception? error = null;
var thread = new Thread(() => {
try
{
action();
}
catch (Exception ex)
{
error = ex;
}
}) { Name = "affinity-test-worker", IsBackground = true };
thread.Start();
thread.Join();
return error;
}
[Test]
public void EnsureLazyChildrenFromNonOwningThread_LoadsOnTheOwningThread()
{
using var owner = new OwnerThread();
var root = new LazyTestNode("root");
root.SetOwner(owner.Thread, owner.Invoke);
root.EnsureLazyChildren();
root.LoadChildrenThread.Should().BeSameAs(owner.Thread);
root.LoadChildrenCount.Should().Be(1);
owner.InvokeCount.Should().Be(1);
}
[Test]
public void EnsureLazyChildrenOnUnownedTree_LoadsOnTheCallingThread()
{
var root = new LazyTestNode("root");
var error = RunOnOtherThread(root.EnsureLazyChildren);
error.Should().BeNull();
root.LoadChildrenThread.Should().NotBeNull();
root.LoadChildrenThread!.Name.Should().Be("affinity-test-worker");
root.LoadChildrenCount.Should().Be(1);
}
[Test]
public void EnsureLazyChildrenOnTheOwningThread_DoesNotMarshalAndLoadsOnce()
{
using var owner = new OwnerThread();
var root = new LazyTestNode("root");
root.SetOwner(owner.Thread, owner.Invoke);
owner.Invoke(() => {
root.EnsureLazyChildren();
root.EnsureLazyChildren();
});
// One marshalled call: the test thread getting onto the owner. Nothing inside it needed a
// second hop, which is what keeps a blocking invoke from deadlocking on itself.
owner.InvokeCount.Should().Be(1);
root.LoadChildrenCount.Should().Be(1);
root.LoadChildrenThread.Should().BeSameAs(owner.Thread);
}
[Test]
public void NestedEnsureLazyChildrenFromNonOwningThread_MarshalsOnlyOnce()
{
// The shape AssemblyReferenceReferencedTypesTreeNode has: loading a node's children
// realises a child node's children from inside LoadChildren.
using var owner = new OwnerThread();
var root = new LazyTestNode("root");
var child = new LazyTestNode("child");
root.OnLoadChildren = node => {
node.Children.Add(child);
child.EnsureLazyChildren();
};
root.SetOwner(owner.Thread, owner.Invoke);
root.EnsureLazyChildren();
owner.InvokeCount.Should().Be(1);
root.LoadChildrenThread.Should().BeSameAs(owner.Thread);
child.LoadChildrenThread.Should().BeSameAs(owner.Thread);
child.LoadChildrenCount.Should().Be(1);
}
#if DEBUG
[Test]
public void EnsureLazyChildrenFromNonOwningThread_ProducesNoViolation()
{
TreeThreadAffinity.FailFast = true;
using var owner = new OwnerThread();
var root = new LazyTestNode("root");
root.OnLoadChildren = node => node.Children.Add(new TestNode("child"));
root.SetOwner(owner.Thread, owner.Invoke);
root.EnsureLazyChildren();
TreeThreadAffinity.Violations.Should().BeEmpty();
}
[Test]
public void FailFastViolation_IsRecordedEvenWhenTheThrowIsSwallowed()
{
// The app's background decompile wraps every node in catch (Exception), so a fail-fast
// throw never reaches the test host. The collector has to stay authoritative no matter who
// catches, or a fail-fast suite run comes back green and means nothing.
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
root.SetOwner();
var error = RunOnOtherThread(() => {
try
{
root.Children.Add(new TestNode("child"));
}
catch (Exception)
{
}
});
error.Should().BeNull();
TreeThreadAffinity.Violations.Should().ContainSingle()
.Which.Operation.Should().Contain("Children.Add");
}
[Test]
public void ChildrenAddFromNonOwningThread_IsReported()
{
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
root.SetOwner();
var error = RunOnOtherThread(() => root.Children.Add(new TestNode("child")));
error.Should().BeOfType<InvalidOperationException>();
error!.Message.Should().Contain("Children.Add").And.Contain("\"root\"");
}
[Test]
public void IsExpandedFromNonOwningThread_IsReported()
{
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
root.Children.Add(new TestNode("child"));
root.SetOwner();
var error = RunOnOtherThread(() => root.IsExpanded = true);
error.Should().BeOfType<InvalidOperationException>();
error!.Message.Should().Contain(nameof(SharpTreeNode.IsExpanded));
}
[Test]
public void IsHiddenFromNonOwningThread_IsReported()
{
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
var child = new TestNode("child");
root.Children.Add(child);
root.SetOwner();
var error = RunOnOtherThread(() => child.IsHidden = true);
error.Should().BeOfType<InvalidOperationException>();
error!.Message.Should().Contain(nameof(SharpTreeNode.IsHidden));
}
[Test]
public void UnownedTree_IsNotChecked()
{
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
var error = RunOnOtherThread(() => {
root.Children.Add(new TestNode("child"));
root.IsExpanded = true;
});
error.Should().BeNull();
TreeThreadAffinity.Violations.Should().BeEmpty();
}
[Test]
public void SubtreeBuiltOffThreadThenPublishedByOwner_IsNotReported()
{
// The pattern the analyzers use: assemble a subtree on a worker, hand it to the owning
// thread, attach it there.
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
root.SetOwner();
TestNode? built = null;
var error = RunOnOtherThread(() => {
built = new TestNode("built");
built.Children.Add(new TestNode("grandchild"));
built.IsExpanded = true;
});
error.Should().BeNull();
root.Children.Add(built!);
TreeThreadAffinity.Violations.Should().BeEmpty();
}
[Test]
public void OwnershipIsInheritedByChildrenAttachedLater()
{
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
root.SetOwner();
var child = new TestNode("child");
root.Children.Add(child);
var error = RunOnOtherThread(() => child.Children.Add(new TestNode("grandchild")));
error.Should().BeOfType<InvalidOperationException>();
error!.Message.Should().Contain("\"child\"");
}
[Test]
public void AttachingSubtreeOwnedByAnotherThread_IsReportedOnceThenInherits()
{
var root = new TestNode("root");
root.SetOwner();
var foreign = new TestNode("foreign");
RunOnOtherThread(() => foreign.SetOwner()).Should().BeNull();
root.Children.Add(foreign);
TreeThreadAffinity.Violations.Should().ContainSingle()
.Which.Operation.Should().Contain("owned by another thread");
// The conflicting owner is dropped, so the subtree now inherits the root's owner: further
// mutation from the owning thread is clean, and from any other thread is a violation.
TreeThreadAffinity.Clear();
foreign.Children.Add(new TestNode("grandchild"));
TreeThreadAffinity.Violations.Should().BeEmpty();
TreeThreadAffinity.FailFast = true;
RunOnOtherThread(() => foreign.Children.Add(new TestNode("other")))
.Should().BeOfType<InvalidOperationException>();
}
[Test]
public void RepeatedViolationsFromOneCallSite_AreDeduplicatedAndCounted()
{
var root = new TestNode("root");
root.SetOwner();
var error = RunOnOtherThread(() => {
for (int i = 0; i < 5; i++)
root.Children.Add(new TestNode("child" + i));
});
error.Should().BeNull();
var violation = TreeThreadAffinity.Violations.Should().ContainSingle().Subject;
violation.Count.Should().Be(5);
violation.StackTrace.Should().Contain(nameof(RepeatedViolationsFromOneCallSite_AreDeduplicatedAndCounted));
violation.ActualThread.Should().Contain("affinity-test-worker");
}
[Test]
public void OwnershipHandoffFromNonOwningThread_IsReported()
{
TreeThreadAffinity.FailFast = true;
var root = new TestNode("root");
root.SetOwner();
var error = RunOnOtherThread(() => root.SetOwner());
error.Should().BeOfType<InvalidOperationException>();
error!.Message.Should().Contain(nameof(SharpTreeNode.SetOwner));
}
#else
[Test]
public void ThreadAffinityChecksAreDebugOnly()
{
Assert.Ignore("SharpTreeNode thread-affinity checking is compiled out in release builds.");
}
#endif
}

67
ILSpy.Tests/TreeThreadAffinityGuard.cs

@ -0,0 +1,67 @@ @@ -0,0 +1,67 @@
// Copyright (c) 2026 Siegfried Pammer
//
// Permission is hereby granted, free of charge, to any person obtaining a copy of this
// software and associated documentation files (the "Software"), to deal in the Software
// without restriction, including without limitation the rights to use, copy, modify, merge,
// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
// to whom the Software is furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all copies or
// substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
using System;
using System.Linq;
using ICSharpCode.ILSpyX.TreeView;
using NUnit.Framework;
using NUnit.Framework.Interfaces;
[assembly: TreeThreadAffinityGuard]
// Deliberately outside any namespace: this is applied to the assembly, and the attribute has to be
// nameable from the assembly-level attribute list.
/// <summary>
/// Fails any test that mutated a displayed tree from a thread other than the tree's owner.
/// </summary>
/// <remarks>
/// The affinity check records into <see cref="TreeThreadAffinity.Violations"/> instead of relying
/// on a throw, because tree mutation happens inside callers that catch Exception - the background
/// decompile turns any exception a node raises into text in the output pane. A throw there is
/// swallowed and the run still passes, so something has to read the collector, and it has to be
/// per test: an assembly-level teardown failure is reported but does not fail the run or change
/// the exit code. Debug builds only - the check compiles away in release, where the collector
/// stays empty and this is a no-op.
///
/// A fixture that provokes violations on purpose clears the collector in its own
/// <c>[TearDown]</c>, which runs before this.
/// </remarks>
[AttributeUsage(AttributeTargets.Assembly)]
public sealed class TreeThreadAffinityGuardAttribute : Attribute, ITestAction
{
public ActionTargets Targets => ActionTargets.Test;
public void BeforeTest(ITest test)
{
TreeThreadAffinity.Clear();
}
public void AfterTest(ITest test)
{
var violations = TreeThreadAffinity.Violations;
if (violations.Count == 0)
return;
TreeThreadAffinity.Clear();
Assert.Fail($"{violations.Count} tree thread-affinity violation(s) were recorded while this test ran:"
+ Environment.NewLine
+ string.Join(Environment.NewLine, violations.Select(v => v.ToString())));
}
}

10
ILSpy/Controls/TreeView/SharpTreeView.cs

@ -20,6 +20,7 @@ using System; @@ -20,6 +20,7 @@ using System;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.Linq;
using System.Threading;
using Avalonia;
using Avalonia.Controls;
@ -129,6 +130,15 @@ namespace ICSharpCode.ILSpy.Controls.TreeView @@ -129,6 +130,15 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
}
if (Root != null)
{
// The tree becomes reachable from the UI here, so this is where the UI thread takes
// ownership of it. Avalonia raises property changes on the UI thread, so the calling
// thread is the right owner. Every SharpTreeView in the app routes through Reload,
// so this single call claims every displayed tree.
//
// The dispatcher invoke goes with it: EnsureLazyChildren uses it to get back onto
// this thread when a background decompile realizes a node's children, which the
// tree model cannot do for itself - it must not name a UI framework.
Root.SetOwner(Thread.CurrentThread, action => Dispatcher.UIThread.Invoke(action));
if (!(ShowRoot && ShowRootExpander))
Root.IsExpanded = true;
flattener = new TreeFlattener(Root, ShowRoot);

5
ILSpy/TreeNodes/ReferenceFolderTreeNode.cs

@ -19,8 +19,6 @@ @@ -19,8 +19,6 @@
using System.Linq;
using System.Threading.Tasks;
using Avalonia.Threading;
using ICSharpCode.Decompiler;
using ICSharpCode.Decompiler.Metadata;
using ICSharpCode.ILSpy.Properties;
@ -69,8 +67,7 @@ namespace ICSharpCode.ILSpy.TreeNodes @@ -69,8 +67,7 @@ namespace ICSharpCode.ILSpy.TreeNodes
output.WriteLine($"Effective TargetFramework-Id: {effectiveTargetFramework}");
output.WriteLine($"Detected RuntimePack: {runtimePack}");
// Children realise lazily on the UI thread; we may run from a background decompile.
Dispatcher.UIThread.Invoke(EnsureLazyChildren);
EnsureLazyChildren();
output.WriteLine();
output.WriteLine("Referenced assemblies (in metadata order):");
foreach (var node in Children.OfType<ILSpyTreeNode>())

4
ILSpy/TreeNodes/ResourceListTreeNode.cs

@ -18,8 +18,6 @@ @@ -18,8 +18,6 @@
using System.Linq;
using Avalonia.Threading;
using ICSharpCode.Decompiler;
using ICSharpCode.Decompiler.Metadata;
using ICSharpCode.ILSpy.Properties;
@ -80,7 +78,7 @@ namespace ICSharpCode.ILSpy.TreeNodes @@ -80,7 +78,7 @@ namespace ICSharpCode.ILSpy.TreeNodes
public override void Decompile(Language language, ITextOutput output, DecompilationOptions options)
{
Dispatcher.UIThread.Invoke(EnsureLazyChildren);
EnsureLazyChildren();
foreach (var child in Children.OfType<ILSpyTreeNode>())
{
child.Decompile(language, output, options);

Loading…
Cancel
Save