From 9ff8246a9dcbad184783ef987c6e9bf8ff4c0f04 Mon Sep 17 00:00:00 2001 From: ryanbodrug-microsoft <56318517+ryanbodrug-microsoft@users.noreply.github.com> Date: Wed, 8 Jul 2020 15:12:36 -0700 Subject: [PATCH] Fixing potential race condition in ListRepository. Now internally implemented as a concurrent dictionary. --- .../Storage/ListRepositoryTests.cs | 98 +++++++++++++++++-- .../Storage/PackageRepository.cs | 11 ++- .../Storage/ListRepository.cs | 32 ++++-- 3 files changed, 121 insertions(+), 20 deletions(-) diff --git a/src/modules/launcher/Plugins/Microsoft.Plugin.Program.UnitTests/Storage/ListRepositoryTests.cs b/src/modules/launcher/Plugins/Microsoft.Plugin.Program.UnitTests/Storage/ListRepositoryTests.cs index e4e7e4ab3c..fd1953419e 100644 --- a/src/modules/launcher/Plugins/Microsoft.Plugin.Program.UnitTests/Storage/ListRepositoryTests.cs +++ b/src/modules/launcher/Plugins/Microsoft.Plugin.Program.UnitTests/Storage/ListRepositoryTests.cs @@ -3,7 +3,10 @@ using Moq; 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 @@ -17,8 +20,7 @@ namespace Microsoft.Plugin.Program.UnitTests.Storage { //Arrange var itemName = "originalItem1"; - var mockStorage = new Mock>>(); - IRepository repository = new ListRepository(mockStorage.Object) { itemName }; + IRepository repository = new ListRepository() { itemName }; //Act var result = repository.Contains(itemName); @@ -31,8 +33,7 @@ namespace Microsoft.Plugin.Program.UnitTests.Storage public void Contains_ShouldReturnTrue_WhenListIsUpdatedWithAdd() { //Arrange - var mockStorage = new Mock>>(); - IRepository repository = new ListRepository(mockStorage.Object); + IRepository repository = new ListRepository(); //Act var itemName = "newItem"; @@ -48,8 +49,7 @@ namespace Microsoft.Plugin.Program.UnitTests.Storage { //Arrange var itemName = "originalItem1"; - var mockStorage = new Mock>>(); - IRepository repository = new ListRepository(mockStorage.Object) { itemName }; + IRepository repository = new ListRepository() { itemName }; //Act repository.Remove(itemName); @@ -58,5 +58,91 @@ namespace Microsoft.Plugin.Program.UnitTests.Storage //Assert Assert.IsFalse(result); } + + [Test] + public async Task Add_ShouldNotThrow_WhenBeingIterated() + { + //Arrange + ListRepository repository = new ListRepository(); + var numItems = 1000; + for(var i=0; i + { + 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 repository = new ListRepository(); + 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 }); + }); + } } } diff --git a/src/modules/launcher/Plugins/Microsoft.Plugin.Program/Storage/PackageRepository.cs b/src/modules/launcher/Plugins/Microsoft.Plugin.Program/Storage/PackageRepository.cs index ce52c83964..2218daded0 100644 --- a/src/modules/launcher/Plugins/Microsoft.Plugin.Program/Storage/PackageRepository.cs +++ b/src/modules/launcher/Plugins/Microsoft.Plugin.Program/Storage/PackageRepository.cs @@ -16,9 +16,12 @@ namespace Microsoft.Plugin.Program.Storage /// internal class PackageRepository : ListRepository, IRepository, IProgramRepository { - IPackageCatalog _packageCatalog; - public PackageRepository(IPackageCatalog packageCatalog, IStorage> storage) : base(storage) + private IStorage> _storage; + + private IPackageCatalog _packageCatalog; + public PackageRepository(IPackageCatalog packageCatalog, IStorage> 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; @@ -55,7 +58,7 @@ namespace Microsoft.Plugin.Program.Storage { //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); @@ -74,7 +77,7 @@ namespace Microsoft.Plugin.Program.Storage public void Save() { - _storage.Save(_items); + _storage.Save(Items); } public void Load() diff --git a/src/modules/launcher/Wox.Infrastructure/Storage/ListRepository.cs b/src/modules/launcher/Wox.Infrastructure/Storage/ListRepository.cs index 93400b5acb..bcd820b778 100644 --- a/src/modules/launcher/Wox.Infrastructure/Storage/ListRepository.cs +++ b/src/modules/launcher/Wox.Infrastructure/Storage/ListRepository.cs @@ -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 { @@ -16,18 +19,19 @@ namespace Wox.Infrastructure.Storage /// public class ListRepository : IRepository, IEnumerable { - protected IList _items = new List(); - protected IStorage> _storage; + public IList Items { get { return _items.Values.ToList(); } } - public ListRepository(IStorage> storage) + private ConcurrentDictionary _items = new ConcurrentDictionary(); + + public ListRepository() { - _storage = storage ?? throw new ArgumentNullException("storage", "StorageRepository requires an initialized storage interface"); + } public void Set(IList items) { //enforce that internal representation - _items = items.ToList(); + _items = new ConcurrentDictionary(items.ToDictionary( i => i.GetHashCode())); } public bool Any() @@ -37,27 +41,35 @@ namespace Wox.Infrastructure.Storage 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 AsParallel() { - return _items.AsParallel(); + return _items.Values.AsParallel(); } public bool Contains(T item) { - return _items.Contains(item); + return _items.ContainsKey(item.GetHashCode()); } public IEnumerator GetEnumerator() { - return _items.GetEnumerator(); + return _items.Values.GetEnumerator(); } IEnumerator IEnumerable.GetEnumerator()