diff --git a/Keybinding.Test/ProfileActivateDeleteConcurrencyTests.cs b/Keybinding.Test/ProfileActivateDeleteConcurrencyTests.cs new file mode 100644 index 0000000..f8f276f --- /dev/null +++ b/Keybinding.Test/ProfileActivateDeleteConcurrencyTests.cs @@ -0,0 +1,55 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core.Services; + +[TestClass] +public class ProfileActivateDeleteConcurrencyTests +{ + private const int Trials = 10000; + + public TestContext TestContext { get; set; } = null!; + + [TestMethod] + public void SetActiveProfile_RacingDeleteProfile_NeverLeavesTheDeletedIdActive() + { + int stale = 0; + for (int trial = 0; trial < Trials; trial++) + { + ProfileManager profiles = new(); + profiles.CreateProfile("x", "X"); + using Barrier barrier = new(2); + + Thread activate = new(() => + { + barrier.SignalAndWait(TestContext.CancellationToken); + + // Keep activating until the delete lands, so the activation is in flight when it does + while (profiles.SetActiveProfile("x")) + { + // Intentionally empty: each iteration is another activation racing the delete + } + }); + Thread delete = new(() => + { + barrier.SignalAndWait(TestContext.CancellationToken); + profiles.DeleteProfile("x"); + }); + + activate.Start(); + delete.Start(); + activate.Join(); + delete.Join(); + + // A profile recreated with the deleted id must not become active on its own + profiles.CreateProfile("x", "Recreated"); + if (profiles.GetActiveProfile() is not null) + { + stale++; + } + } + + Assert.AreEqual(0, stale, $"{stale} of {Trials} trials left the active id pointing at the deleted profile."); + } +} diff --git a/Keybinding/Services/ProfileManager.cs b/Keybinding/Services/ProfileManager.cs index 923182d..d9df93a 100644 --- a/Keybinding/Services/ProfileManager.cs +++ b/Keybinding/Services/ProfileManager.cs @@ -57,15 +57,22 @@ public bool DeleteProfile(string profileId) } string normalizedId = profileId.Trim(); - bool removed = _profiles.TryRemove(normalizedId, out _); - // Clear active profile if it was the one being deleted - if (removed && _activeProfileId == normalizedId) + // Remove and clear under the lock SetActiveProfile checks and sets under, so an activation + // cannot land between them and leave the active id naming the deleted profile + // (ktsu-dev/Keybinding#120) + lock (_lock) { - _activeProfileId = null; - } + bool removed = _profiles.TryRemove(normalizedId, out _); - return removed; + // Clear active profile if it was the one being deleted + if (removed && _activeProfileId == normalizedId) + { + _activeProfileId = null; + } + + return removed; + } } /// @@ -102,13 +109,16 @@ public bool SetActiveProfile(string profileId) string normalizedId = profileId.Trim(); - if (!_profiles.ContainsKey(normalizedId)) + lock (_lock) { - return false; - } + if (!_profiles.ContainsKey(normalizedId)) + { + return false; + } - _activeProfileId = normalizedId; - return true; + _activeProfileId = normalizedId; + return true; + } } ///