uno.extensions icon indicating copy to clipboard operation
uno.extensions copied to clipboard

[#MVU] Updating model crashes on net8.0-windows10.0.19041 but works fine on net8.0-desktop

Open jeromelaban opened this issue 1 year ago • 4 comments

I have created very simple app that presents a list of Items in an ItemsRepeater. All works fine on net8.0-desktop but on net8.0-windows10.0.19041 the app constantly crashes when MainPageModel.AddItem() method is called.

Model:

internal record Item(string Text, Brush Brush);

internal partial record MainPageModel
{
  public IListState<Item> Items => ListState<Item>.Value(this, () =>
  [
      new Item("A", new SolidColorBrush(Colors.DeepSkyBlue)),
      new Item("B", new SolidColorBrush(Colors.DarkOrchid)),
      new Item("C", new SolidColorBrush(Colors.DarkOrange)),
  ]);
  
  public async ValueTask AddItem()
  {
      await Items.Update(items => items.Add(GetRandomItem()), CancellationToken.None).ConfigureAwait(true); // crashes
  }
}

View:

new ItemsRepeater()
.ItemsSource(() => vm.Items)
.Layout(new StackLayout())
.ItemTemplate<Item>(item => GetItemView(item)),
 new Button()
 .Content("Add item")
 .Command(() => vm.AddItem)


private static TextBlock GetItemView(Item item) => new TextBlock()
    .Text(() => item.Text)
    .Foreground(() => item.Brush);

Note that the App crashes differently cross many executions which suggests some concurrency issue. Note that another concurrency issue is as well reported in #2349. Maybe both are related?

09:12:57:390 Exception thrown: 'System.Runtime.InteropServices.COMException' in System.Private.CoreLib.dll 09:12:57:390 Exception thrown: 'System.Runtime.InteropServices.COMException' in System.Private.CoreLib.dll 09:13:00:252 Exception thrown: 'System.Runtime.InteropServices.COMException' in WinRT.Runtime.dll 09:13:00:252 An exception of type 'System.Runtime.InteropServices.COMException' occurred in WinRT.Runtime.dll but was not handled in user code

repro: IListState.Update.crashes.zip

Originally posted by @kucint in https://github.com/unoplatform/uno.extensions/discussions/2394

jeromelaban avatar Jul 24 '24 13:07 jeromelaban

Alright, the problem is that when adding a new Item we are initializing a new SolidColorBrush in GetRandomItem() while we are on the background thread. The Dispatcher will need to be used to run that logic on the UI thread

kazo0 avatar Jul 30 '24 12:07 kazo0

For further clarity, in MVUX, anything happening in a Command is OFF the UI Thread. If you make a UI update by updating your Model using IState, the State is responsible to make sure (through the generated Bindable Proxy) to go back on the UI Thread.

The rule is to NOT deal with UI elements in a Model (like creating Brushers or others). If needed, you need to make those calls by going back on the UI Thread directly using the IDispatcher.

I would assume (needs testing) that Colors can be created off UI thread but not the Brush directly.

francoistanguay avatar Jul 30 '24 13:07 francoistanguay

The rule is to NOT deal with UI elements in a Model (like creating Brushers or others). If needed, you need to make those calls by going back on the UI Thread directly using the IDispatcher.

I would agree, @kucint having a proper separation of concerns you should never have UI types in the DataContext regardless of whether you're using MVVM or MVU.

That said as @francoistanguay suggested the Color itself should be fine as it is just a struct.

private static TextBlock GetItemView(Item item) => new TextBlock()
    .Text(() => item.Text)
    .Foreground(new SolidColorBrush().Color(() => item.Brush)));

dansiegel avatar Jul 30 '24 13:07 dansiegel

I would agree, @kucint having a proper separation of concerns you should never have UI types in the DataContext regardless of whether you're using MVVM or MVU.

@dansiegel This is very valid statement, Proper separation of concerns is critical. I took a lesson. Thanks!

kucint avatar Jul 31 '24 15:07 kucint

fyi @kucint / @dr1rrb

I will close this issue as the remaining work is tracked as part of https://github.com/unoplatform/uno.extensions/issues/2434 to highlight the UI Thread concerns when dealing with ICommands in MVUX

kazo0 avatar Aug 08 '24 15:08 kazo0