3

When a property changes its value I want to call an async method that fetches data from a web service an then updates another property to which the UI is bound causing the UI to update. It makes sense to me that the update is async as I want the UI to remain responsive while the update is going on.

Is it wrong to call an async method from the non async setter? I noticed that if the async method return void then VS does not complain but if it returns Task then visual studio complains that the call is not awaited. My code looks like this:

public int Property1
{
    set 
    {
        _property1 = value;
        NotityPropertyChanged();
        UpdateData();
    }
}

private async void UpdateData()
{
    // show data loading message
    var data = await GetDataFromWebService();
    Property2 = data;
    // hide data loading message
}

It seems to work but I wonder if I am not using async the way it was intended given the warning I get from VS if the return type is Task.

UPDATE: Several answers and comments have suggest the user of commands rather than updating in response to a change in a property. For my case I am not sure how that could be applied so am providing more details about how the UI is expected to work.

In the user interface there is date picker (which is bound to the property in question on the view model) where the user selects the date for which he wants to view records. When a new date is selected by the user, the app should show a busy indicator, and then fetch records in the background to avoid blocking the UI thread. Preferably I want the update to be initiated when the date selected, without requiring the user to push a button after the date is selected.

Would it be better to bind the SelectionChanged event of the date picker to and async command on the ViewModel or alternatively have an sync handler for the SelectionChanged which calls the update method on the view model directly?

Shane
  • 2,271
  • 3
  • 27
  • 55
  • Maybe helpful: https://stackoverflow.com/q/12144077, https://stackoverflow.com/q/44883765 – Clemens May 04 '18 at 13:10
  • 1
    Properties should not be `async`, neither getters nor setters. – dymanoid May 04 '18 at 13:27
  • You are using a VM property setter here as if it were a model action, Command or button click handler. A *click* should call a method or action, not set properties and expect indirect actions. – Panagiotis Kanavos May 04 '18 at 15:30
  • Possible duplicate of [Binding Button click to a method](https://stackoverflow.com/questions/3531772/binding-button-click-to-a-method) – Panagiotis Kanavos May 04 '18 at 15:31
  • @Panagiotis Kanavos - My UI needs to update in response to a change in date field rather than a button click - have updated by question to make this more clear. Could you please suggest how the scenario could be accommodated without initiating the update in the setter? One answer suggested listening to the PropertyChanged event but since the event is called from each property setter, I don't think this is much different from initiating the update from the property setter directly. – Shane May 05 '18 at 03:36
  • @Panagiotis Kanavos - Would it be better to trigger the update in SelectionChanged event of the date picker, or bind that event to an async command? This way, the update occurs after the property setter exits. – Shane May 05 '18 at 05:09

3 Answers3

5

Is it wrong to call an async method from the non async setter?

In short, yes. Properties should not not be kicking off asynchronous background operations in their setters.

I recommend you to read Stephen Cleary's blog post and MSDN article on the subject:

Async Programming : Patterns for Asynchronous MVVM Applications: Data Binding: https://msdn.microsoft.com/en-us/magazine/dn605875.aspx

Async OOP 3: Properties: https://blog.stephencleary.com/2013/01/async-oop-3-properties.html

You may also want to look into a functional MVVM framework such as ReactiveUI that handles this scenario by converting properties into observable streams of values that you can subscribe to: https://reactiveui.net/docs/getting-started/

mm8
  • 163,881
  • 10
  • 57
  • 88
  • 1
    There's no need for ReactiveUI here. This isn't about observing changes to properties but performing a single long running operation using one input parameter and calculating another, eg loading a Customer record using an ID entered by the user. That's the job of plain old methods or Commands. When these complete, they should either update the VM or create a new one – Panagiotis Kanavos May 04 '18 at 15:28
0

It's not the "cleanest" way as already stated, but you can leave it. It's not harmful in the first place except you are hiding an unexpected long and expensive operation behind a quick assignment.

MSDN says:

An async method can also have a void return type. This return type is used primarily to define event handlers, where a void return type is required. Async event handlers often serve as the starting point for async programs.

An async method that has a void return type can’t be awaited, and the caller of a void-returning method can't catch any exceptions that the method throws.

A simple solution could be to listen to the PropertyChanged event. This would be more like a work around to escape the setter. A better way is to make your ViewModel expose an update command by implementing the ICommand interface asynchrounous by adding asynchronous execution to it. You would then invoke this AsyncCommand from your View, whenever the property changes. You could also pass the new value as a command parameter to the ViewModel.

Community
  • 1
  • 1
BionicCode
  • 1
  • 4
  • 28
  • 44
  • On the contrary, MSDN warns that `async void` is meant for event handlers. The snippet you quoted says exactly that. A property is *not* an event handler. Using `async void` anywhere else is a serious bug. Using *properties* to kick off work is a bad design too. – Panagiotis Kanavos May 04 '18 at 15:10
  • Yes they hint at it saying that the caller gets nothing to await in return and that no exceptions can be caught. You can know that. It's not 'wrong' in a way something won't work correctly, your event handler does. But not best practice. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/async – BionicCode May 04 '18 at 15:14
  • No, it's very wrong. It's a very bad practice and you even get warnings from code analyzers. The links you post don't say what you think - when they say "primarily" they mean "not for anything else without a very good reason". The *walkthroughs* and *guidelines* clearly say to *not* do this. – Panagiotis Kanavos May 04 '18 at 15:16
  • You'll find a *lot* of SO questions where people got burned by using `async void`. In almost all cases they didn't realize that `can't be awaited` means that the application may terminate *before* that `async void` method even runs, or some other code can run before that method even though it was called later. The method's local data can be garbage collected before it finishes – Panagiotis Kanavos May 04 '18 at 15:18
  • And *service proxies* generated inside an `async void` can be garbage collected before the call to the web service completes – Panagiotis Kanavos May 04 '18 at 15:18
  • Code analyzers follow best practice recommendations. Compiler says when something is wrong. MSDN doesn't say it's wrong. They only recommend you to do it for event handlers only. For good reason. And they explained the reason. – BionicCode May 04 '18 at 15:19
  • You just suggested that the OP use this inside a *property* to call a *web service* which can easily cause an aborted call and never return any data. As for `Compiler says when something is wrong` no. NREs are wrong and yet the compiler can't catch them. – Panagiotis Kanavos May 04 '18 at 15:21
  • No I never suggested him to do it like he has done it. I even gave him a hint on the risks he is taking. I gave him a solution by telling to implement the ICommand interface. – BionicCode May 04 '18 at 15:24
  • I only meant that when a return type other than "void" is mandatory by design or required to make async work the compiler would give you a feedback. – BionicCode May 04 '18 at 15:30
0

Automatically calling updates when just a property changes is usually a bad idea of itself.
Usually a button and a command are a better plan. You could alternatively invoke a command from the UI rather than use that setter. And that'd be relatively simple to do.

At a minimum, you want a different thread for calling some web service in that sort of a way.
A fire and forget thread wouldn't exactly be terrible.
So long as you don't care so much what happens after you forgot it.

Andy
  • 11,864
  • 2
  • 17
  • 20