From 5968918bed7649987095f0f8f6fc192d0408d97f Mon Sep 17 00:00:00 2001 From: Grigorii Vasilchenko <36913490+11v1@users.noreply.github.com> Date: Fri, 8 Aug 2025 14:44:25 +0700 Subject: [PATCH] WeakEvent subscription management thread race condition fix (#19383) * Fixed thread-race condition when INotifyCollectionChanged event fired on non-ui thread * Fixed WeakEvent thread-race condition (NullReferenceException) when event fired on non-ui thread * Limited delegates creation in WeakEvent * Avoid casting in WeakEvent --- src/Avalonia.Base/Utilities/WeakEvent.cs | 111 +++++++++++++----- .../Utils/CollectionChangedEventManager.cs | 12 +- 2 files changed, 89 insertions(+), 34 deletions(-) diff --git a/src/Avalonia.Base/Utilities/WeakEvent.cs b/src/Avalonia.Base/Utilities/WeakEvent.cs index bc7cd04568..a237143c6c 100644 --- a/src/Avalonia.Base/Utilities/WeakEvent.cs +++ b/src/Avalonia.Base/Utilities/WeakEvent.cs @@ -1,5 +1,7 @@ using System; using System.Runtime.CompilerServices; +using System.Threading; +using Avalonia.Collections.Pooled; using Avalonia.Threading; namespace Avalonia.Utilities; @@ -10,8 +12,8 @@ namespace Avalonia.Utilities; public sealed class WeakEvent : WeakEvent where TSender : class { private readonly Func, Action> _subscribe; - - private readonly ConditionalWeakTable _subscriptions = new(); + private readonly ConditionalWeakTable _subscriptions = new(); + private readonly ConditionalWeakTable.CreateValueCallback _createSubscription; internal WeakEvent( Action> subscribe, @@ -22,33 +24,43 @@ public sealed class WeakEvent : WeakEvent where TSender : c subscribe(t, s); return () => unsubscribe(t, s); }; + _createSubscription = CreateSubscription; } - + internal WeakEvent(Func, Action> subscribe) { _subscribe = subscribe; + _createSubscription = CreateSubscription; } - + public void Subscribe(TSender target, IWeakEventSubscriber subscriber) { - if (!_subscriptions.TryGetValue(target, out var subscription)) - _subscriptions.Add(target, subscription = new Subscription(this, target)); - subscription.Add(subscriber); + var spinWait = default(SpinWait); + while (true) + { + var subscription = _subscriptions.GetValue(target, _createSubscription); + if (subscription.Add(subscriber)) + break; + spinWait.SpinOnce(); + } } public void Unsubscribe(TSender target, IWeakEventSubscriber subscriber) { - if (_subscriptions.TryGetValue(target, out var subscription)) + if (_subscriptions.TryGetValue(target, out var subscription)) subscription.Remove(subscriber); } + private Subscription CreateSubscription(TSender key) => new(this, key); + private sealed class Subscription { private readonly WeakEvent _ev; private readonly TSender _target; private readonly Action _compact; - private readonly Action _unsubscribe; private readonly WeakHashList> _list = new(); + private readonly object _lock = new(); + private Action? _unsubscribe; private bool _compactScheduled; private bool _destroyed; @@ -57,7 +69,6 @@ public sealed class WeakEvent : WeakEvent where TSender : c _ev = ev; _target = target; _compact = Compact; - _unsubscribe = ev._subscribe(target, OnEvent); } private void Destroy() @@ -65,19 +76,42 @@ public sealed class WeakEvent : WeakEvent where TSender : c if(_destroyed) return; _destroyed = true; - _unsubscribe(); + _unsubscribe?.Invoke(); _ev._subscriptions.Remove(_target); } - public void Add(IWeakEventSubscriber s) => _list.Add(s); + public bool Add(IWeakEventSubscriber s) + { + if (_destroyed) + return false; + + lock (_lock) + { + if (_destroyed) + return false; + + _unsubscribe ??= _ev._subscribe(_target, OnEvent); + _list.Add(s); + return true; + } + } public void Remove(IWeakEventSubscriber s) { - _list.Remove(s); - if(_list.IsEmpty) - Destroy(); - else if(_list.NeedCompact && _compactScheduled) - ScheduleCompact(); + if (_destroyed) + return; + + lock (_lock) + { + if (_destroyed) + return; + + _list.Remove(s); + if(_list.IsEmpty) + Destroy(); + else if(_list.NeedCompact && _compactScheduled) + ScheduleCompact(); + } } private void ScheduleCompact() @@ -90,23 +124,40 @@ public sealed class WeakEvent : WeakEvent where TSender : c private void Compact() { - if(!_compactScheduled) + if (_destroyed) return; - _compactScheduled = false; - _list.Compact(); - if (_list.IsEmpty) - Destroy(); + + lock (_lock) + { + if (_destroyed) + return; + if(!_compactScheduled) + return; + _compactScheduled = false; + _list.Compact(); + if (_list.IsEmpty) + Destroy(); + } } private void OnEvent(object? sender, TEventArgs eventArgs) { - var alive = _list.GetAlive(); - if(alive == null) - Destroy(); - else + PooledList>? alive; + lock (_lock) + { + alive = _list.GetAlive(); + if (alive == null) + { + Destroy(); + return; + } + } + + foreach(var item in alive.Span) + item.OnEvent(_target, _ev, eventArgs); + + lock (_lock) { - foreach(var item in alive.Span) - item.OnEvent(_target, _ev, eventArgs); WeakHashList>.ReturnToSharedPool(alive); if(_list.NeedCompact && !_compactScheduled) ScheduleCompact(); @@ -124,13 +175,13 @@ public class WeakEvent { return new WeakEvent(subscribe, unsubscribe); } - + public static WeakEvent Register( Func, Action> subscribe) where TSender : class where TEventArgs : EventArgs { return new WeakEvent(subscribe); } - + public static WeakEvent Register( Action subscribe, Action unsubscribe) where TSender : class diff --git a/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs b/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs index 675e9e13e5..d9fe30b72b 100644 --- a/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs +++ b/src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs @@ -132,18 +132,22 @@ namespace Avalonia.Controls.Utils } } - var l = Listeners.ToArray(); - if (Dispatcher.UIThread.CheckAccess()) { + var l = Listeners.ToArray(); Notify(_collection, e, l); } else { var eCapture = e; - Dispatcher.UIThread.Post(() => Notify(_collection, eCapture, l), DispatcherPriority.Send); + Dispatcher.UIThread.Post(() => + { + var l = Listeners.ToArray(); + Notify(_collection, eCapture, l); + }, + DispatcherPriority.Send); } } } } -} +} \ No newline at end of file