[Proposal] Remove automatic UI thread marshaling from INotifyCollectionChanged handlers in Forms

[hartez]

UPDATE

If you have not read the background information provided in this post AND the discussion of PR #5866, stop. Hit the 'Cancel' button below the reply you're typing. Read this post, read the discussion thread of PR #5866, and then come back.

END UPDATE

This proposal is a breaking change for some users.

In all cases where Forms automatically marshals handling of the INotifyCollectionChanged interface's CollectionChanged event to the UI thread, we instead should throw an exception if the event is raised on a non-UI thread.

Background

In all of the platforms supported by Xamarin.Forms, any changes to the UI have to be made on the main/UI thread. Trying to make a UI change from a non-UI thread (e.g., a thread from the thread pool) will result in some version of a cross-thread exception.

In .NET, we often bind controls with multiple items to collections which implement the INotifyCollectionChanged interface. The interface contains a CollectionChanged event which the control can listen for. When the event is raised, the control can update itself to reflect the change in the underlying data.

These two concepts collide when changes are made to one of these collections from a non-UI thread. When the data changes in a non-UI thread, the CollectionChanged event is raised and handled on that same non-UI thread; if those changes would result in a control change which modifies the UI (as most of them do), then those UI changes are happening off of the UI thread. A cross-thread exception ensues.

There are two options to avoid this problem:

1. Make your collection changes on the UI thread.
2. Handle the CollectionChanged event on the UI thread.

Option 1 is the better option. Option 2 introduces some subtle problems, as we'll see later.

A long, long time ago (so far back that I can't link the commit because it's in the pre-open-source repo), a well-intentioned change was made to the ListProxy class (which handles a lot of bookkeeping for the ListView control). The most recent version of the change can be found here. This code is basically automating option 2. The handler for the CollectionChanged event is running the update action using the Dispatcher. The Dispatcher checks to see if it's on the UI thread; if so, it immediately runs the update action. If not, it queues up the action on the UI thread. In the latter case, the action won't actually be executed until the UI thread can get to it, which might be immediately or it might be at some point in the future.

The intent of this change was to make the ListView easier to use; if you doing something off the main thread (say, processing a bunch of data) and needed to update the collection, you could safely do so - the actual update to the UI would be marshaled to the UI thread by ListProxy, and you wouldn't get a cross-thread exception.

At the time, this was the only control in Forms which performed automatic marshaling when bound to an INotifyCollectionChanged source. Other controls notably did not do this. The Pins collection for Xamarin.Forms.Maps, for example, required you to handle the marshaling yourself: see the discussion in PR 5866.

After a whole lot of discussion, we decided to make the other INotifyCollectionChanged sources in Forms consistent with ListView. So we merged PR 5866 (for the Map Pins) and PR 8568 (for CollectionView). The intent was to make working with these collections/bindings off of the UI thread as simple and easy as possible. We thought we were improving the product.

We were wrong.

Why the current solution does not work

The problem occurs under the following conditions:

1. The data changes.
2. An update to the control is queued up on the main thread. It will run later.
3. The control does something which depends on the current state of the data.
4. The queued update on the main thread is processed.

It's during part 3 that things go pear-shaped. The control has not yet been notified that the underlying data has been changed; it has expectations about the shape of the data (e.g., number of items, order of items) which are no longer valid. When it tries to query the underlying data, the results are not in the expected shape and the control has to throw a consistency exception.

An example of this is issue 10735. The Android RecyclerView is attempting to animate some data updates, but the underlying data has already been changed; it's querying for data based on an index which is no longer valid, and an IndexOutOfBoundsException is thrown. Issue 9753 is a similar case.

The problem is the gap between the update of the actual underlying data and the notification to the native control (the RecyclerView or ListView or UICollectionView) that the data has changed. Because the updates have to be queue on the UI thread, they are sometimes delayed, and during that delay any work the control does which queries the underlying data will get inconsistent results.

So simply pushing the handling of CollectionChanged onto the UI thread is a bust; it'll work a lot of the time, but when it fails it fails hard and in ways which are difficult to reproduce/diagnose.

In the end, this means that the only way to safely update the data in a bound observable collection (INotifyCollectionChanged) is to do so on the UI thread (i.e., option 1 above).

Thus, the proposal to remove all automatic marshaling for these collections.

What things look like if we don't accept this proposal

We currently have a workaround for this on Android in the form of PR 11235's MarshalingObservableCollection, which automatically wraps any INCC used as a CollectionView ItemsSource on that platform. (To be clear - this problem is not limited to Android; all the platforms have it. Android is just the most common problem area right now, and we may apply MOC to the other platforms in the future). MOC basically watches for off-UI-thread changes in the wrapped collection and presents a snapshot of the pre-modification data to the CollectionView until the notifications can be processed. In short, it lies.

While this works to fix the two reported issues, we don't know if it covers all possibilities. And it adds additional overhead (in both memory allocation and CPU cycles) for everyone using CollectionView, even if they aren't adding items off of the UI thread. And it adds additional maintenance overhead for Forms (and going forward, .NET MAUI).

So the trade-off is "more maintenance and less performance" for "some devs can add collection items from thread pool threads without having to call BeginInvokeOnMainThread". Keep in mind that we cannot even guarantee that the continued behavior won't run into further bugs.

What things look like if we accept this proposal

From a maintenance perspective for Forms and .NET MAUI, things get simpler. We can drop some classes and their tests, and we have an entire class of bugs we don't have to worry about anymore.

The trade off is that those devs who are relying on the current behavior (that they can add items to a collection off of the main thread without cross-thread exceptions) would have to change their code. The change would be from something like this:

// From a non-UI thread
myCollection.Add("item");

to this:

// From a non-UI thread
Device.BeginInvokeOnMainThread(() => myCollection.Add("item"));

In an attempt to surface this particular problem to the developer as soon as possible, at each location where we have been marshaling the event handler to the UI thread, we will instead throw an exception with very specific language regarding the change and how to address it.

[mattleibow]

How do the other data binding platforms work? I think that I usually have to marshal myself for say UWP?

[hartez]

Yes, you do.

[adammeaney]

I would ask what we believe the real replacement is to this, because I don't believe that anyone ever should write

Device.BeginInvokeOnMainThread(() => myCollection.Add("item"));

In their VM. Forms code does not belong in your ViewModel.

Are we suggesting everyone wraps that in an IMainThread interface in their project and injects it into their ViewModel?

Are we suggesting we use SynchronizationContext and get the one from the UI thread and inject that?

[mackayn]

Use that approach already with IDeviceService in Prism so the viewmodel can be unit tested and that service mocked via base view model. I'd prefer Forms is consistent and doesn't try and marshal to the UI thread for you.

Agree the vm should not reference Xamarin.Forms if possible.

[hartez]

I would ask what we believe the real replacement is to this, because I don't believe that anyone ever should write

Device.BeginInvokeOnMainThread(() => myCollection.Add("item"));

In their VM. Forms code does not belong in your ViewModel.

I'm not suggesting that you should put any of this code into your view model. Or at least, I didn't intend to suggest that. I'm simply saying that if you have an INotifyCollectionChanged in your view model, any changes to that collection should be made from the UI thread.

To be honest, that goes for all view model changes. Your view model is an abstraction of your view. It is, by its very nature, tied to the UI and thus the UI thread. A change to the properties of your view model is a change to your UI. So when working with a UI which requires that all modifications happen on the UI thread (which is most of them), it follows that you should never be making changes to the view model from another thread. (Forms lets you get away with this for properties and we tried to let you get away with it for collections, but, well, see the root post.)

So no, don't add BeginInvokeOnMainThread() to your view model. And if you've got code that's running on a non-UI thread and making view model changes, I would suggest changing that (at least if the part of the view model that you're changing is or affects an INotifyCollectionChanged).

[mackayn]

I really don't see the problem with this change, it just makes things clearer.

[Tommigun1980]

[adammeaney]

What is the "correct thread"? From my ViewModel's perspective, why would I need to have a collection changed on the main thread. The only reason to do it is because the framework requires it, and if the framework is the one requiring it, the framework can fix it.

From my viewmodel, I might not even know what thread I am on most of the time. I would say most of my ViewModel operations don't really care what thread they were on, except they put network requests on the background thread.

After I get back from that network operation, why would I bother to go back to the main thread? From my ViewModel, there is no reason to do it. Only because Xamarin wants me to do it.

And if Xamarin requires all things that touch the View to be on the View thread, why don't they make that happen instead of me?

What if Xamarin requires INCC on the main thread, and WPF (this is an example, dont know how it works there), which my VMs could also target, requires INPC on the main thread too. So now I need to wrap every single property change in invoke on main thread, even though Xamarin doesn't need it.

Littering my VM with threading stuff to get to the main thread just because Xamarin needs it doesnt really feel like it should be my responsibility, but theirs.

[hartez]

Only because Xamarin wants me to do it.
The only reason to do it is because the framework requires it, and if the framework is the one requiring it, the framework can fix it.

It's not an arbitrary requirement we created. It's a fundamental requirement of the native platforms. AFAIK, every UI platform we currently target requires this.

And if Xamarin requires all things that touch the View to be on the View thread, why don't they make that happen instead of me?

We do make that happen for bindable properties (though I'd argue we shouldn't). Most frameworks don't. Please read the background information in the PR #5866 discussion.

And we technically can handle it for you for these collections in many situations, but doing so comes at a cost in performance for everyone, even if they aren't doing work off of the main thread.

What if Xamarin requires INCC on the main thread, and WPF (this is an example, dont know how it works there), which my VMs could also target, requires INPC on the main thread too. So now I need to wrap every single property change in invoke on main thread, even though Xamarin doesn't need it.

Well, first off - WPF and Forms do have the same requirements here. Also, WPF does not do the marshaling for you, so if you wanted to use your view models across the two platforms the same concerns would apply.

But even assuming they didn't, this is only an issue if you are making view model changes off of the UI thread. If you never do that, you never have to worry about this issue.

From my viewmodel, I might not even know what thread I am on most of the time. I would say most of my ViewModel operations don't really care what thread they were on, except they put network requests on the background thread.

Think about that statement for a bit. Your view model obviously does care what thread it's on if you put your network requests on "the background thread". You wouldn't bother to do that if you didn't care about blocking the UI thread.

Anyway, if the concern is about changes which may come from these background network calls, you just need to marshal the callback onto the UI thread. No need to wrap every VM property.

[programmation]

I'm not convinced that forcing developers to marshall data updates to the UI thread in ViewModels will solve the underlying issue. The issue as described appears to be that the control is referencing out of date data when there's an update queued to the main thread but not yet processed, so to me it seems more logical that the fix should be applied to that mechanism once, in the control, rather than every developer having to create their own version of it for all time from here on out.

[KPixel]

Please, post an example of code running in Initialize() that would be problematic.

For example, this code doesn't require any marshaling: (based on the doc)

public override async Task Initialize()
{
    await base.Initialize();

    MyBoundLabel = "Loading...";

    var items = await _dataService.RetrieveData();

    foreach (var item in items)
    {
        MyBoundObservableList.Add(item);
    }
    MyBoundLabel = "Done";
}

[mackayn]

Please, post an example of code running in Initialize() that would be problematic.

For example, this code doesn't require any marshaling: (based on the doc)

public override async Task Initialize()
{
    await base.Initialize();

    MyBoundLabel = "Loading...";

    var items = await _dataService.RetrieveData();

    foreach (var item in items)
    {
        MyBoundObservableList.Add(item);
    }
    MyBoundLabel = "Done";
}

Yeah, but it slower, setting RetrieveData to run of the UI thread runs faster, you then push changes back to the UI thread. I know that code is safe but I wouldn't write it like that.

[KPixel]

In practice, the code inside RetrieveData() will typically be a network call (to a web API) followed by a bit of code to deserialize the response.

Something like:

public async Task<ItemList> RetrieveData()
{
    var json = await _httpClient.GetStringAsync("https://example.com/api/item/list").ConfigureAwait(false);
    var items = System.Text.Json.JsonSerializer.Deserialize<ItemList>(json);
    return items;
}

The network call is the part that takes the most time by far (let's say, at least 50 milliseconds) and all that time will be spent off the UI thread. Then, the Deserialize() call will typically happen off the UI thread as well, and in any case only take some nanoseconds.

When we return the items to Initialize(), we will come back on the UI thread, and quickly add them to the list.

I think this implementation, without automatic marshaling, is actually the fastest one! If we had left the UI thread explicitly, then each, each call to MyBoundObservableList.Add(item); with automatic marshaling would result in a lot of code to move each item back to the UI thread individually.

Please, read this blog post for more details:

https://devblogs.microsoft.com/dotnet/configureawait-faq/#when-should-i-use-configureawaitfalse

[mackayn]

In practice, the code inside RetrieveData() will typically be a network call (to a web API) followed by a bit of code to deserialize the response.

Something like:

public async Task<ItemList> RetrieveData()
{
    var json = await _httpClient.GetStringAsync("https://example.com/api/item/list").ConfigureAwait(false);
    var items = System.Text.Json.JsonSerializer.Deserialize<ItemList>(json);
    return items;
}

The network call is the part that takes the most time by far (let's say, at least 50 milliseconds) and all that time will be spent off the UI thread. Then, the Deserialize() call will typically happen off the UI thread as well, and in any case only take some nanoseconds.

When we return the items to Initialize(), we will come back on the UI thread, and quickly add them to the list.

I think this implementation, without automatic marshaling, is actually the fastest one! If we had left the UI thread explicitly, then each, each call to MyBoundObservableList.Add(item); with automatic marshaling would result in a lot of code to move each item back to the UI thread individually.

Please, read this blog post for more details:

https://devblogs.microsoft.com/dotnet/configureawait-faq/#when-should-i-use-configureawaitfalse

That's not my experience in optimizing Xamarin apps and I've read many articles on this topic but I think we'll just go round in circles on this point arguing. I have no problem with the proposed change, just makes things clearer.

[KPixel]

If you have the time, do post some links to these articles. I do want to learn more, if I am missing something.

[KPixel]

I agree with the idea that this behavior should not be implicit.
However, I would like a solution that supports multi-window apps: What if a collection is bound to two views on two different windows?

I'm currently working on that scenario, and I have already been hit by a few bugs in X.F related to the assumption that there is only ever a single window.

BTW, Device.BeginInvokeOnMainThread() should be avoided for that reason.

[KPixel]

So, what would be the recommended solution for that scenario? (A collection is bound to two views on two different windows)

[hartez]

So, what would be the recommended solution for that scenario? (A collection is bound to two views on two different windows)

I would recommend not doing that.

If I'm understanding your question correctly, you have a single instance of a view model, and you are binding controls from two different windows to that single instance. I'm going to assume that you aren't doing any two-way binding, or if you are it's not to controls in both windows (since that would have other obvious problems).

I would recommend having a separate view model instance for each view, and updating each instance on the appropriate UI thread. That way you avoid any unintentional (and potentially hard to debug) conflicts between the two threads. Of course, this depends a lot on what's actually in your view model, and your model, and how you update them.

[KPixel]

I see the ViewModel as an extension to the View itself. So, with that understanding, I agree that each view/window should have its own ViewModel.

Here, the hypothetical collection that I want to share is a static list. For example, let's say it is a game that allows people to open multiple instances, each in its own window. Then, the players can each play on their own window (with touch screens which support multiple parallel inputs). Then, we have a leaderboard page to display the highest scores.
So, all the open windows must have access to this list, and any time a player wins, that list is updated, which affects all the windows.
How do we share this list?

[hartez]

In that case I'd recommend that each time your static list is updated, you broadcast that change to all the interested parties (windows, controls, etc.) with an event aggregator (or whatever your preferred pub/sub solution is). The listeners can update the various view models which back your actual controls.

This decouples the native controls entirely (e.g., ListView or RecyclerView or UICollectionView) from your primary static list. Then you don't have to worry at all about inconsistencies between the native controls' internal bookkeeping and the static list.

If you have multiple list controls in multiple windows (and thus on multiple threads) all accessing the same static list, you're pretty much guaranteed to have a thread-safety issue. For example:

1. Window B's ListView accesses the static list's Count, and gets 8
2. Window A's thread removes an item from the list
3. Window B's ListView tries to animate in the 8th item (which no longer exists).

Give them all their own copy, and you avoid issues like this.

[KPixel]

If I understand correctly, that's basically what the MarshalingObservableCollection class does, right?

Hence, my suggestion:

myCollectionView.ItemsSource = new MarshalingObservableCollection(mySharedCollection, Dispatcher);

Edit: Of course, updated to pass a dispatcher.

[mattleibow]

What is the correct way to marshal to the main thread? For example, lets say I press a button which downloads some data. This operation has a callback/event that fires on the download background thread.

And, this VM that I am in does not reference the Xamarin.Forms (or any UI) framework.

What is the recommendation?

[adammeaney]

@hartez Guess we are just saying that the official advice is wrap IDispatcher, give it to your viewmodels, and anything that is an event (INPC, INCC, maybe your own events the view is listening to for some reason) should be queued up using the wrapped IDispatcher.BeginInvokeOnMainThread?

[hartez]

It should not be the caller's responsibility to invoke changes to collections to the UI thread.

It is on most platforms.

An excellent example of this is VMs - none of my VMs have any kind of UI code in there and are purely business logic. I don't want to break that by suddenly adding Xamarin specific thread handling code there, or add yet another layer.

Do you operate on your view model off of the UI thread? If you don't, then this is of no concern to you. If you have code which modifies your view model off of the UI thread, then you will need to marshal those calls onto the UI thread. You do not need to add any thread handling code to the view model itself.

I can't even fathom the amount of code smell this would introduce, not to mention, once again, making VMs and separation of concern utterly impossible.

I must not be conveying this point well: what we are talking about here is reverting a change which was made 8 months ago. So either your code prior to version 4.3 of Forms already had the code smell you are worried about, or this issue does not affect you.

Guess we are just saying that the official advice is wrap IDispatcher, give it to your viewmodels, and anything that is an event (INPC, INCC, maybe your own events the view is listening to for some reason) should be queued up using the wrapped IDispatcher.BeginInvokeOnMainThread?

No. That is absolutely not the official advice.

If, for some reason, you have code somewhere in your application

1. which modifies a collection
2. and that collection implements INotifyCollectionChanged
3. and that collection is the ItemsSource for a control (Map.Pins, CollectionView, etc.)
4. and that code does not run on the UI thread

then that code should be marshaled to the UI thread, using whatever means you prefer (I would suggest IDispatcher).

How you accomplish that is entirely up to you.

[hartez]

a callback/event that fires on the download background thread

Presumably your callback modifies the collection? Then the callback should marshal that modification to the UI thread.

Think of it this way - what would you do in that callback if you weren't using some form of loose coupling like MVVM? If that callback just changed the text on a UILabel directly by setting the Text property? You'd have to marshal that call with something like BeginInvokeOnMainThread. Same deal here.

Forms conveniently lets you ignore that requirement much of the time. This proposed change is about rolling back a few particular instances of that convenience because they introduce other problems and hurt performance.

[adammeaney]

And in that rolling back of this convenience, you have argued that you should roll back that convenience on property changes too.

We do make that happen for bindable properties (though I'd argue we shouldn't). Most frameworks don't. Please read the background information in the PR #5866 discussion.

But the basic thrust of what I am saying in this thread is that you recommended we use IDispatcher to handle any call that should be happening on the main thread. But any way of finding the main thread and running on it is ok for the same solution. Is that not what you meant by that?

I have never known or cared which thread started a ViewModel command or property update, just that I don't block it when doing things I know will be long running. Whether I started on a background thread or the UI thread doesn't matter to my View Model, except in the fact that all my INCC (and potentially my INPC, if you decide to undo that convenience as well as it should be my responsibility) need to be on the main thread now.

[hartez]

And in that rolling back of this convenience, you have argued that you should roll back that convenience on property changes too.

I personally don't think we should auto-marshal property bindings, no. But that's not part of this proposal. This proposal is only about INotfiyCollectionChanged. Basic binding expressions are not on the table here.

The INotifyCollectionChanged situation is being treated separately because

1. The auto-marshaling is a relatively recent change (prior to December 2019 it only existed for ListView)
2. It causes some demonstrable issues with the underlying controls
3. The solutions for those problems add a performance hit for everyone, not just the folks who are doing things off the UI thread
4. The solutions for those problems add a maintenance burden to Forms, because they are hard to diagnose/debug when they go wrong

So we're proposing to roll that change back.

But the basic thrust of what I am saying in this thread is that you recommended we use IDispatcher to handle any call that should be happening on the main thread. But any way of finding the main thread and running on it is ok for the same solution. Is that not what you meant by that?

I'm trying to parse this paragraph, but I'm not getting it. If you need a cross-platform way of marshaling stuff to the UI thread, then yes, I recommend IDispatcher because it will work correctly in multi-window scenarios. But feel free to use whatever you like to marshal the call to the UI thread.

I have never known or cared which thread started a ViewModel command or property update, just that I don't block it when doing things I know will be long running.

Do you write a lot of code where you don't know which thread you're on?

and potentially my INPC, if you decide to undo that convenience as well

Again, INPC is not on the table. INCC only.

So it sounds like your preference is strongly toward "I am fine with a small performance hit as long as I never have to think about threads."

[KPixel]

@hartez I just saw the update in your initial post. I think that it is too much reading for everyone :)

I would suggest having a short example right from the start to illustrate the problem that this proposal is trying to solve. Maybe, you can copy the GetData() code here.

[mattleibow]

Looking at the code in the linked issues in the update section really helped.

[GiampaoloGabba]

if the result of this rollback is more performance & stability, less bug, more maintanability, and align with UWP/WPF behaviours this is a no-brainer yes to me.... We should not even speaking of this, the PROS are simply too big :)

Maybe we can maintain the auto-marshaling for collections as opt-in (or opt-out) and update the documentation with a clear statement that this behaviour will have performance and stability downsides (basically: use it at your own risk). With this we dont break any existing code and we can move one from those nasty bugs.