reactiveui / ReactiveUI

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming. ReactiveUI allows you to abstract mutable state away from your user interfaces, express the idea around a feature in one readable place and improve the testability of your application.
https://www.reactiveui.net
MIT License
8.11k stars 1.12k forks source link

[Bug]: Property observers on the same object, but with different schedulers, sometimes run on the wrong scheduler #3621

Open mark-deepcellbio opened 1 year ago

mark-deepcellbio commented 1 year ago

Describe the bug 🐞

If an object has multiple properties that observe another object with WhenAnyValue().ToProperty(), and some (but not all) of them use ObserveOn to ensure they are updated on the UI thread, they are sometimes updated on background threads anyway.

I think the minimal scenario is:

If the business object's property change events fire too rapidly, the binding will try to update the view on a background thread, which causes a crash under WPF.

Step to reproduce

Run this test.

ObserveOnTests.cs.txt

Case 2, where Counter2 observes the business object on CurrentThreadScheduler, will fail somewhere in the first few hundred iterations.

(The same behavior happens if the scheduler is passed to ToProperty.)

Reproduction repository

https://github.com/reactiveui/ReactiveUI

Expected behavior

I expect that a property defined with ObserveOn(MainThreadScheduler).ToProperty() will always update on the main thread.

Screenshots 🖼️

No response

IDE

No response

Operating system

Windows 11

Version

No response

Device

No response

ReactiveUI Version

18.3.1 and 19.4.1

Additional information ℹ️

The real case where this came up is in a WPF app that uses a mix of ReactiveUI bindings (to view model properties that marshal to the UI thread) and XAML bindings (to properties that don't marshal, because that's allowed). I'm aware this is not the recommended way to build view models.

ChrisPulman commented 1 year ago

Thank you for reporting this, we will take a look as soon as possible to see how best to resolve this.

glennawatson commented 1 year ago

Whats your RxApp.MainThreadScheduler type when you look at it.

You need to make sure you set WPF .NET based applications to a minimum of net6.0-windows10.0.19041 otherwise the MainThreadScheduler can be set to the incorrect object.

This is due to downstream dependencies requiring a minimum or net6.0-windows10.0.19041

mark-deepcellbio commented 1 year ago

@glennawatson We are targeting net7.0-windows. RxApp.MainThreadScheduler is type ReactiveUI.WaitForDispatcherScheduler.

glennawatson commented 1 year ago

Target net7.0-windows10.0.19041

You can still run on older SDK versions you just get bug fixes with the newer SDK

System.Reactive has that minimum target (which is not something we control in ReactiveUI)

anaisbetts commented 1 year ago

This is again by-design, ViewModels inherit the thread affinity of their views - meaning that once you bind a ViewModel, you should only set its properties on the UI thread. ReactiveUI intentionally does not solve this problem because if it did, it would cause threading "convoys" where scheduling delays propagate and build up in large applications.

Instead of:

// On some other thread
someViewModel.Foo = "Bar";

Do:

// On some other thread
RxApp.MainThreadScheduler.Schedule(() => someViewModel.Foo = "Bar");

Basically the short version is, writing WhenAnyValue.ObserveOn is always a bug. Instead you need to track down the code doing the Setting and fix That instead.

glen-nicol commented 6 months ago

Howdy, @anaisbetts could we get the requirement for viewmodel properties being updated on the UIThread documented on the databindings page or maybe somewhere related to viewmodel properties? I think it needs to be documented because in vanilla WPF (I assume other XAML framework too), bindings made in XAML handle other threads updating a viewmodel property and there isn't even a trace log message about it being remotely problematic.

I think the requirement has good intentions, but it is surprising when you hit it coming from vanilla and annoying that there is no workaround to get a scheduler in there given that some legacy viewmodel code may be written with the vanilla assumption.

Adding insult to injury in most cases(maybe all cases by design?) BindTo is being called from a view where it could easily access the UI thread Dispatcher to do the scheduling. But I realize that probably can't be done in a single portable library and instead would need implementations for each framework.

My final thought about this is that having to add .ObserveOn() wherever viewmodel properties are set sounds like a mess.