ILSpy

9664 Commits

54 Branches

94 Tags

91 MiB

Tree: ca3e528ce2

Commit Graph

Author	SHA1	Message	Date
Siegfried Pammer	ca3e528ce2	Add a Debug-only thread-affinity check to the shared tree model Issue #3290 is a NullReferenceException in GetNodeByVisibleIndex that is provably unreachable single-threaded: TreeFlattener.Count and GetNodeByVisibleIndex read the same totalListLength fields back to back, so a stale index yields ArgumentOutOfRangeException, never an NRE. A stress harness with reader threads racing an IsExpanded/Children mutator reproduces exactly that NRE, so the crash requires a mutation from a foreign thread. The rule that a displayed tree is only mutated from the UI thread was pure convention: ICSharpCode.ILSpyX/TreeView contained no VerifyAccess, lock or dispatcher of any kind, and two tree nodes already carry a Dispatcher.UIThread.Invoke workaround for the same hazard, which means it has been hit before and fixed one site at a time. ICSharpCode.ILSpyX is host-agnostic and must not name a dispatcher, so ownership is stated by the host instead of inferred: SetOwner(Thread) marks the thread allowed to mutate a node and its subtree. Unowned means unchecked, which is what makes the analyzer pattern legal - build a subtree on a worker, publish it on the UI thread - without an exception carved into the rule. The owner is resolved by walking up the model-parent chain to the nearest explicit owner rather than stamped onto every node. That buys the propagation rules for free: one call on the root covers the whole displayed tree, children attached later inherit it with no bookkeeping, and a subtree built off-thread is unchecked while it is being built yet inherits the owner the moment it is attached - an attachment which is itself a checked mutation of the owned tree. A subtree that already carries a different owner would otherwise leave one displayed tree demanding two threads, so that case is reported once and the incoming owner dropped, rather than reported on every later mutation. Re-owning is allowed because handing a tree over is the point, but the handoff must come from the current owner: a background thread taking a live tree away from the UI is the race being hunted. The check sits in SharpTreeNodeCollection.OnCollectionChanged, which every Children mutator funnels through, and in the IsExpanded and IsHidden setters - the three entry points that invalidate totalListLength. Checking in OnCollectionChanged also means a violation is reported before OnChildrenChanged rewrites the flat-list tree, so the AVL structure is left intact. Violations are collected rather than fatal by default, with per-call-site deduplication and a count, and the first hit of each site written straight through to a log file so a long exploratory session can be read while it runs. FailFast makes them throw so tests can observe one deterministically. Everything is behind #if DEBUG plus [Conditional("DEBUG")], so the release build has no field on SharpTreeNode and no call at any site; verified by decompiling the release assembly. Assisted-by: Claude:claude-opus-5:Claude Code	2 weeks ago

Author

SHA1

Message

Date

Siegfried Pammer

ca3e528ce2

Add a Debug-only thread-affinity check to the shared tree model

Issue #3290 is a NullReferenceException in GetNodeByVisibleIndex that is
provably unreachable single-threaded: TreeFlattener.Count and
GetNodeByVisibleIndex read the same totalListLength fields back to back, so a
stale index yields ArgumentOutOfRangeException, never an NRE. A stress harness
with reader threads racing an IsExpanded/Children mutator reproduces exactly
that NRE, so the crash requires a mutation from a foreign thread. The rule that
a displayed tree is only mutated from the UI thread was pure convention:
ICSharpCode.ILSpyX/TreeView contained no VerifyAccess, lock or dispatcher of any
kind, and two tree nodes already carry a Dispatcher.UIThread.Invoke workaround
for the same hazard, which means it has been hit before and fixed one site at a
time.

ICSharpCode.ILSpyX is host-agnostic and must not name a dispatcher, so ownership
is stated by the host instead of inferred: SetOwner(Thread) marks the thread
allowed to mutate a node and its subtree. Unowned means unchecked, which is what
makes the analyzer pattern legal - build a subtree on a worker, publish it on
the UI thread - without an exception carved into the rule.

The owner is resolved by walking up the model-parent chain to the nearest
explicit owner rather than stamped onto every node. That buys the propagation
rules for free: one call on the root covers the whole displayed tree, children
attached later inherit it with no bookkeeping, and a subtree built off-thread is
unchecked while it is being built yet inherits the owner the moment it is
attached - an attachment which is itself a checked mutation of the owned tree.
A subtree that already carries a different owner would otherwise leave one
displayed tree demanding two threads, so that case is reported once and the
incoming owner dropped, rather than reported on every later mutation. Re-owning
is allowed because handing a tree over is the point, but the handoff must come
from the current owner: a background thread taking a live tree away from the UI
is the race being hunted.

The check sits in SharpTreeNodeCollection.OnCollectionChanged, which every
Children mutator funnels through, and in the IsExpanded and IsHidden setters -
the three entry points that invalidate totalListLength. Checking in
OnCollectionChanged also means a violation is reported before OnChildrenChanged
rewrites the flat-list tree, so the AVL structure is left intact.

Violations are collected rather than fatal by default, with per-call-site
deduplication and a count, and the first hit of each site written straight
through to a log file so a long exploratory session can be read while it runs.
FailFast makes them throw so tests can observe one deterministically.

Everything is behind #if DEBUG plus [Conditional("DEBUG")], so the release build
has no field on SharpTreeNode and no call at any site; verified by decompiling
the release assembly.

Assisted-by: Claude:claude-opus-5:Claude Code

2 weeks ago

1 Commits (ca3e528ce24bf62df6d6bbbb3b34846633ead6a4)