Multiple Publish Deadlock

[bradencross]

I am running into a situation where I think the EventAggregator is locking up. The gist of my code is:

A service that creates a thread that reads from a serial port 1/s and publishes an event containing the string from the SerialPort read. This event is subscribed to in a viewmodel so that it can update the UI with data coming from the serial port.

Another event is published whenever a button is pressed to let the ShellViewModel know that some progress has been made and to update a progress tracker UI element.

It appears that whenever that button is pressed at the same time the serial port event is being published that the application comes to a deadlock. It looks like the handlersLock is never being released within the PublishWithDispatcher method.

Sample Code:

Serial Reader Thread

private async Task SerialThread()
{
    await Task.Run(() => 
   {
      // This is not actually in an never ending while loop
       while(true)
       {
           // A response is received 1/s
            string output = _serialPort.ReadLine();
             _eventAggregator.Publish(new SerialEvent() { Data = output });
       }
}

Button Click in MyViewModel

public void ButtonClicked()
{
    _eventAggregator.Publish(new ProgressEvent() { StepComplete = true });
}

MyViewModel subscribes to the SerialEvent and ShellViewModel subscribes to the ProgressEvent. Both these events cause the UI to be updated in some way. Whenever the ProgressEvent and SerialEvent were published at the same time it caused the application to come to a deadlock. I made the change the follwing change _eventAggregator.PublishOnUIThread(new SerialEvent() { Data = output }); and that seemed to fix the issue.

I am wondering if this is the expected behaviour? I think it should also be noted that even if I removed all the subscribers from the events it still deadlocked.

Call Stack:

[Waiting on lock owned by Thread 5028, double-click or press enter to switch to thread]	
>	System.Private.CoreLib.dll!System.Threading.SynchronizationContext.WaitHelper(System.IntPtr[] waitHandles, bool waitAll, int millisecondsTimeout) Line 55	C#
 	WindowsBase.dll!System.Windows.Threading.DispatcherSynchronizationContext.Wait(System.IntPtr[] waitHandles, bool waitAll, int millisecondsTimeout) Line 106	C#
 	System.Private.CoreLib.dll!System.Threading.SynchronizationContext.InvokeWaitMethodHelper(System.Threading.SynchronizationContext syncContext, System.IntPtr[] waitHandles, bool waitAll, int millisecondsTimeout) Line 11	C#
 	[Native to Managed Transition]	
 	[Managed to Native Transition]	
 	Stylet.dll!Stylet.EventAggregator.PublishWithDispatcher(object message, System.Action<System.Action> dispatcher, string[] channels) Line 117	C#
 	Stylet.dll!Stylet.EventAggregatorExtensions.Publish(Stylet.IEventAggregator eventAggregator, object message, string[] channels) Line 251	C#
 	Mako.Core.dll!Mako.Core.ViewModels.ConnectViewModel.OpenAddSensorWindow() Line 162	C#
 	[Native to Managed Transition]	
 	[Managed to Native Transition]	
 	Stylet.dll!Stylet.Xaml.ActionBase.InvokeTargetMethod(object[] parameters) Line 237	C#
 	Stylet.dll!Stylet.Xaml.CommandAction.Execute(object parameter) Line 174	C#
 	PresentationFramework.dll!MS.Internal.Commands.CommandHelpers.CriticalExecuteCommandSource(System.Windows.Input.ICommandSource commandSource, bool userInitiated) Line 181	C#
 	PresentationFramework.dll!System.Windows.Controls.Primitives.ButtonBase.OnClick() Line 75	C#
 	PresentationFramework.dll!System.Windows.Controls.Button.OnClick() Line 273	C#
 	Telerik.Windows.Controls.dll!Telerik.Windows.Controls.RadButton.OnClick()

Thread 5028:

>	System.Private.CoreLib.dll!System.Threading.WaitHandle.WaitOneNoCheck(int millisecondsTimeout) Line 161	C#
 	System.Private.CoreLib.dll!System.Threading.WaitHandle.WaitOne(System.TimeSpan timeout, bool exitContext) Line 408	C#
 	WindowsBase.dll!System.Windows.Threading.DispatcherOperation.DispatcherOperationEvent.WaitOne() Line 659	C#
 	WindowsBase.dll!System.Windows.Threading.DispatcherOperation.Wait(System.TimeSpan timeout) Line 224	C#
 	WindowsBase.dll!System.Windows.Threading.Dispatcher.InvokeImpl(System.Windows.Threading.DispatcherOperation operation, System.Threading.CancellationToken cancellationToken, System.TimeSpan timeout) Line 1386	C#
 	WindowsBase.dll!System.Windows.Threading.Dispatcher.Invoke(System.Action callback, System.Windows.Threading.DispatcherPriority priority, System.Threading.CancellationToken cancellationToken, System.TimeSpan timeout) Line 629	C#
 	WindowsBase.dll!System.Windows.Threading.Dispatcher.Invoke(System.Action callback) Line 511	C#
 	Stylet.dll!Stylet.ApplicationDispatcher.Send(System.Action action) Line 66	C#
 	Stylet.dll!Stylet.Execute.OnUIThreadSync(System.Action action) Line 105	C#
 	Stylet.dll!Stylet.BindableCollection<double>.InsertItem(int index, double item) Line 181	C#
 	System.Private.CoreLib.dll!System.Collections.ObjectModel.Collection<double>.Add(double item) Line 70	C#
 	Mako.Core.dll!Mako.Core.ViewModels.CollectViewModel.Handle(Mako.Core.Events.ReferenceDataEvent message) Line 244	C#
 	Stylet.dll!Stylet.EventAggregator.HandlerInvoker.Invoke(object message) Line 222	C#
 	Stylet.dll!Stylet.EventAggregator.PublishWithDispatcher.AnonymousMethod__2() Line 124	C#
 	Stylet.dll!Stylet.EventAggregatorExtensions.Publish.AnonymousMethod__1_0(System.Action a) Line 250	C#
 	Stylet.dll!Stylet.EventAggregator.PublishWithDispatcher(object message, System.Action<System.Action> dispatcher, string[] channels) Line 124	C#
 	Stylet.dll!Stylet.EventAggregatorExtensions.Publish(Stylet.IEventAggregator eventAggregator, object message, string[] channels) Line 251	C#
 	Mako.Core.dll!Mako.Core.Services.SerialService.StartParoStreamThreadAsync.AnonymousMethod__28_0() Line 292	C#
 	System.Private.CoreLib.dll!System.Threading.Tasks.Task.InnerInvoke() Line 2449	C#
 	System.Private.CoreLib.dll!System.Threading.Tasks.Task..cctor.AnonymousMethod__274_0(object obj) Line 2428	C#
 	System.Private.CoreLib.dll!System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(System.Threading.Thread threadPoolThread, System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state) Line 289	C#
 	System.Private.CoreLib.dll!System.Threading.Tasks.Task.ExecuteWithThreadLocal(ref System.Threading.Tasks.Task currentTaskSlot, System.Threading.Thread threadPoolThread) Line 2389	C#
 	System.Private.CoreLib.dll!System.Threading.Tasks.Task.ExecuteEntryUnsafe(System.Threading.Thread threadPoolThread) Line 2327	C#
 	System.Private.CoreLib.dll!System.Threading.Tasks.Task.ExecuteFromThreadPool(System.Threading.Thread threadPoolThread) Line 2312	C#
 	System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch() Line 663	C#
 	System.Private.CoreLib.dll!System.Threading._ThreadPoolWaitCallback.PerformWaitCallback() Line 29	C#

[canton7]

Ouch, that's a nice one.

Your second thread called into the event aggregator from a non UI thread. That's fine, but it means the handlers are invoked on that same background thread. The handler is invoked while handlersLock is held, and updates a BindableCollection. When a BindableCollection is updated from a non UI thread, it synchronously sends its CollectionChanged event invocation to the UI thread, and waits for it to complete (WPF crashes out if CollectionChanged is invoked on a non UI thread).

While that was happening, your first thread called into the event aggregator from the UI thread, and tried to acquire handlersLock. However, that lock is held by the second thread, which is blocked waiting for the UI to fire the CollectionChanged event.

So the UI thread is waiting for a lock, and the thing holding for the lock is waiting for the UI thread to fire the CollectionChanged event. And that's your deadlock.

Calling PublishOnUIThread is the right approach: you shouldn't be running ViewModel code on background threads, and that means that BindableCollection doesn't have to do expensive synchronous dispatches to the UI thread to fire its CollectionChanged event.

That said, the event aggregator shouldn't be calling upwards from within a lock: that's basic best practice. I'll fix that.

Nice report, thanks!

[bradencross]

Happy to help and thanks for the quick response!

[Yoooi0]

Fixed in #390