From 101df501c5c7dbee407b3c6b14434847c5540bfe Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 15:03:09 -0400 Subject: [PATCH 1/8] refactor: ensure thread safety for _collectionsByLocal; extracted add collection logic; removed duplicate calls Adding / removing collections do not require framework thread access. It is only when assigning via AssignTemporaryCollection(), it needs to access `(ObjectManager)objects[actorIndex]` when framework thread is needed. --- .../Collections/Manager/CollectionStorage.cs | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index 531b6333..f336f48d 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -33,8 +33,7 @@ public ModCollection Create(string name, int index, ModCollection? duplicate) { var newCollection = duplicate?.Duplicate(name, CurrentCollectionId, index) ?? ModCollection.CreateEmpty(name, CurrentCollectionId, index, _modStorage.Count); - _collectionsByLocal[CurrentCollectionId] = newCollection; - CurrentCollectionId += 1; + Add(newCollection); return newCollection; } @@ -43,21 +42,29 @@ public ModCollection CreateFromData(Guid id, string name, int version, Dictionar { var newCollection = ModCollection.CreateFromData(_saveService, _modStorage, new ModCollectionIdentity(id, CurrentCollectionId, name, Count), version, allSettings, inheritances); - _collectionsByLocal[CurrentCollectionId] = newCollection; - CurrentCollectionId += 1; + Add(newCollection); return newCollection; } public ModCollection CreateTemporary(string name, int index, int globalChangeCounter) { var newCollection = ModCollection.CreateTemporary(name, CurrentCollectionId, index, globalChangeCounter); - _collectionsByLocal[CurrentCollectionId] = newCollection; - CurrentCollectionId += 1; + Add(newCollection); return newCollection; } + /// Atomically add to _collectionLocal and increments _currentCollectionIdValue. + private void Add(ModCollection newCollection) + { + _collectionsByLocal.AddOrUpdate(CurrentCollectionId, + static (_, newColl) => newColl, + static (_, _, newColl) => newColl, + newCollection); + Interlocked.Increment(ref _currentCollectionIdValue); + } + public void Delete(ModCollection collection) - => _collectionsByLocal.Remove(collection.Identity.LocalId); + => _collectionsByLocal.TryRemove(collection.Identity.LocalId, out _); /// The empty collection is always available at Index 0. private readonly List _collections = @@ -66,7 +73,7 @@ public void Delete(ModCollection collection) ]; /// A list of all collections ever created still existing by their local id. - private readonly Dictionary + private readonly ConcurrentDictionary _collectionsByLocal = new() { [LocalCollectionId.Zero] = ModCollection.Empty }; @@ -186,7 +193,6 @@ public bool RemoveCollection(ModCollection collection) // Update indices. for (var i = collection.Identity.Index; i < Count; ++i) _collections[i].Identity.Index = i; - _collectionsByLocal.Remove(collection.Identity.LocalId); Penumbra.Messager.NotificationMessage($"Deleted collection {collection.Identity.AnonymizedName}.", NotificationType.Success, false); _communicator.CollectionChange.Invoke(CollectionType.Inactive, collection, null, string.Empty); From 723eaa0076ec8ab1c42ccfc9dc714531c6b40045 Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 15:45:26 -0400 Subject: [PATCH 2/8] refactor: made _collections thread-safe --- .../Collections/Manager/CollectionStorage.cs | 61 +++++++++++++------ 1 file changed, 44 insertions(+), 17 deletions(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index f336f48d..f8c42dea 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -71,6 +71,8 @@ public void Delete(ModCollection collection) [ ModCollection.Empty, ]; + + private readonly Lock _collectionsLock = new(); /// A list of all collections ever created still existing by their local id. private readonly ConcurrentDictionary @@ -79,8 +81,11 @@ private readonly ConcurrentDictionary public readonly ModCollection DefaultNamed; - /// Incremented by 1 because the empty collection gets Zero. - public LocalCollectionId CurrentCollectionId { get; private set; } = LocalCollectionId.Zero + 1; + /// Starts at 1 because the empty collection gets Zero. + private int _currentCollectionIdValue = 1; + + /// Starts at 1 because the empty collection gets Zero. + public LocalCollectionId CurrentCollectionId => new(_currentCollectionIdValue); /// Default enumeration skips the empty collection. public IEnumerator GetEnumerator() @@ -162,8 +167,12 @@ public bool AddCollection(string name, ModCollection? duplicate) if (name.Length == 0) return false; - var newCollection = Create(name, _collections.Count, duplicate); - _collections.Add(newCollection); + ModCollection newCollection; + lock (_collectionsLock) + { + newCollection = Create(name, _collections.Count, duplicate); + _collections.Add(newCollection); + } _saveService.ImmediateSave(new ModCollectionSave(_modStorage, newCollection)); Penumbra.Messager.NotificationMessage($"Created new collection {newCollection.Identity.AnonymizedName}.", NotificationType.Success, false); _communicator.CollectionChange.Invoke(CollectionType.Inactive, null, newCollection, string.Empty); @@ -189,10 +198,13 @@ public bool RemoveCollection(ModCollection collection) Delete(collection); _saveService.ImmediateDelete(new ModCollectionSave(_modStorage, collection)); - _collections.RemoveAt(collection.Identity.Index); - // Update indices. - for (var i = collection.Identity.Index; i < Count; ++i) - _collections[i].Identity.Index = i; + lock (_collectionsLock) + { + _collections.RemoveAt(collection.Identity.Index); + // Update indices. + for (var i = collection.Identity.Index; i < Count; ++i) + _collections[i].Identity.Index = i; + } Penumbra.Messager.NotificationMessage($"Deleted collection {collection.Identity.AnonymizedName}.", NotificationType.Success, false); _communicator.CollectionChange.Invoke(CollectionType.Inactive, collection, null, string.Empty); @@ -316,15 +328,17 @@ private ModCollection SetDefaultNamedCollection() /// Move all settings in all collections to unused settings. private void OnModDiscoveryStarted() { - foreach (var collection in this) + var snapshot = GetModSnapShot(); + foreach (var collection in snapshot) collection.Settings.PrepareModDiscovery(_modStorage); } /// Restore all settings in all collections to mods. private void OnModDiscoveryFinished() { + var snapshot = GetModSnapShot(); // Re-apply all mod settings. - foreach (var collection in this) + foreach (var collection in snapshot) collection.Settings.ApplyModSettings(collection, _saveService, _modStorage); } @@ -332,22 +346,23 @@ private void OnModDiscoveryFinished() private void OnModPathChange(ModPathChangeType type, Mod mod, DirectoryInfo? oldDirectory, DirectoryInfo? newDirectory) { + var snapshot = GetModSnapShot(); switch (type) { case ModPathChangeType.Added: - foreach (var collection in this) + foreach (var collection in snapshot) collection.Settings.AddMod(mod); break; case ModPathChangeType.Deleted: - foreach (var collection in this) + foreach (var collection in snapshot) collection.Settings.RemoveMod(mod); break; case ModPathChangeType.Moved: - foreach (var collection in this.Where(collection => collection.GetOwnSettings(mod.Index) != null)) + foreach (var collection in snapshot.Where(collection => collection.GetOwnSettings(mod.Index) != null)) _saveService.QueueSave(new ModCollectionSave(_modStorage, collection)); break; case ModPathChangeType.Reloaded: - foreach (var collection in this) + foreach (var collection in snapshot) { if (collection.GetOwnSettings(mod.Index)?.Settings.FixAll(mod) ?? false) _saveService.QueueSave(new ModCollectionSave(_modStorage, collection)); @@ -365,8 +380,9 @@ private void OnModOptionChange(ModOptionChangeType type, Mod mod, IModGroup? gro type.HandlingInfo(out var requiresSaving, out _, out _); if (!requiresSaving) return; - - foreach (var collection in this) + + var snapshot = GetModSnapShot(); + foreach (var collection in snapshot) { if (collection.GetOwnSettings(mod.Index)?.HandleChanges(type, mod, group, option, movedToIdx) ?? false) _saveService.QueueSave(new ModCollectionSave(_modStorage, collection)); @@ -380,11 +396,22 @@ private void OnModFileChanged(Mod mod, FileRegistry file) if (file.CurrentUsage == 0) return; - foreach (var collection in this) + var snapshot = GetModSnapShot(); + foreach (var collection in snapshot) { var (settings, _) = collection.GetActualSettings(mod.Index); if (settings is { Enabled: true }) collection.Counters.IncrementChange(); } } + + private ModCollection[] GetModSnapShot() + { + ModCollection[] snapshot; + lock (_collectionsLock) + { + snapshot = _collections.Skip(1).ToArray(); + } + return snapshot; + } } From d4b6f611f854ea2e47bb2c65e706e8f7b3a03b7f Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 16:45:33 -0400 Subject: [PATCH 3/8] bugfix: fixed a race condition potentially causing last write wins --- .../Collections/Manager/CollectionStorage.cs | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index f8c42dea..68820706 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -31,36 +31,38 @@ public class CollectionStorage : IReadOnlyList, IDisposable, ISer public ModCollection Create(string name, int index, ModCollection? duplicate) { - var newCollection = duplicate?.Duplicate(name, CurrentCollectionId, index) - ?? ModCollection.CreateEmpty(name, CurrentCollectionId, index, _modStorage.Count); - Add(newCollection); + var localId = AllocateNextId(); + var newCollection = duplicate?.Duplicate(name, localId, index) + ?? ModCollection.CreateEmpty(name, localId, index, _modStorage.Count); + AddAtLocalId(newCollection, localId); return newCollection; } public ModCollection CreateFromData(Guid id, string name, int version, Dictionary allSettings, IReadOnlyList inheritances) { + var localId = AllocateNextId(); var newCollection = ModCollection.CreateFromData(_saveService, _modStorage, - new ModCollectionIdentity(id, CurrentCollectionId, name, Count), version, allSettings, inheritances); - Add(newCollection); + new ModCollectionIdentity(id, localId, name, Count), version, allSettings, inheritances); + AddAtLocalId(newCollection, localId); return newCollection; } public ModCollection CreateTemporary(string name, int index, int globalChangeCounter) { - var newCollection = ModCollection.CreateTemporary(name, CurrentCollectionId, index, globalChangeCounter); - Add(newCollection); + var localId = AllocateNextId(); + var newCollection = ModCollection.CreateTemporary(name, localId, index, globalChangeCounter); + AddAtLocalId(newCollection, localId); return newCollection; } - /// Atomically add to _collectionLocal and increments _currentCollectionIdValue. - private void Add(ModCollection newCollection) + /// Atomically add to _collectionLocal at the id given. + private void AddAtLocalId(ModCollection newCollection, LocalCollectionId id) { - _collectionsByLocal.AddOrUpdate(CurrentCollectionId, + _collectionsByLocal.AddOrUpdate(id, static (_, newColl) => newColl, static (_, _, newColl) => newColl, newCollection); - Interlocked.Increment(ref _currentCollectionIdValue); } public void Delete(ModCollection collection) @@ -86,6 +88,9 @@ private readonly ConcurrentDictionary /// Starts at 1 because the empty collection gets Zero. public LocalCollectionId CurrentCollectionId => new(_currentCollectionIdValue); + + private LocalCollectionId AllocateNextId () + => new(Interlocked.Increment(ref _currentCollectionIdValue)); /// Default enumeration skips the empty collection. public IEnumerator GetEnumerator() From 0bf9fdea84d272841f5162f16b497b98a2b41824 Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 17:12:00 -0400 Subject: [PATCH 4/8] bugfix: added locks on external methods --- .../Collections/Manager/CollectionStorage.cs | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index 68820706..0c583efa 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -94,23 +94,35 @@ private LocalCollectionId AllocateNextId () /// Default enumeration skips the empty collection. public IEnumerator GetEnumerator() - => _collections.Skip(1).GetEnumerator(); + => GetModSnapShot().ToList().GetEnumerator(); IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); public int Count - => _collections.Count; + { + get + { + lock(_collectionsLock) + return _collections.Count; + } + } public ModCollection this[int index] - => _collections[index]; + { + get + { + lock(_collectionsLock) + return _collections[index]; + } + } /// Find a collection by its name. If the name is empty or None, the empty collection is returned. public bool ByName(string name, [NotNullWhen(true)] out ModCollection? collection) { if (name.Length != 0) - return _collections.FindFirst(c => string.Equals(c.Identity.Name, name, StringComparison.OrdinalIgnoreCase), out collection); - + lock(_collectionsLock) + return _collections.FindFirst(c => string.Equals(c.Identity.Name, name, StringComparison.OrdinalIgnoreCase), out collection); collection = ModCollection.Empty; return true; } @@ -119,8 +131,8 @@ public bool ByName(string name, [NotNullWhen(true)] out ModCollection? collectio public bool ById(Guid id, [NotNullWhen(true)] out ModCollection? collection) { if (id != Guid.Empty) - return _collections.FindFirst(c => c.Identity.Id == id, out collection); - + lock(_collectionsLock) + return _collections.FindFirst(c => c.Identity.Id == id, out collection); collection = ModCollection.Empty; return true; } @@ -189,7 +201,7 @@ public bool AddCollection(string name, ModCollection? duplicate) /// public bool RemoveCollection(ModCollection collection) { - if (collection.Identity.Index <= ModCollection.Empty.Identity.Index || collection.Identity.Index >= _collections.Count) + if (collection.Identity.Index <= ModCollection.Empty.Identity.Index || collection.Identity.Index >= Count) { Penumbra.Messager.NotificationMessage("Can not remove the empty collection.", NotificationType.Error, false); return false; @@ -203,11 +215,11 @@ public bool RemoveCollection(ModCollection collection) Delete(collection); _saveService.ImmediateDelete(new ModCollectionSave(_modStorage, collection)); - lock (_collectionsLock) + lock (_collectionsLock) { _collections.RemoveAt(collection.Identity.Index); // Update indices. - for (var i = collection.Identity.Index; i < Count; ++i) + for (var i = collection.Identity.Index; i < _collections.Count; ++i) _collections[i].Identity.Index = i; } @@ -322,12 +334,12 @@ private ModCollection SetDefaultNamedCollection() return collection; if (AddCollection(ModCollectionIdentity.DefaultCollectionName, null)) - return _collections[^1]; + return this[^1]; Penumbra.Messager.NotificationMessage( $"Unknown problem creating a collection with the name {ModCollectionIdentity.DefaultCollectionName}, which is required to exist.", NotificationType.Error); - return Count > 1 ? _collections[1] : _collections[0]; + return Count > 1 ? this[1] : this[0]; } /// Move all settings in all collections to unused settings. From 1d7603bb8cc74e6eb3605b1d88f6424bff0834d6 Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 17:20:16 -0400 Subject: [PATCH 5/8] bugfix: moved validation inside lock --- .../Collections/Manager/CollectionStorage.cs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index 0c583efa..c26ce7f9 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -201,22 +201,23 @@ public bool AddCollection(string name, ModCollection? duplicate) /// public bool RemoveCollection(ModCollection collection) { - if (collection.Identity.Index <= ModCollection.Empty.Identity.Index || collection.Identity.Index >= Count) + lock (_collectionsLock) { - Penumbra.Messager.NotificationMessage("Can not remove the empty collection.", NotificationType.Error, false); - return false; - } + if (collection.Identity.Index <= ModCollection.Empty.Identity.Index || collection.Identity.Index >= Count) + { + Penumbra.Messager.NotificationMessage("Can not remove the empty collection.", NotificationType.Error, false); + return false; + } - if (collection.Identity.Index == DefaultNamed.Identity.Index) - { - Penumbra.Messager.NotificationMessage("Can not remove the default collection.", NotificationType.Error, false); - return false; - } + if (collection.Identity.Index == DefaultNamed.Identity.Index) + { + Penumbra.Messager.NotificationMessage("Can not remove the default collection.", NotificationType.Error, false); + return false; + } + + Delete(collection); + _saveService.ImmediateDelete(new ModCollectionSave(_modStorage, collection)); - Delete(collection); - _saveService.ImmediateDelete(new ModCollectionSave(_modStorage, collection)); - lock (_collectionsLock) - { _collections.RemoveAt(collection.Identity.Index); // Update indices. for (var i = collection.Identity.Index; i < _collections.Count; ++i) From cc95678d0520aa815827f9f2191fc5cf3e1ebed9 Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 17:22:45 -0400 Subject: [PATCH 6/8] bugfix: prevent skipping over 1 Maintainers expect _currentCollectionIdValue to start at 1, and I don't want to change that --- Penumbra/Collections/Manager/CollectionStorage.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index c26ce7f9..c20d72bb 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -89,8 +89,12 @@ private readonly ConcurrentDictionary /// Starts at 1 because the empty collection gets Zero. public LocalCollectionId CurrentCollectionId => new(_currentCollectionIdValue); - private LocalCollectionId AllocateNextId () - => new(Interlocked.Increment(ref _currentCollectionIdValue)); + private LocalCollectionId AllocateNextId () + { + var newLocalId = new LocalCollectionId(_currentCollectionIdValue); + Interlocked.Increment(ref _currentCollectionIdValue); + return newLocalId; + } /// Default enumeration skips the empty collection. public IEnumerator GetEnumerator() From 3f8199f1cdf331157f5e349b7bd561d7e3423b83 Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 17:23:56 -0400 Subject: [PATCH 7/8] misc: already in lock, change back to _collections.Count for readibility --- Penumbra/Collections/Manager/CollectionStorage.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index c20d72bb..1f49e4e1 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -207,7 +207,7 @@ public bool RemoveCollection(ModCollection collection) { lock (_collectionsLock) { - if (collection.Identity.Index <= ModCollection.Empty.Identity.Index || collection.Identity.Index >= Count) + if (collection.Identity.Index <= ModCollection.Empty.Identity.Index || collection.Identity.Index >= _collections.Count) { Penumbra.Messager.NotificationMessage("Can not remove the empty collection.", NotificationType.Error, false); return false; From c42f89277c2ced54f4cc81607edbabfd1dad6c42 Mon Sep 17 00:00:00 2001 From: mayo Date: Sat, 1 Nov 2025 17:26:04 -0400 Subject: [PATCH 8/8] lint sorry, haskell slipping through. --- Penumbra/Collections/Manager/CollectionStorage.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Penumbra/Collections/Manager/CollectionStorage.cs b/Penumbra/Collections/Manager/CollectionStorage.cs index 1f49e4e1..64341157 100644 --- a/Penumbra/Collections/Manager/CollectionStorage.cs +++ b/Penumbra/Collections/Manager/CollectionStorage.cs @@ -89,7 +89,7 @@ private readonly ConcurrentDictionary /// Starts at 1 because the empty collection gets Zero. public LocalCollectionId CurrentCollectionId => new(_currentCollectionIdValue); - private LocalCollectionId AllocateNextId () + private LocalCollectionId AllocateNextId() { var newLocalId = new LocalCollectionId(_currentCollectionIdValue); Interlocked.Increment(ref _currentCollectionIdValue);