Reactive: Deadlock on OSX

[molnard]

Hi there,

I am refactoring our CoinListViewModel in WasabiWallet. It works fine on Windows and Linux but on OSX it hangs randomly. I have no clue it might be Reactive so I decided to ask you guys.

Environment:

• OSX 10.15.1
• Dotnet core 3.0
• Avalonia 9 preview 7
  
  
  
  • Assembly ReactiveUI, Version=10.3.0.0
  
  
  • Assembly System.Reactive, Version=4.1.0.0
  
  
  • Assembly DynamicData, Version=6.13.0.0
  
  

Stack trace

The UI gets unresponsible and when I pause the execution it stands here:
https://github.com/zkSNACKs/WalletWasabi/pull/2602/files#diff-8ef0b233124cda8dfcef1707a9d8df4aR598

Called from here:
https://github.com/zkSNACKs/WalletWasabi/pull/2602/files#diff-8ef0b233124cda8dfcef1707a9d8df4aR462

Sometimes it hangs here too: https://github.com/zkSNACKs/WalletWasabi/pull/2602/files#diff-8ef0b233124cda8dfcef1707a9d8df4aR593

Purpose of the code

Instead of refreshing the CoinList every time when there is a change I created an event CoinListChanged. I am calling this event from several places and throttle it - so the list will be refreshed only once.

Hacky solution

If I replace this line:
CoinListChanged?.Invoke(this, null);
with
Task.Run(() => CoinListChanged?.Invoke(this, null));
It will work.

Theory

ThreadPool randomly selects the executor Thread for the tasks. If it selects the same thread which should run the task it will wait for itself forever. Or there is some circular waiting list that is not detected on OSX. I also noticed the same behavior when I used the Subscribe with WhenAnyValue and modify the same value inside the subscription - it also hangs randomly on OSX at the line: set => this.RaiseAndSetIfChanged.

This is a long-running issue in Wasabi and I always hacked it around which is not a long term solution, any suggestion appreciated.

[akarnokd]

Is there a code that blocks or locks? Can you check who is doing what when there is a deadlock? Could you try with System.Reactive 4.2.0?

Also your project looks complicated for someone outside of it to track things down. Could you create a minimal project that shows the hang issue?

[molnard]

Is there a code that blocks or locks?

No, it just rebuilds the CoinList. No locks or blocks.

Could you try with System.Reactive 4.2.0?

Upgraded, it hangs too.

Could you create a minimal project that shows the hang issue?

Yes, I can but it is not easy because I am using reactive through Avalonia and maybe that causes the problem. Just by looking at this ViewModel everything seems legit there?
https://github.com/zkSNACKs/WalletWasabi/blob/65c7f044bc3e937ce17768e4eb327b394df9e29b/WalletWasabi.Gui/Controls/WalletExplorer/CoinListViewModel.cs#L452

[akarnokd]

Rx' FromEventPattern is not thread safe by itself and can cause arbitrary bugs down the stream. If your events can come from any thread, it's possible something will break. Try applying Synchronize on those events and see if it helps.

[molnard]

Hmm, that makes sense. By Synchronize you thought about this?
Observable.FromEventPattern(this, nameof(CoinListChanged),RxApp.MainThreadScheduler)

I added the UI thread RxApp.MainThreadScheduler.
The problem is the Merge? That two different events can occur at the same time?

[akarnokd]

No, Synchronize().

Individual sources can't be driven concurrently. FromEventPattern uses a Subject to dispatch and the scheduler version is just like applying ObserveOn, so it won't likely solve the problem. The issue could be that the same event source triggers multiple events at the same time from different threads and that breaks the internals. Applying Synchronize should fix that case. If not, further investigations are needed.

[quinmars]

Observable
    .Merge(Observable.FromEventPattern(this, nameof(CoinListChanged)).Select(_ => Unit.Default))
    .Throttle(TimeSpan.FromSeconds(2)) // Throttle TransactionProcessor events adds/removes.
    .Merge(Observable.FromEventPattern(this, nameof(CoinListShown)).Select(_ => Unit.Default)) // Load the list immediately.
    .ObserveOn(RxApp.MainThreadScheduler)

The subscription to C# events is usally not thread safe, hence you have to be careful where the event subscription happens. Rx has some fences there to force subscription on the UI thread, but I do not know if they work for avalonia. In the second Observable.FromEventPattern will subscibe on the Throttle thread and not on the UI thread. You could add to that observable an SubscribeOn(RxApp.MainThreadScheduler) or even better use the main thread scheduler already for the throttling .Throttle(TimeSpan.FromSeconds(2), RxApp.MainThreadScheduler). I do not know, however, if that causes the dead lock you observing.

BTW I see that you are using ObserveOn in many places. In my experience the view model should have an explicit UI thread affinity, although that is not necessarily by WPF (I do not know how Avalonia handles this). So if you only subscribe to the view model via WhenAny you only need ObserveOn if you are leaving the main thread with a time based operator like Throttle.

[molnard]

So I did this and looks like the situation improved. I added Synchronize to every Observable.FromEventPattern.
C# Observable .Merge(Observable.FromEventPattern<ReplaceTransactionReceivedEventArgs>(Global.WalletService.TransactionProcessor, nameof(Global.WalletService.TransactionProcessor.ReplaceTransactionReceived)).Select(_ => Unit.Default).Synchronize()) .Merge(Observable.FromEventPattern<DoubleSpendReceivedEventArgs>(Global.WalletService.TransactionProcessor, nameof(Global.WalletService.TransactionProcessor.DoubleSpendReceived)).Select(_ => Unit.Default).Synchronize()) .Merge(Observable.FromEventPattern<SmartCoin>(Global.WalletService.TransactionProcessor, nameof(Global.WalletService.TransactionProcessor.CoinSpent)).Select(_ => Unit.Default).Synchronize()) .Merge(Observable.FromEventPattern<SmartCoin>(Global.WalletService.TransactionProcessor, nameof(Global.WalletService.TransactionProcessor.CoinReceived)).Select(_ => Unit.Default).Synchronize()) .Subscribe(args => { try { OnCoinListChanged(); } catch (Exception ex) { Logger.LogError(ex); } }) .DisposeWith(Disposables);

As far as I can understand I do not need to sync events from different sources but I have to sync events from the same source. So there is no need to create an object myLock and add as a parameter to sync. Throttle is OK with that?

[molnard]

Thank you for your help @akarnokd and @quinmars. I solved the problem. I elaborated on the problem and a deeper I went the dirtier it became but anyway:

The first one is to use Synchronize(refreshLock) in case if an event source can trigger the same event at the same time. For example, I had a list of items. Every item has a status that can be changed for several reasons. In the lists view model, I need to detect if any of the statuses of the items are changed or not. Subscribe with WhenAnyValue() to every item and sync with the same lock.

The second problem was with Throttle. As @quinmars mentioned Throttle can change the thread. In our project, we are always ObserveOn the UI thread before every Subscribe. In this case, combining Throttles and creating Observable on top of Throttled property caused deadlocks. Try to avoid multiple Throttling. Always use the Model properties as the source of the Observable and use Throttle on them and ObserveOn at the end once.

Useful articles:
http://introtorx.com/Content/v1.0.10621.0/15_SchedulingAndThreading.html
https://jonskeet.uk/csharp/threads/lockchoice.html