Skip to content

Commit

Permalink
Fixing potential race condition in ListRepository. Now internally imp…
Browse files Browse the repository at this point in the history 
…lemented as a concurrent dictionary.
  • Loading branch information
@ryanbodrug-microsoft
ryanbodrug-microsoft committed Jul 9, 2020
1 parent 2c45956 commit 9ff8246
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 20 deletions.
98 changes: 92 additions & 6 deletions ...odules/launcher/Plugins/Microsoft.Plugin.Program.UnitTests/Storage/ListRepositoryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
using NUnit.Framework;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Windows.Media.Capture;
using Wox.Infrastructure.Storage;

namespace Microsoft.Plugin.Program.UnitTests.Storage
Expand All @@ -17,8 +20,7 @@ public void Contains_ShouldReturnTrue_WhenListIsInitializedWithItem()
{
//Arrange
var itemName = "originalItem1";
var mockStorage = new Mock<IStorage<IList<string>>>();
IRepository<string> repository = new ListRepository<string>(mockStorage.Object) { itemName };
IRepository<string> repository = new ListRepository<string>() { itemName };

//Act
var result = repository.Contains(itemName);
Expand All @@ -31,8 +33,7 @@ public void Contains_ShouldReturnTrue_WhenListIsInitializedWithItem()
public void Contains_ShouldReturnTrue_WhenListIsUpdatedWithAdd()
{
//Arrange
var mockStorage = new Mock<IStorage<IList<string>>>();
IRepository<string> repository = new ListRepository<string>(mockStorage.Object);
IRepository<string> repository = new ListRepository<string>();

//Act
var itemName = "newItem";
Expand All @@ -48,8 +49,7 @@ public void Contains_ShouldReturnFalse_WhenListIsUpdatedWithRemove()
{
//Arrange
var itemName = "originalItem1";
var mockStorage = new Mock<IStorage<IList<string>>>();
IRepository<string> repository = new ListRepository<string>(mockStorage.Object) { itemName };
IRepository<string> repository = new ListRepository<string>() { itemName };

//Act
repository.Remove(itemName);
Expand All @@ -58,5 +58,91 @@ public void Contains_ShouldReturnFalse_WhenListIsUpdatedWithRemove()
//Assert
Assert.IsFalse(result);
}

[Test]
public async Task Add_ShouldNotThrow_WhenBeingIterated()
{
//Arrange
ListRepository<string> repository = new ListRepository<string>();
var numItems = 1000;
for(var i=0; i<numItems;++i)
{
repository.Add($"OriginalItem_{i}");
}

//Act - Begin iterating on one thread
var iterationTask = Task.Run(() =>
{
var remainingIterations = 10000;
while (remainingIterations > 0)
{
foreach (var item in repository)
{
//keep iterating
}
--remainingIterations;
}
});

//Act - Insert on another thread
var addTask = Task.Run(() =>
{
for (var i = 0; i < numItems; ++i)
{
repository.Add($"NewItem_{i}");
}
});

//Assert that this does not throw. Collections that aren't syncronized will throw an invalidoperatioexception if the list is modified while enumerating
Assert.DoesNotThrowAsync(async () =>
{
await Task.WhenAll(new Task[] { iterationTask, addTask });
});
}

[Test]
public async Task Remove_ShouldNotThrow_WhenBeingIterated()
{
//Arrange
ListRepository<string> repository = new ListRepository<string>();
var numItems = 1000;
for (var i = 0; i < numItems; ++i)
{
repository.Add($"OriginalItem_{i}");
}

//Act - Begin iterating on one thread
var iterationTask = Task.Run(() =>
{
var remainingIterations = 10000;
while (remainingIterations > 0)
{
foreach (var item in repository)
{
//keep iterating
}
--remainingIterations;
}
});

//Act - Remove on another thread
var addTask = Task.Run(() =>
{
for (var i = 0; i < numItems; ++i)
{
repository.Remove($"OriginalItem_{i}");
}
});

//Assert that this does not throw. Collections that aren't syncronized will throw an invalidoperatioexception if the list is modified while enumerating
Assert.DoesNotThrowAsync(async () =>
{
await Task.WhenAll(new Task[] { iterationTask, addTask });
});
}
}
}
11 changes: 7 additions & 4 deletions src/modules/launcher/Plugins/Microsoft.Plugin.Program/Storage/PackageRepository.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@ namespace Microsoft.Plugin.Program.Storage
/// </summary>
internal class PackageRepository : ListRepository<UWP.Application>, IRepository<UWP.Application>, IProgramRepository
{
IPackageCatalog _packageCatalog;
public PackageRepository(IPackageCatalog packageCatalog, IStorage<IList<UWP.Application>> storage) : base(storage)
private IStorage<IList<UWP.Application>> _storage;

private IPackageCatalog _packageCatalog;
public PackageRepository(IPackageCatalog packageCatalog, IStorage<IList<UWP.Application>> storage)
{
_storage = storage ?? throw new ArgumentNullException("storage", "StorageRepository requires an initialized storage interface");
_packageCatalog = packageCatalog ?? throw new ArgumentNullException("packageCatalog", "PackageRepository expects an interface to be able to subscribe to package events");
_packageCatalog.PackageInstalling += OnPackageInstalling;
_packageCatalog.PackageUninstalling += OnPackageUninstalling;
Expand Down Expand Up @@ -55,7 +58,7 @@ public void OnPackageUninstalling(PackageCatalog p, PackageUninstallingEventArgs
{
//find apps associated with this package.
var uwp = new UWP(args.Package);
var apps = _items.Where(a => a.Package.Equals(uwp)).ToArray();
var apps = Items.Where(a => a.Package.Equals(uwp)).ToArray();
foreach (var app in apps)
{
Remove(app);
Expand All @@ -74,7 +77,7 @@ public void IndexPrograms()

public void Save()
{
_storage.Save(_items);
_storage.Save(Items);
}

public void Load()
Expand Down
32 changes: 22 additions & 10 deletions src/modules/launcher/Wox.Infrastructure/Storage/ListRepository.cs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
using NLog.Filters;
using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Controls.Primitives;
using Wox.Infrastructure;
using Wox.Infrastructure.Logger;

namespace Wox.Infrastructure.Storage
{
Expand All @@ -16,18 +19,19 @@ namespace Wox.Infrastructure.Storage
/// <typeparam name="T"></typeparam>
public class ListRepository<T> : IRepository<T>, IEnumerable<T>
{
protected IList<T> _items = new List<T>();
protected IStorage<IList<T>> _storage;
public IList<T> Items { get { return _items.Values.ToList(); } }

public ListRepository(IStorage<IList<T>> storage)
private ConcurrentDictionary<int, T> _items = new ConcurrentDictionary<int, T>();

public ListRepository()
{
_storage = storage ?? throw new ArgumentNullException("storage", "StorageRepository requires an initialized storage interface");

}

public void Set(IList<T> items)
{
//enforce that internal representation
_items = items.ToList<T>();
_items = new ConcurrentDictionary<int, T>(items.ToDictionary( i => i.GetHashCode()));
}

public bool Any()
Expand All @@ -37,27 +41,35 @@ public bool Any()

public void Add(T insertedItem)
{
_items.Add(insertedItem);
if (!_items.TryAdd(insertedItem.GetHashCode(), insertedItem))
{
Log.Error($"|ListRepository.Add| Item Already Exists <{insertedItem}>");
}

}

public void Remove(T removedItem)
{
_items.Remove(removedItem);

if (!_items.TryRemove(removedItem.GetHashCode(), out _))
{
Log.Error($"|ListRepository.Remove| Item Not Found <{removedItem}>");
}
}

public ParallelQuery<T> AsParallel()
{
return _items.AsParallel();
return _items.Values.AsParallel();
}

public bool Contains(T item)
{
return _items.Contains(item);
return _items.ContainsKey(item.GetHashCode());
}

public IEnumerator<T> GetEnumerator()
{
return _items.GetEnumerator();
return _items.Values.GetEnumerator();
}

IEnumerator IEnumerable.GetEnumerator()
Expand Down

0 comments on commit 9ff8246

Please sign in to comment.