From e470482695a9fe5d5211f91f97a43d22f11c4966 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 08:27:49 +0000 Subject: [PATCH 1/2] Return the stored profile from concurrent CreateProfile calls [patch] CreateProfile looked the id up, then created a Profile and ignored whether TryAdd stored it. Two callers racing on the same id could each return their own instance while only one was in the map, so bindings set on the other were invisible to GetProfile and never saved. Use ConcurrentDictionary.GetOrAdd so every caller gets the stored instance. Fixes ktsu-dev/Keybinding#112 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01P6PRqvoi3pKgXQu5F8XJb1 --- .../ProfileCreateConcurrencyTests.cs | 47 +++++++++++++++++++ Keybinding/Services/ProfileManager.cs | 14 ++---- 2 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 Keybinding.Test/ProfileCreateConcurrencyTests.cs diff --git a/Keybinding.Test/ProfileCreateConcurrencyTests.cs b/Keybinding.Test/ProfileCreateConcurrencyTests.cs new file mode 100644 index 0000000..472eaed --- /dev/null +++ b/Keybinding.Test/ProfileCreateConcurrencyTests.cs @@ -0,0 +1,47 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +[TestClass] +public class ProfileCreateConcurrencyTests +{ + private const int ThreadCount = 8; + private const int Trials = 500; + + [TestMethod] + public void CreateProfile_CalledConcurrentlyForOneId_ReturnsTheStoredProfileToEveryCaller() + { + for (int trial = 0; trial < Trials; trial++) + { + ProfileManager profiles = new(); + Profile[] returned = new Profile[ThreadCount]; + using Barrier barrier = new(ThreadCount); + + Thread[] threads = [.. Enumerable.Range(0, ThreadCount).Select(i => new Thread(() => + { + barrier.SignalAndWait(); + returned[i] = profiles.CreateProfile("p", "P"); + }))]; + + foreach (Thread thread in threads) + { + thread.Start(); + } + + foreach (Thread thread in threads) + { + thread.Join(); + } + + Profile? stored = profiles.GetProfile("p"); + Assert.IsNotNull(stored); + for (int i = 0; i < ThreadCount; i++) + { + Assert.AreSame(stored, returned[i], $"Trial {trial}: caller {i} got a Profile that was never stored, so its bindings would be lost."); + } + } + } +} diff --git a/Keybinding/Services/ProfileManager.cs b/Keybinding/Services/ProfileManager.cs index 80206e2..55e0afb 100644 --- a/Keybinding/Services/ProfileManager.cs +++ b/Keybinding/Services/ProfileManager.cs @@ -42,16 +42,10 @@ public Profile CreateProfile(string id, string name, string? description = null) string normalizedId = id.Trim(); - // Return existing profile if it already exists - if (_profiles.TryGetValue(normalizedId, out Profile? existingProfile)) - { - return existingProfile; - } - - // Create new profile - Profile newProfile = new(normalizedId, name.Trim(), description); - _profiles.TryAdd(normalizedId, newProfile); - return newProfile; + // Return the existing profile, or store a new one, in one atomic step: a separate lookup + // and add let two concurrent callers each return their own Profile while only one of + // them was stored (ktsu-dev/Keybinding#112) + return _profiles.GetOrAdd(normalizedId, _ => new Profile(normalizedId, name.Trim(), description)); } /// From d13bcc613fb1f746befaaed819e92c1b0de82653 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 08:34:43 +0000 Subject: [PATCH 2/2] Address SonarCloud findings on the CreateProfile fix [patch] Use GetOrAdd's key parameter rather than capturing normalizedId (S6612), and pass the test's cancellation token to Barrier.SignalAndWait (MSTEST0049). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01P6PRqvoi3pKgXQu5F8XJb1 --- Keybinding.Test/ProfileCreateConcurrencyTests.cs | 4 +++- Keybinding/Services/ProfileManager.cs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Keybinding.Test/ProfileCreateConcurrencyTests.cs b/Keybinding.Test/ProfileCreateConcurrencyTests.cs index 472eaed..00a2ee3 100644 --- a/Keybinding.Test/ProfileCreateConcurrencyTests.cs +++ b/Keybinding.Test/ProfileCreateConcurrencyTests.cs @@ -11,6 +11,8 @@ public class ProfileCreateConcurrencyTests private const int ThreadCount = 8; private const int Trials = 500; + public TestContext TestContext { get; set; } = null!; + [TestMethod] public void CreateProfile_CalledConcurrentlyForOneId_ReturnsTheStoredProfileToEveryCaller() { @@ -22,7 +24,7 @@ public void CreateProfile_CalledConcurrentlyForOneId_ReturnsTheStoredProfileToEv Thread[] threads = [.. Enumerable.Range(0, ThreadCount).Select(i => new Thread(() => { - barrier.SignalAndWait(); + barrier.SignalAndWait(TestContext.CancellationToken); returned[i] = profiles.CreateProfile("p", "P"); }))]; diff --git a/Keybinding/Services/ProfileManager.cs b/Keybinding/Services/ProfileManager.cs index 55e0afb..7d46fb1 100644 --- a/Keybinding/Services/ProfileManager.cs +++ b/Keybinding/Services/ProfileManager.cs @@ -45,7 +45,7 @@ public Profile CreateProfile(string id, string name, string? description = null) // Return the existing profile, or store a new one, in one atomic step: a separate lookup // and add let two concurrent callers each return their own Profile while only one of // them was stored (ktsu-dev/Keybinding#112) - return _profiles.GetOrAdd(normalizedId, _ => new Profile(normalizedId, name.Trim(), description)); + return _profiles.GetOrAdd(normalizedId, key => new Profile(key, name.Trim(), description)); } ///