I managed to get myself into a bit of a pickle with some code that I was writing and so I thought I’d share – there’s nothing here that’s particularly complex or earth-shattering but it is something that I spent a good 10 minutes trying to fix before my ‘aha’ moment so maybe it’ll save someone some time in the future.
Description
This revolves around me having a XAML/.NET WinRT application which has a few pages in it and navigation between them.
I can simplify that down to 2 pages for a cut down version of the problem – let’s say a MainPage and a SubPage.
The two pages share data from a file. Specifically, the MainPage reads the data from a file and the SubPage reads the data from the same file but also modifies the data in that file.
In trying to be ‘stateless’ (and simple) I do not cache the data that’s loaded from the file across the pages, I simply hit the disk when I want the data as a page loads. I figured that I can always add caching as an optimisation at a later point.
I also wanted to avoid having a ‘Save’ button and so this means that when a user navigates from the SubPage back to the MainPage there’s a possibility that the data on that page is ‘dirty’ and so needs to be written out before it’s displayed back on the MainPage again.
This is the root cause of my problem
The Repro ‘UI’
Here’s the UI for my MainPage.xaml page;
which just displays some data loaded from a file in a GridView and allows navigation to my sub page which displays the same thing;
but also has a button which adds 10 more items into the list;
and when the user navigates back from this page I save the added data into the file so that the MainPage will be able to display the up to date data when I get back to that page.
MainPage Code
In terms of how this is put together, I have my MainPage.xaml file;
<local:BasePage x:Name="pageRoot" x:Class="MyApp.MainPage" xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:local="using:MyApp" xmlns:common="using:MyApp.Common" xmlns:d="http://schemas.microsoft.com/expression/blend/2008" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" mc:Ignorable="d"> <Page.DataContext> <local:MainPageViewModel /> </Page.DataContext> <Page.Resources> <x:String x:Key="AppName">Main Page</x:String> </Page.Resources> <Grid Style="{StaticResource LayoutRootStyle}"> <Grid.RowDefinitions> <RowDefinition Height="140" /> <RowDefinition Height="*" /> </Grid.RowDefinitions> <Grid> <Grid.ColumnDefinitions> <ColumnDefinition Width="Auto" /> <ColumnDefinition Width="*" /> </Grid.ColumnDefinitions> <Button x:Name="backButton" Click="GoBack" IsEnabled="{Binding Frame.CanGoBack, ElementName=pageRoot}" Style="{StaticResource BackButtonStyle}" /> <TextBlock x:Name="pageTitle" Grid.Column="1" Text="{StaticResource AppName}" Style="{StaticResource PageHeaderTextStyle}" /> </Grid> <Grid Grid.Row="1"> <Grid.RowDefinitions> <RowDefinition /> <RowDefinition Height="Auto" /> </Grid.RowDefinitions> <GridView ItemsSource="{Binding Data}" /> <Button Grid.Row="1" Content="Go to sub page" Command="{Binding SubPageCommand}" /> </Grid> </Grid> </local:BasePage>
and so this view is pulling in a view model (MainPageViewModel) and binding a collection of data (called Data) from it and also binding a command SubPageCommand which will navigate to the sub-page.
This page is derived from the LayoutAwarePage that ships in the VS 2012 templates but I added an intermediate base class called BasePage and the purpose of that is to attempt to ‘notify’ my view model as someone navigates to/from the page so that it can do setup/teardown work. To help with that I defined a simple interface;
interface INavigateToViewModel { void Setup(object parameter); void Teardown(); }
and then I have my BasePage check for its presence on the DataContext;
public class BasePage : LayoutAwarePage { public BasePage() { } protected override void OnNavigatedTo(NavigationEventArgs e) { base.OnNavigatedTo(e); INavigateToViewModel navigateToViewModel = this.DataContext as INavigateToViewModel; if (navigateToViewModel != null) { navigateToViewModel.Setup(e.Parameter); } } protected override void OnNavigatedFrom(NavigationEventArgs e) { base.OnNavigatedFrom(e); INavigateToViewModel navigateToViewModel = this.DataContext as INavigateToViewModel; if (navigateToViewModel != null) { navigateToViewModel.Teardown(); } } }
which hooks into OnNavigatedTo/OnNavigatedFrom in order to notify the underlying view model as the user comes to the page and leaves it. My simple view model for this page is sitting underneath this;
class MainPageViewModel : BindableBase, INavigateToViewModel { public MainPageViewModel() { this._subPageCommand = new Lazy<ICommand>( () => new SimpleCommand(OnSubPage)); } public async void Setup(object parameter) { // could inject this service, haven't bothered. IDataService dataService = DataService.Instance; this.Data = new ObservableCollection<int>( await dataService.LoadDataAsync()); } public void Teardown() { } public ObservableCollection<int> Data { get { return (this._data); } set { base.SetProperty(ref this._data, value); } } public ICommand SubPageCommand { get { return (this._subPageCommand.Value); } } void OnSubPage() { // TODO: being lazy here, could build a navigation service to // avoid this view model having to be aware of the Frame. Frame f = Window.Current.Content as Frame; if (f != null) { f.Navigate(typeof(SubPage)); } } Lazy<ICommand> _subPageCommand; ObservableCollection<int> _data; }
And so, essentially, when this view model is initialised, it will use an underlying service which it gets from DataService.Instance to load up the data (just a list of integers) from the data file and it’ll display that list. That DataService is a relatively cheap and cheerful idea and, clearly, it’s not going the whole way in terms of dependency injection and all that jazz but it could easily be refactored that way;
interface IDataService { Task SaveDataAsync(IEnumerable<int> data); Task<IEnumerable<int>> LoadDataAsync(); } class DataService : IDataService { private DataService() { } static DataService() { _instance = new Lazy<IDataService>( () => { return (new DataService()); } ); _serializer = new Lazy<DataContractJsonSerializer>( () => { return (new DataContractJsonSerializer(typeof(List<int>))); } ); } public static IDataService Instance { get { return (_instance.Value); } } public async Task SaveDataAsync(IEnumerable<int> data) { var file = await ApplicationData.Current.LocalFolder.CreateFileAsync( fileName, CreationCollisionOption.ReplaceExisting); List<int> list = new List<int>(data); using (var stream = await file.OpenStreamForWriteAsync()) { _serializer.Value.WriteObject(stream, list); } } public async Task<IEnumerable<int>> LoadDataAsync() { List<int> list = new List<int>(); try { var file = await ApplicationData.Current.LocalFolder.GetFileAsync( fileName); using (var stream = await file.OpenStreamForReadAsync()) { list = (List<int>)_serializer.Value.ReadObject(stream); } } catch (FileNotFoundException) { } return (list); } static Lazy<IDataService> _instance; static Lazy<DataContractJsonSerializer> _serializer; static readonly string fileName = "data.txt"; }
and so this is just using DataContractJsonSerializer to read a List<int> to/from a file and I take a little care to try and ensure that I Dispose of the .NET Stream that I get from that file to make sure that the file is closed when I’ve finished with it.
SubPage Code
The SubPage follows a very similar model. Here’s the UI definition in XAML;
<local:BasePage x:Name="pageRoot" x:Class="MyApp.SubPage" xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:local="using:MyApp" xmlns:common="using:MyApp.Common" xmlns:d="http://schemas.microsoft.com/expression/blend/2008" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" mc:Ignorable="d"> <Page.DataContext> <local:SubPageViewModel /> </Page.DataContext> <Page.Resources> <x:String x:Key="AppName">Sub Page</x:String> </Page.Resources> <Grid Style="{StaticResource LayoutRootStyle}"> <Grid.RowDefinitions> <RowDefinition Height="140" /> <RowDefinition Height="*" /> </Grid.RowDefinitions> <Grid> <Grid.ColumnDefinitions> <ColumnDefinition Width="Auto" /> <ColumnDefinition Width="*" /> </Grid.ColumnDefinitions> <Button x:Name="backButton" Click="GoBack" IsEnabled="{Binding Frame.CanGoBack, ElementName=pageRoot}" Style="{StaticResource BackButtonStyle}" /> <TextBlock x:Name="pageTitle" Grid.Column="1" Text="{StaticResource AppName}" Style="{StaticResource PageHeaderTextStyle}" /> </Grid> <Grid Grid.Row="1"> <Grid.RowDefinitions> <RowDefinition /> <RowDefinition Height="Auto" /> <RowDefinition Height="Auto" /> </Grid.RowDefinitions> <Button Command="{Binding GetMoreDataCommand, Mode=OneWay}" Content="Make More Data" Grid.Row="1" /> <GridView ItemsSource="{Binding Data}" /> </Grid> </Grid> </local:BasePage>
and here’s the ViewModel that sits behind that;
class SubPageViewModel : BindableBase, INavigateToViewModel { public SubPageViewModel() { this._getMoreDataCommand = new Lazy<ICommand>( () => new SimpleCommand(OnGetMoreData)); } public async void Setup(object parameter) { // could inject this service, haven't bothered. IDataService dataService = DataService.Instance; this.Data = new ObservableCollection<int>( await dataService.LoadDataAsync()); } public async void Teardown() { IDataService dataService = DataService.Instance; await dataService.SaveDataAsync(this.Data); } public ObservableCollection<int> Data { get { return (this._data); } set { base.SetProperty(ref this._data, value); } } public ICommand GetMoreDataCommand { get { return (this._getMoreDataCommand.Value); } } void OnGetMoreData() { var start = this.Data.Count; for (int i = start; i < start + 10; i++) { this.Data.Add(i); } } Lazy<ICommand> _getMoreDataCommand; ObservableCollection<int> _data; }
and it uses the same IDataService implementation as the main page and it accesses the same data (albeit in a read/write mode) that the main page relies on.
The Problem
When I run this code and navigate from the main page to the sub page, add some data and then navigate back to the main page I see;
I’m glad that I see this because it’s exactly the scenario that I was trying to recreate as it occurred in a real app where it was initially harder to figure out.
As you might expect, this exception is thrown from my data service code as it tries to open the file to read the data for the main page as I navigate to it from the sub-page;
public async Task<IEnumerable<int>> LoadDataAsync() { List<int> list = new List<int>(); try { var file = await ApplicationData.Current.LocalFolder.GetFileAsync( fileName); using (var stream = await file.OpenStreamForReadAsync()) { list = (List<int>)_serializer.Value.ReadObject(stream); } } catch (FileNotFoundException) { } return (list); }
(i.e. on line 10 of that code above)
and in my real app I initially spent quite a bit of time trying to figure out whether I had accidentally left the file open after I had written to it and trying various techniques to make sure that I hadn’t and so on.
As you’ve possibly already spotted, this really isn’t likely to be the problem. The problem is more in the sequence of events which causes a race condition in that I have;
- Sub page creates new data.
- Navigation begins to move away from the sub page back to the main page.
- Navigation causes my OnTerminate() code in the sub page to begin to run.
- The OnTerminate() code begins to asynchronously save the data.
- Navigation is now causing the main page to start to load up.
- The main page goes to try and open the file to load the data.
- There is a risk that the file is locked from read access if Step(4) has not completed.
This is one of those scenarios where adding in a debugger affects the race condition and you can chase your tail a little bit but the addition of a simple boolean flag added below around lines 35 to 40 and then around lines 51-54 helps;
class DataService : IDataService { private DataService() { } static DataService() { _instance = new Lazy<IDataService>( () => { return (new DataService()); } ); _serializer = new Lazy<DataContractJsonSerializer>( () => { return (new DataContractJsonSerializer(typeof(List<int>))); } ); } public static IDataService Instance { get { return (_instance.Value); } } public async Task SaveDataAsync(IEnumerable<int> data) { var file = await ApplicationData.Current.LocalFolder.CreateFileAsync( fileName, CreationCollisionOption.ReplaceExisting); List<int> list = new List<int>(data); this._debugFlag = true; using (var stream = await file.OpenStreamForWriteAsync()) { _serializer.Value.WriteObject(stream, list); } this._debugFlag = false; } public async Task<IEnumerable<int>> LoadDataAsync() { List<int> list = new List<int>(); try { var file = await ApplicationData.Current.LocalFolder.GetFileAsync( fileName); if (this._debugFlag) { Debug.WriteLine("Houston, we have a problem"); } using (var stream = await file.OpenStreamForReadAsync()) { list = (List<int>)_serializer.Value.ReadObject(stream); } } catch (FileNotFoundException) { } return (list); } volatile bool _debugFlag; static Lazy<IDataService> _instance; static Lazy<DataContractJsonSerializer> _serializer; static readonly string fileName = "data.txt"; }
and what I see in my output window in Visual Studio is;
Fixing the Problem
Now, you’re possibly thinking that this race condition was easy to spot all along but it was interesting to me that I made an assumption about the file being left open and, largely, that was because I’m still a bit new to the WinRT way of dealing with files.
Also, I was checking my async C# 5.0 code and thinking “Ok, I seem to have all my awaits in the right place” and getting bogged down in where I thought my bug was.
However, leaving the file open at the lower level wasn’t really my problem. My problem was that my app was still running code relating to navigating away from the sub page (where the file writes are being async’d) while trying to run code relating to navigating towards my main page which wants to read the file at the same time and there’s no real way to defer the navigation although I could do some work to combine the interface that I built ( INavigateToViewModel ) with navigation such that the navigation didn’t happen until the view model had completed any async work that it needs to do as the user moves away from the page.
In this instance, that’s not the route I tried to take. I wanted to keep it pretty simple while avoiding having to change the calling code which still looks like this;
IDataService dataService = DataService.Instance; this.Data = new ObservableCollection<int>( await dataService.LoadDataAsync());
Now, the things that kind of ‘help me out’ here are that I’m not trying to support multiple concurrent read/writes via my data service and nor am I trying to write a thread-safe service because all of this code is ultimately invoked from the UI thread ( albeit asynchronously ) so I can probably get away with having a fairly simple flag that wraps around my use of the file. Given that I want to be able to continue to await I can perhaps use a TaskCompletionSource as my flag as in this modified DataService;
class DataService : IDataService { class SimpleAwaitable { public async Task ClaimAsync() { if (this._completionSource != null) { await this._completionSource.Task; } this._completionSource = new TaskCompletionSource<bool>(); } public void Release() { if (this._completionSource != null) { this._completionSource.SetResult(true); } this._completionSource = null; } TaskCompletionSource<bool> _completionSource; } private DataService() { this._awaitable = new SimpleAwaitable(); } static DataService() { _instance = new Lazy<IDataService>( () => { return (new DataService()); } ); _serializer = new Lazy<DataContractJsonSerializer>( () => { return (new DataContractJsonSerializer(typeof(List<int>))); } ); } public static IDataService Instance { get { return (_instance.Value); } } public async Task SaveDataAsync(IEnumerable<int> data) { await this._awaitable.ClaimAsync(); var file = await ApplicationData.Current.LocalFolder.CreateFileAsync( fileName, CreationCollisionOption.ReplaceExisting); List<int> list = new List<int>(data); using (var stream = await file.OpenStreamForWriteAsync()) { _serializer.Value.WriteObject(stream, list); } this._awaitable.Release(); } public async Task<IEnumerable<int>> LoadDataAsync() { await this._awaitable.ClaimAsync(); List<int> list = new List<int>(); try { var file = await ApplicationData.Current.LocalFolder.GetFileAsync( fileName); using (var stream = await file.OpenStreamForReadAsync()) { list = (List<int>)_serializer.Value.ReadObject(stream); } } catch (FileNotFoundException) { } this._awaitable.Release(); return (list); } SimpleAwaitable _awaitable; static Lazy<IDataService> _instance; static Lazy<DataContractJsonSerializer> _serializer; static readonly string fileName = "data.txt"; }
and so while the code is reading/writing to the file it makes sure that it has a form of mutual exclusion ( on the single UI thread ) by creating a TaskCompletionSource<bool> (the bool is arbitrary here) and if there is already one of those in existence then assuming that the ‘flag’ is owned by another piece of code and so using await to wait for the associated Task to finish.
You might need something more elaborate but in my specific case, that seems to offer enough protection to the shared file to make sure that my code doesn’t try to read it while my other code is still writing it and I stop hitting access violations over the file.