BusyIndicator allows to fire RelayCommand twice

Question

I have a WPF application, which follows MVVM.

To make UI responsive, I'm using TPL for executing long-running commands and BusyIndicator to display to user, that application is busy now.

In one of the view models I have this command:

public ICommand RefreshOrdersCommand { get; private set; }

public OrdersEditorVM()
{
    this.Orders = new ObservableCollection<OrderVM>();
    this.RefreshOrdersCommand = new RelayCommand(HandleRefreshOrders);

    HandleRefreshOrders();
}

private void HandleRefreshOrders()
{
    var task = Task.Factory.StartNew(() => RefreshOrders());
    task.ContinueWith(t => RefreshOrdersCompleted(t.Result), 
        CancellationToken.None, TaskContinuationOptions.OnlyOnRanToCompletion, TaskScheduler.FromCurrentSynchronizationContext());
    task.ContinueWith(t => this.LogAggregateException(t.Exception, Resources.OrdersEditorVM_OrdersLoading, Resources.OrdersEditorVM_OrdersLoadingFaulted),
        CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.FromCurrentSynchronizationContext());
}

private Order[] RefreshOrders()
{
    IsBusy = true;
    System.Diagnostics.Debug.WriteLine("Start refresh.");
    try
    {
        var orders = // building a query with Entity Framework DB Context API

        return orders
            .ToArray();
    }
    finally
    {
        IsBusy = false;
        System.Diagnostics.Debug.WriteLine("Stop refresh.");
    }
}

private void RefreshOrdersCompleted(Order[] orders)
{
    Orders.RelpaceContent(orders.Select(o => new OrderVM(this, o)));

    if (Orders.Count > 0)
    {
        Orders[0].IsSelected = true;
    }
}

IsBusy property is bound with BusyIndicator.IsBusy, and RefreshOrdersCommand property is bound with a button on a toolbar, which is placed inside BusyIndicator in the view of this view model.

The problem.

If the user clicks on button not frequently, everything works fine: BusyIndicator hides toolbar with the button, data becomes loaded, BusyIndicator disappears. In the output window I can see pairs of lines:

Start refresh. Stop refresh.

But if the user clicks on button very frequently, look like BusyIndicator doesn't hide toolbar in time, and two background threads attempt to execute RefreshOrders method, which causes exceptions (and it is OK, because EF DbContext isn't thread-safe). In the output window I see this picture:

Start refresh. Start refresh.

What am I doing wrong?

I've looked into BusyIndicator's code. I have no idea, what can be wrong there: setting IsBusy just makes two calls to VisualStateManager.GoToState, which, in turn, just makes visible a rectangle, which hides BusyIndicator's content:

                    <VisualState x:Name="Visible">
                       <Storyboard>
                          <ObjectAnimationUsingKeyFrames BeginTime="00:00:00" Duration="00:00:00.001" Storyboard.TargetName="busycontent" Storyboard.TargetProperty="(UIElement.Visibility)">
                             <DiscreteObjectKeyFrame KeyTime="00:00:00">
                                <DiscreteObjectKeyFrame.Value>
                                   <Visibility>Visible</Visibility>
                                </DiscreteObjectKeyFrame.Value>
                             </DiscreteObjectKeyFrame>
                          </ObjectAnimationUsingKeyFrames>
                          <ObjectAnimationUsingKeyFrames BeginTime="00:00:00" Duration="00:00:00.001" Storyboard.TargetName="overlay" Storyboard.TargetProperty="(UIElement.Visibility)">
                             <DiscreteObjectKeyFrame KeyTime="00:00:00">
                                <DiscreteObjectKeyFrame.Value>
                                   <Visibility>Visible</Visibility>
                                </DiscreteObjectKeyFrame.Value>
                             </DiscreteObjectKeyFrame>
                          </ObjectAnimationUsingKeyFrames>
                       </Storyboard>
                    </VisualState>

...and disables content:

                    <VisualState x:Name="Busy">
                       <Storyboard>
                          <ObjectAnimationUsingKeyFrames BeginTime="00:00:00" Duration="00:00:00.001" Storyboard.TargetName="content" Storyboard.TargetProperty="(Control.IsEnabled)">
                             <DiscreteObjectKeyFrame KeyTime="00:00:00">
                                <DiscreteObjectKeyFrame.Value>
                                   <sys:Boolean>False</sys:Boolean>
                                </DiscreteObjectKeyFrame.Value>
                             </DiscreteObjectKeyFrame>
                          </ObjectAnimationUsingKeyFrames>
                       </Storyboard>
                    </VisualState>

Any ideas?

Update. The question isn't about how to prevent command from reentrance. I'm wondering about mechanism, which allows to press button twice.

Here's how it works (from my point):

• ViewModel.IsBusy is bound with BusyIndicator.IsBusy. The binding is synchronous.
• Setter of BusyIndicator.IsBusy calls VisualStateManager.GoToState twice. One of these calls makes visible a rectangle, which overlaps BusyIndicator's content (the button, in my case).
• So, as I understand, I can't physically achieve the button after I've set ViewModel.IsBusy, because all of these things happen on the same (UI) thread.

But how the button becomes pressed twice??

Answer 1

I wouldn't try to rely on a busy indicator to control valid program flow. The command execution sets IsBusy, and it should itself not run if IsBusy is already True.

private void HandleRefreshOrders()
{
    if (IsBusy)
        return;
...

You can also bind the button's Enabled state to IsBusy as well, but I think the core solution is to guard your commands from unintended re-entrance. Kicking off a worker will also add some complexity. I'd move the setting of the status to the HandleRefreshOrders then handle the resetting of the state in the ContinueWith implementation. (Some additional advice/reading may be needed to ensure it's thread-safe.)

*Edit: To clarify moving the IsBusy to avoid double-execution:

private void HandleRefreshOrders()
{
    if (IsBusy)
        return;

    IsBusy = true;

    var task = Task.Factory.StartNew(() => RefreshOrders());
    task.ContinueWith(t => 
            {
                RefreshOrdersCompleted(t.Result);
                IsBusy = false;
            }, 
        CancellationToken.None, TaskContinuationOptions.OnlyOnRanToCompletion,  TaskScheduler.FromCurrentSynchronizationContext());
    task.ContinueWith(t => 
            {
                this.LogAggregateException(t.Exception, Resources.OrdersEditorVM_OrdersLoading, Resources.OrdersEditorVM_OrdersLoadingFaulted);
                IsBusy = false;
            },
    CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.FromCurrentSynchronizationContext());
}

Then remove the IsBusy references from the RefreshOrders method. Once you click on the button once, IsBusy will be set. If the user rapid-clicks, the second attempt to trigger the command will skip out. The Task will execute on a background thread, and once it is complete, the IsBusy flag will be reset and the command will respond once again. This currently assumes that calls to RefreshOrdersCompleted and LogAggregateException don't bubble exceptions, otherwise if they throw, the flag will not be reset. Can move the flag reset ahead of these calls, or use a try/finally within the annon. method declaration.