Potential Concurrency Issue in Stylet Framework

[TreatTrick]

Hello Stylet team,

I'm writing to bring to your attention a potential issue with the Stylet Framework that I've encountered while developing an application using your library.

Here is the sample code to reproduce the issue:

   public class TestModel : PropertyChangedBase
    {
        public string Name { get; set; }
    }

    public class MainWindowViewModel: Screen,IHandle<TestEventcs>
    {
        [Inject]
        public IEventAggregator _eventAggregator;

        protected override void OnViewLoaded()
        {
            base.OnViewLoaded();
            _eventAggregator.Subscribe(this);
        }
        public MainWindowViewModel()
        {
            for (int i = 0; i < 10; i++)
            {
                TestModels.Add(new TestModel { Name = i.ToString() });
            }
        }

        public BindableCollection<TestModel> TestModels { get; set; } = new BindableCollection<TestModel>();

        public void Test()
        {
            Task.Run(() =>
            {
                _eventAggregator.Publish(new TestEventcs());
            });
            Thread.Sleep(100);//to make sure te UI thread and above thread access te TestModels at the same time.
            _eventAggregator.Publish(new TestEventcs());
        }

        public void Handle(TestEventcs message)
        {
            Console.WriteLine("Before Clear, thread: " + Thread.CurrentThread.ManagedThreadId);
            TestModels.Clear();
            for (int j = 0; j < 5; j++)
            {
                Console.WriteLine(j.ToString() + ", thread: " + Thread.CurrentThread.ManagedThreadId);
                TestModels.Add(new TestModel { Name = j.ToString() });
            }     
        }
    }

The issue arises when the Test() method is called. the UI hangs after printing "Before Clear, thread: 3" in the console. This seems to suggest a concurrency problem, as the UI thread appears to be waiting indefinitely for something to complete.

Interestingly, if the Test() function is replaced with code below, which uses another "Task.Run" to replace the direct _eventAggregato call, the UI doesn't hang. This seems to suggest that the issue is specifically related to the two thread, one UI tread and another one background thread, handling the property "TestModels" at the same time.

        public void Test()
        {
            Task.Run(() =>
            {
                _eventAggregator.Publish(new TestEventcs());
            });
            Task.Run(() =>
            {
                _eventAggregator.Publish(new TestEventcs());
            });
        }

This issue is critical for my application, and I'd appreciate any insights you might have into why it's occurring and how it might be resolved. I also attached the project, so any one who wants to test the code could download and run it.TestCode.zip

Thank you for your time and for developing such a useful library!

Best regards,
Xq Sora

[canton7]

The deadlock is:

1. _eventAggregator.Publish(new TestEventcs()); called on background thread
2. Handle(TestEventcs message) called synchronously on that background thread
3. That background thread calls TestModels.Clear();. Updating an ObservableCollection from a background thread is not safe, so this needs to do a synchsonous dispatch to the UI thread
4. However, only one event can be raised at a time. The background thread which is calling TestModels.Clear(); is holding that lock, which means the UI thread can't acquire it. However the lock can't be released until the UI thread processes the TestModels.Clear()
5. Deadlock

There are a number of things which came together here:

1. You tried to update a BindableCollection from a background thread. Although this is supported, it's best avoided: it requires an expensive synchronous call to the UI thread, and you can end up with threading issues.
2. The event aggregator doesn't need to hold the lock when invoking events, which I can fix.

The correct approach is to call _eventAggregator.PublishOnUIThread, if you're going to be calling it from a background thread. This means that Handle(TestEventcs message) is called on the UI thread, which solves the issue with updating a BindableCollection.

I will also fix the EventAggregator to not call handlers while holding a lock.

[TreatTrick]

Thanks very much for your reply! I have understand the reason causing the problem and solve it.

[Yoooi0]

Fixed in #390