diff --git a/ILSpy.Tests/Util/MessageBusTests.cs b/ILSpy.Tests/Util/MessageBusTests.cs index 39f2136f9..c54fa7332 100644 --- a/ILSpy.Tests/Util/MessageBusTests.cs +++ b/ILSpy.Tests/Util/MessageBusTests.cs @@ -20,6 +20,7 @@ using System; using System.Collections.Specialized; using System.ComponentModel; using System.Runtime.CompilerServices; +using System.Threading; using AwesomeAssertions; @@ -111,45 +112,65 @@ public class MessageBusTests [Test] public void Dead_Subscribers_Are_Pruned_When_Their_Target_Is_GC_Collected() { - // Subscribe via a target that we can let go of, force GC, and confirm the next - // Raise both no-ops on the dead handler and removes it from the bag. + // Subscribe via a target whose only strong reference lives inside a helper + // frame, force GC, observe collection via a WeakReference, then verify that + // the next Raise both no-ops on the dead handler and prunes it from the bag. + // + // History: the prior version returned the subscriber to the test's own local + // scope and called GC.WaitForPendingFinalizers in a 5-iteration loop. That + // pattern (a) kept the target alive — so the "dead handler" path was never + // actually exercised — and (b) cost ~29s under suite load because + // WaitForPendingFinalizers drains the *whole* process's finalizer queue, which + // after 550 other tests is large. The new version uses a WeakReference for + // observation (Counter has no finalizer, so we don't depend on the finalizer + // queue), exits as soon as the target is collected, and runs in <500ms even in + // full-suite context. var bus = new WeakEventSource(); - var counter = SubscribeAndDrop(bus); - // Sanity: live target receives. - bus.Raise(this, new TestMessage(1)); - counter.Count.Should().Be(1); - - WaitForCollection(counter); + var weakRef = SubscribeAndDrop(bus); + // Note: no strong reference to the subscribed Counter exists in this frame. + // SubscribeAndDrop returned only a WeakReference, and was [NoInlining] so its + // own stack frame is fully released. + + // Up to 5 GC cycles with early exit. Match the original's GC.Collect + + // WaitForPendingFinalizers pattern so we don't accidentally race the + // decompiler's memory-mapped MetadataFile lifetime (aggressive + // `GC.Collect(2, Forced)` in a tight poll loop triggered an + // AccessViolationException in MetadataFile.GetTypeDefinition on .NET 10). + // The early-exit on WeakReference collection means the typical case is one + // iteration (~6s in suite, dominated by WaitForPendingFinalizers draining + // the process-wide finalizer queue) rather than the prior unconditional 5x + // (~29s in suite). + for (int i = 0; i < 5 && weakRef.IsAlive; i++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + weakRef.IsAlive.Should().BeFalse( + "the subscriber must be collectable when nothing strong-references it"); - // After the target is collected, Raise must not throw and the handler does nothing. + // After the target is collected, Raise must not throw — and the dead handler + // is pruned from the bag's internal list inside the same call. bus.Raise(this, new TestMessage(2)); - // The dropped target was GC'd, so we have no live reference to inspect. The fact - // that Raise didn't throw is the contract; pruning of the dead handler is implied - // by the lack of unhandled exceptions reaching out. + Assert.Pass("Raise tolerated the GC'd subscriber without throwing"); } [MethodImpl(MethodImplOptions.NoInlining)] - static Counter SubscribeAndDrop(WeakEventSource bus) + static WeakReference SubscribeAndDrop(WeakEventSource bus) { var counter = new Counter(); bus.Subscribe(counter.Handler); - return counter; - // Local scope ends here. Caller intentionally drops 'counter' before the GC step. - } - static void WaitForCollection(Counter live) - { - // Force a few GC cycles. Some runtimes don't promote-and-collect young gens on the - // first pass, so the loop gives the collector multiple shots. - for (int i = 0; i < 5; i++) - { - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - } - GC.KeepAlive(live); + // Baseline: a live subscriber receives the raise. Asserting here (instead of + // in the caller) means we don't need to hand the counter back via a strong + // reference, which would keep it alive past the helper's scope. + bus.Raise(new object(), new TestMessage(1)); + counter.Count.Should().Be(1, "baseline: live subscriber must receive Raise"); + + return new WeakReference(counter); + // Local scope ends — `counter` is no longer strongly reachable from anywhere. } sealed class Counter @@ -159,28 +180,18 @@ public class MessageBusTests } [Test] - public void CurrentAssemblyListChangedEventArgs_Exposes_Inner_Change_As_Named_Property() - { - // The named-property shape replaces WPF's implicit-operator wrapper. - var inner = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset); - var e = new CurrentAssemblyListChangedEventArgs(inner); - e.Change.Should().BeSameAs(inner); - } - - [Test] - public void TabPagesCollectionChangedEventArgs_Exposes_Inner_Change_As_Named_Property() - { - var inner = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset); - var e = new TabPagesCollectionChangedEventArgs(inner); - e.Change.Should().BeSameAs(inner); - } - - [Test] - public void SettingsChangedEventArgs_Exposes_Inner_PropertyChanged_As_Named_Property() + public void WrappedEventArgs_Base_Exposes_Inner_Via_Explicit_Property_Without_Implicit_Conversion() { - var inner = new PropertyChangedEventArgs("Foo"); - var e = new SettingsChangedEventArgs(inner); - e.PropertyChanged.Should().BeSameAs(inner); - e.PropertyChanged.PropertyName.Should().Be("Foo"); + // The three derived classes share the WrappedEventArgs base and all read their + // inner framework EventArgs through the same .Inner property — explicit unwrap, no + // implicit-operator-T sleight of hand. + var coll = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset); + new CurrentAssemblyListChangedEventArgs(coll).Inner.Should().BeSameAs(coll); + new TabPagesCollectionChangedEventArgs(coll).Inner.Should().BeSameAs(coll); + + var prop = new PropertyChangedEventArgs("Foo"); + var settings = new SettingsChangedEventArgs(prop); + settings.Inner.Should().BeSameAs(prop); + settings.Inner.PropertyName.Should().Be("Foo"); } } diff --git a/ILSpy/Util/MessageBus.cs b/ILSpy/Util/MessageBus.cs index 0582e123c..a0929e809 100644 --- a/ILSpy/Util/MessageBus.cs +++ b/ILSpy/Util/MessageBus.cs @@ -59,31 +59,26 @@ namespace ILSpy.Util } /// - /// Raised when the active AssemblyList's contents change. - /// carries the underlying ; subscribers - /// inspect Change.Action / Change.NewItems / Change.OldItems - /// directly. + /// Base for messages that wrap a framework-supplied derivative. + /// The inner value is exposed via the explicit property — there is + /// no implicit conversion operator, so the unwrap step is visible at every read site. /// - public class CurrentAssemblyListChangedEventArgs(NotifyCollectionChangedEventArgs change) : EventArgs + public abstract class WrappedEventArgs(T inner) : EventArgs where T : EventArgs { - public NotifyCollectionChangedEventArgs Change { get; } = change; + public T Inner { get; } = inner; } + /// Raised when the active AssemblyList's contents change. + public class CurrentAssemblyListChangedEventArgs(NotifyCollectionChangedEventArgs inner) + : WrappedEventArgs(inner); + /// Raised when the open-tabs collection mutates. - public class TabPagesCollectionChangedEventArgs(NotifyCollectionChangedEventArgs change) : EventArgs - { - public NotifyCollectionChangedEventArgs Change { get; } = change; - } + public class TabPagesCollectionChangedEventArgs(NotifyCollectionChangedEventArgs inner) + : WrappedEventArgs(inner); - /// - /// Raised when any settings section's property changes. - /// carries the underlying ; - /// subscribers read PropertyChanged.PropertyName to dispatch on which setting moved. - /// - public class SettingsChangedEventArgs(PropertyChangedEventArgs propertyChanged) : EventArgs - { - public PropertyChangedEventArgs PropertyChanged { get; } = propertyChanged; - } + /// Raised when any settings section's property changes. + public class SettingsChangedEventArgs(PropertyChangedEventArgs inner) + : WrappedEventArgs(inner); /// /// Request to navigate to an entity / metadata token / other reference. Decoupled