[GH-325] Add WeakEventSource.SuppressNotifications() opt-in suppression scope

[amaggiulli]

This pull request introduces a thread-local notification suppression mechanism to the WeakEventSource class, allowing callers to temporarily disable all event notifications and related overhead on a per-thread basis. This is especially useful for scenarios involving large numbers of short-lived observer graphs, improving performance by eliminating unnecessary notification logic when it's not needed.

[Copilot]

Pull request overview

Adds an opt-in, per-thread suppression scope to WeakEventSource so callers can bypass the subscribe/unsubscribe/raise machinery (and its locking/reflection overhead) during performance-critical operations where notifications are unnecessary.

Changes:

• Introduces a [ThreadStatic] suppression depth counter and public NotificationsSuppressed flag.
• Adds WeakEventSource.SuppressNotifications() returning an IDisposable scope for temporary suppression.
• Short-circuits Raise, Subscribe, and Unsubscribe when suppression is active.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/QLNet/Patterns/WeakEventSource.cs:77

• Dispose() decrements _suppressionDepth without guarding against underflow. If the counter is ever corrupted (e.g., due to a misbalanced scope), it can go negative and silently disable suppression checks. Add an explicit underflow guard to fail fast.

            _disposed = true;
            _suppressionDepth--;

src/QLNet/Patterns/WeakEventSource.cs:101

• SuppressNotifications() currently makes Subscribe/Unsubscribe no-ops as well as Raise(). That means subscriptions made inside a suppression scope will be silently dropped, and unsubscriptions will be skipped, which can leave an object graph in an unexpected state once suppression ends. If the intent is specifically to suppress notifications (Raise) and its overhead, consider letting Subscribe/Unsubscribe continue to mutate the handler list normally.

      public void Subscribe(Callback handler)
      {
         if (NotificationsSuppressed)
            return;

src/QLNet/Patterns/WeakEventSource.cs:60

• SuppressNotifications() increments the [ThreadStatic] counter before allocating/constructing the scope object. If scope allocation/construction throws (e.g., OOM), the suppression depth leaks and can permanently suppress notifications for the current thread. Move the increment into the scope constructor so the counter is only changed after the scope is successfully created.

This issue also appears in the following locations of the same file:

• line 76
• line 97

      public static IDisposable SuppressNotifications()
      {
         _suppressionDepth++;
         return new SuppressionScope(Environment.CurrentManagedThreadId);
      }

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/QLNet/Patterns/WeakEventSource.cs:40

• The doc comment says the suppression scope makes every WeakEventSource instance a "complete no-op" for the thread, but Clear() remains active and still mutates the handler list (affecting all threads). This makes the comment misleading and could confuse callers about what is actually suppressed.

      // Per-thread opt-out for the whole Subscribe/Unsubscribe/Raise pipeline. Some callers construct large
      // numbers of short-lived, single-use IObservable/IObserver graphs (e.g. one-off bond pricing) where the
      // observer-notification machinery (reflection-based weak-delegate wrapping, list mutation under lock) is
      // pure overhead: nothing outlives the call, and nothing needs to be notified of anything. Wrapping such a
      // call in a using(WeakEventSource.SuppressNotifications()) scope makes every WeakEventSource instance a

src/QLNet/Patterns/WeakEventSource.cs:49

• SuppressNotifications() returns IDisposable and always allocates a heap object (new SuppressionScope(...)). Since the stated goal is reducing overhead in hot paths, consider exposing a non-allocating scope type (e.g., a public readonly struct scope returned by value) so using var _ = WeakEventSource.SuppressNotifications(); can avoid allocations.

      public static IDisposable SuppressNotifications()
      {
         return new SuppressionScope(Environment.CurrentManagedThreadId);
      }

src/QLNet/Patterns/WeakEventSource.cs:78

• Dispose() throws if the scope is disposed on a different thread. Throwing from Dispose (especially inside a using/finally) can mask an exception thrown inside the suppressed scope and make failures harder to diagnose in production. Consider switching this to a debug-only assertion/logging, or otherwise ensuring Dispose is non-throwing (while still preventing state corruption).

            if (Environment.CurrentManagedThreadId != _creatingThreadId)
               throw new InvalidOperationException(
                  "WeakEventSource.SuppressNotifications() scope must be disposed on the same thread that created it.");

src/QLNet/Patterns/WeakEventSource.cs:49

• New suppression behavior is public API and affects core observer mechanics, but there are no tests covering it. Adding unit tests would help lock down: (1) Subscribe/Raise/Unsubscribe are suppressed within the scope, (2) nesting increments/decrements correctly, (3) suppression is thread-local (other threads still notify), and (4) the chosen cross-thread-dispose behavior (throw or not).

      public static bool NotificationsSuppressed => _suppressionDepth > 0;

      public static IDisposable SuppressNotifications()
      {
         return new SuppressionScope(Environment.CurrentManagedThreadId);
      }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (6)

tests/QLNet.Tests/T_WeakEventSourceSuppression.cs:156

• Like the other waits/joins in this test, these calls have no timeout and can hang the suite on failure. Prefer bounded waits/joins with assertions so CI fails fast.

            Assert.True(WeakEventSource.NotificationsSuppressed);
            suppressionEntered.Set();
            releaseOtherThread.Wait();
         }

         otherThread.Join();

tests/QLNet.Tests/T_WeakEventSourceSuppression.cs:183

• Thread.Join() without a timeout can hang the entire test run if the worker thread blocks unexpectedly. Using a bounded join makes failures deterministic.

         otherThread.IsBackground = true;
         otherThread.Start();
         otherThread.Join();

src/QLNet/Patterns/WeakEventSource.cs:61

• SuppressionScope is a mutable struct used as a scope/handle. Because value types are copyable (assignment, passing as parameters, capturing in lambdas, etc.), the _disposed flag only protects a single instance; disposing a copy can either (a) throw the underflow guard or (b) leave the original scope undisposed. This makes the “idempotent Dispose” guarantee unreliable and can cause surprising runtime failures for callers.

Consider switching the scope to a reference type (to make idempotency real), or to a stack-only pattern (ref struct with pattern-based Dispose and no IDisposable) to prevent capturing/escaping. Either way, the current API surface is easy to misuse unintentionally.

      // Public, non-allocating value-type scope: SuppressNotifications() is meant to be used in hot paths, so
      // it returns this concrete struct (rather than IDisposable) to let `using (WeakEventSource.
      // SuppressNotifications())` dispose it via the C# pattern-based using support with no heap allocation and
      // no boxing. It cannot be a `readonly struct` because Dispose() needs to mutate the _disposed field to
      // stay idempotent (safe to call more than once), per the usual IDisposable contract.
      public struct SuppressionScope : IDisposable
      {
         private readonly int _creatingThreadId;
         private bool _disposed;

src/QLNet/Patterns/WeakEventSource.cs:71

• This comment is misleading: SuppressionScope is a value type, so there is no heap “allocation of this object” here, and the OOM scenario described doesn’t really apply. The intent (incrementing inside the constructor so it pairs with scope creation) is good, but it should be described accurately.

            // Increment only once construction is actually underway, not before allocating this object: if
            // allocation/construction itself were to throw (e.g. OutOfMemoryException), the counter would
            // otherwise have already been bumped with no scope ever handed back to the caller to dispose it,
            // permanently (for this thread) suppressing notifications.
            _suppressionDepth++;

tests/QLNet.Tests/T_WeakEventSourceSuppression.cs:104

• The comment claims idempotent Dispose() is part of the “standard IDisposable contract”, but IDisposable does not require idempotency (it’s just a common pattern). Since this test is defining behavior, it would be clearer to describe it as an explicit design choice for this API.

         // A second Dispose() call must be a safe no-op (standard IDisposable contract), and must
         // not decrement _suppressionDepth a second time (which would trigger the underflow guard
         // on a subsequent, unrelated SuppressNotifications()/Dispose() pair on this thread).

tests/QLNet.Tests/T_WeakEventSourceSuppression.cs:135

• These synchronization waits have no timeout, so a regression can hang the test run indefinitely (CI stall) instead of failing fast. Add a reasonable timeout and assert success.

This issue also appears in the following locations of the same file:

• line 151
• line 180

               // Wait until the main thread has entered its suppression scope before raising, so
               // this genuinely exercises "another thread while suppression is active elsewhere".
               suppressionEntered.Wait();
               Assert.False(WeakEventSource.NotificationsSuppressed);
               source.Raise();

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/QLNet/Patterns/WeakEventSource.cs:81

• The constructor currently sets _disposed = false, but for a value type this makes the default value indistinguishable from an entered scope. If you switch to an _active flag, set it to true here so default(SuppressionScope) can remain inactive and disposing it can be a no-op.

         internal SuppressionScope(bool _)
         {
            _disposed = false;
            _suppressionDepth++;
         }

src/QLNet/Patterns/WeakEventSource.cs:87

• Dispose() currently throws when _suppressionDepth is 0, which includes the default(SuppressionScope) case. If the scope tracks whether it is active (entered), Dispose() can become a no-op for inactive/default scopes while still throwing on genuine underflow for active scopes.

         public void Dispose()
         {
            if (_disposed)
               return;

tests/QLNet.Tests/T_WeakEventSourceSuppression.cs:96

• Similar to the nested-scope test, scope is created without using/try/finally. If the test throws between creation and the first Dispose(), suppression can leak to subsequent tests on the same thread. Use using var scope = ... to guarantee cleanup even on failure.

         var scope = WeakEventSource.SuppressNotifications();

src/QLNet/Patterns/WeakEventSource.cs:69

• SuppressionScope is a public ref struct, so callers can legally end up with default(SuppressionScope) (e.g. via a conditional scope). With the current field naming/semantics (_disposed defaulting to false), calling Dispose() on a default value will throw due to _suppressionDepth == 0. Consider tracking whether the scope is actually active (entered) so default can behave as a safe no-op scope while preserving the underflow guard for real scopes.

This issue also appears in the following locations of the same file:

• line 77
• line 83

      public ref struct SuppressionScope
      {
         private bool _disposed;

tests/QLNet.Tests/T_WeakEventSourceSuppression.cs:82

• This test manually manages suppression scopes without a using/try/finally. If an assertion throws before Dispose() is reached, _suppressionDepth can leak on the current thread and cascade failures into later tests. Prefer using var for the scopes to guarantee cleanup even on test failure (extra Dispose calls are fine since the scope is designed to be idempotent).

This issue also appears on line 96 of the same file.

         var outer = WeakEventSource.SuppressNotifications();
         Assert.True(WeakEventSource.NotificationsSuppressed);

         var inner = WeakEventSource.SuppressNotifications();
         Assert.True(WeakEventSource.NotificationsSuppressed);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/QLNet.Tests/T_WeakEventSourceSuppression.cs:71

• In this test, the suppression behavior of Raise() isn’t actually asserted while a real handler is registered. As written, the only Raise() inside a suppression scope happens after a suppressed Subscribe(), so the test would still pass even if Raise() were not suppressed (because there are no handlers). Add an in-scope Raise() after a real Subscribe(), and assert the count doesn’t change, to validate the intended behavior.

         using (WeakEventSource.SuppressNotifications())
         {
            // Unsubscribe should be a no-op inside the scope: the handler must remain registered.
            source.Unsubscribe(counter.Increment);
         }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.