Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mark-deepcellbio opened this issue Sep 26, 2023 · 6 comments
Labels

Comments

@mark-deepcellbio
Copy link

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:

  • A ReactiveObject ("business object") has one or more properties which get updated by a worker thread.
  • Another ReactiveObject ("view model") has a property P1 which is linked to the business object by WhenAnyValue(...).ObserveOn(RxApp.MainThreadScheduler).ToProperty().
  • A third object ("view") observes P1, and its observer needs to run on the main thread.
  • But the view model also has a property P2 which is linked to the same business object, and does not use ObserveOn (or uses it with some other scheduler).

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
Copy link
Member

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

@glennawatson
Copy link
Contributor

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
Copy link
Author

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

@glennawatson
Copy link
Contributor

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
Copy link
Member

anaisbetts commented Oct 12, 2023

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
Copy link

glen-nicol commented May 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants