Weird race condition by using ViewModel-first (binding) approach

Question

I am experimenting with a simple ViewModel-first based WPF application and some primitive navigation logic. The application consists of two views (screens). One screen contains a button "Go forward" and the other a button "Go backward". By pressing one of the buttons a delegate command is invoked, which in turn causes the shell view-model to switch the active screen. Screen 1 switches to Screen 2, whereas Screen 2 switches to Screen 1.

The problem with this approach is, that it introduces a race condition. When clicking fast enough there is a chance that the corresponding action (go forward/go backward) is executed twice, thus causing the application to fail. The interesting thing about that is that the screen has already been changed, but the UI doesn't reflect the state changes instantaneously. Until now I never had experienced this kind of gap and I made this experiment just to prove, that a single-threaded (dispatched) WPF application is automatically thread-safe.

Does somebody have an explanation for this odd behavior? Is the WPF binding mechanism too slow, so that the button can be pressed a second time, until the UI has updated itself to represent the new screen state?

I have no idea how to fix this according to the recommendations for developing mvvm applications. There is no way to synchronize the code, because there is only one thread. I hope you can help me, because now I feel very unsecure with relying on the WPF data binding and templating system.

Zip archive containing the project files

MainWindow.xaml:

<Window x:Class="MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:WpfApplication1"
        mc:Ignorable="d"
        Title="MainWindow" Height="350" Width="525">
    <Window.Resources>
        <DataTemplate DataType="{x:Type local:Screen1}">
            <local:View1/>
        </DataTemplate>
        <DataTemplate DataType="{x:Type local:Screen2}">
            <local:View2/>
        </DataTemplate>
    </Window.Resources>
    <Window.DataContext>
        <local:ShellViewModel/>
    </Window.DataContext>
    <Grid>
        <ContentControl Content="{Binding CurrentScreen}"/>
    </Grid>
</Window>

The ShellViewModel containing a "go forward" and "go backward" method:

Public Class ShellViewModel
    Inherits PropertyChangedBase

    Private _currentScreen As Object
    Public Property Screens As Stack(Of Object) = New Stack(Of Object)()

    Public Sub New()
        Me.Screens.Push(New Screen1(Me))
        Me.GoForward()
    End Sub

    Property CurrentScreen As Object
        Get
            Return _currentScreen
        End Get
        Set(value)
            _currentScreen = value
            RaisePropertyChanged()
        End Set
    End Property

    Public Sub GoBack()
        Log("Going backward.")
        If (Me.Screens.Count > 2) Then
            Throw New InvalidOperationException("Race condition detected.")
        End If
        Log("Switching to Screen 1")
        Me.Screens.Pop()
        Me.CurrentScreen = Me.Screens.Peek()
    End Sub

    Public Sub GoForward()
        Log("Going forward.")
        If (Me.Screens.Count > 1) Then
            Throw New InvalidOperationException("Race condition detected.")
        End If
        Log("Switching to Screen 2.")

        Me.Screens.Push(New Screen2(Me))
        Me.CurrentScreen = Me.Screens.Peek()
    End Sub

End Class

The Screen class containing just a delegate command to start the action:

Public Class Screen1
    Inherits PropertyChangedBase

    Private _clickCommand As ICommand
    Private _shellViewModel As ShellViewModel

    Public Sub New(parent As ShellViewModel)
        _shellViewModel = parent
    End Sub


    Public ReadOnly Property ClickCommand As ICommand
        Get
            If _clickCommand Is Nothing Then
                _clickCommand = New RelayCommand(AddressOf ExecuteClick, AddressOf CanExecute)
            End If
            Return _clickCommand
        End Get
    End Property

    Private Function CanExecute(arg As Object) As Boolean
        Return True
    End Function

    Private Sub ExecuteClick(obj As Object)
        Threading.Thread.SpinWait(100000000)
        _shellViewModel.GoForward()
    End Sub

End Class

Answer 1

There is no weird race condition

I've run your code. There is one thread. The main one.

One thread = no race condition.

Why do you want to prove the following ?

I made this experiment just to prove, that a single-threaded (dispatched) WPF application is automatically thread-safe.

It's a bullet proof fact. One thread = thread safe (as long as you don't share data process wide, but then it's not thread safety anymore).

Binding and Method no supporting successive calls

In fact, your methods GoBack and GoForward are not supporting successive calls. They should be called one after another.

Thread safety here doesn't imply that your methods cannot be call twice in a row. If there is any task queue in the process, methods can be called twice.

What you may intend to prove is maybe the following:

Clicks are captured and processed in line, without any task queuing between the click, the property changed event raised, the dispatcher invocation, the binding / display refresh.

It's clearly Wrong!

When you call Dispatcher.BeginInvoke or Invoke, it's using internally a queue of tasks. And nothing prevent you from queuing twice the same task coming from two similar clicks for example.

To be frank, I was unable to reproduce your bug. I think it's the same thread that captures clicks that dispatch it to your code and then display it on screen. However since task for click events, display refresh are in the same queue, it is theoretically possible to enqueue two clicks before screen change. However :

• I cannot click fast enough to beat my cpu.
• I don't think the SpinWait are needed.
• Something may miss in my configuration.

Why not making your methods supporting successive calls ?

GoBack and GoBackward could check a status and do nothing if the current status is not valid.

You could have used:

1. Two screens both instantiated from the start.

2. A bool to indicate the current state (Forward or Back).

3. An enum to be more clearer in code.

4. A state machine.. no! I'm kidding.

Note: Why using a Stack to push and pop screen(only one by the way) ? and... in case, you add another thread: Stack pop / push are not thread safe. Instead use ConcurrentStack<T>