From da0e2785249b3a57631da2e6afaeaec43689b5d8 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 28 Sep 2026 17:25:58 +0000 Subject: [PATCH 1/2] Lock a profile's chords so concurrent binds cannot corrupt them [minor] Profile kept its bindings in a plain Dictionary that SetChord, RemoveChord and ClearChords changed without synchronization while FindCommandByChord and SaveAsync enumerated it. Parallel binds lost bindings and threw. Every read and write of the bindings now takes a per-profile lock, and the services read them through snapshots. The public Chords dictionary is marked [Obsolete] because touching it bypasses the lock; ChordCount replaces its only internal use that wasn't a snapshot. Fixes #109 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015BJVibRvmTotkqEGp3695R --- .../ProfileChordConcurrencyTests.cs | 49 +++++++ Keybinding/KeybindingManager.cs | 2 +- Keybinding/Models/Profile.cs | 120 +++++++++++++++--- .../Services/JsonKeybindingRepository.cs | 4 +- Keybinding/Services/KeybindingService.cs | 8 +- Keybinding/Services/ProfileManager.cs | 2 +- 6 files changed, 157 insertions(+), 28 deletions(-) create mode 100644 Keybinding.Test/ProfileChordConcurrencyTests.cs diff --git a/Keybinding.Test/ProfileChordConcurrencyTests.cs b/Keybinding.Test/ProfileChordConcurrencyTests.cs new file mode 100644 index 0000000..3408b35 --- /dev/null +++ b/Keybinding.Test/ProfileChordConcurrencyTests.cs @@ -0,0 +1,49 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using System.Globalization; +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +[TestClass] +public class ProfileChordConcurrencyTests +{ + private const int CommandCount = 20_000; + + [TestMethod] + public void BindUnbindAndFind_CalledInParallel_KeepEveryBindingAndDoNotThrow() + { + CommandRegistry registry = new(); + ProfileManager profiles = new(); + profiles.CreateProfile("p", "Profile"); + profiles.SetActiveProfile("p"); + KeybindingService service = new(registry, profiles); + + for (int i = 0; i < CommandCount; i++) + { + registry.RegisterCommand(new Command($"c{i}", $"C{i}")); + } + + Chord chord = Chord.Parse("Ctrl+K"); + + // Odd commands stay bound, even ones are unbound again, and reads race the writes. + Parallel.For(0, CommandCount, i => + { + string commandId = $"c{i}"; + Assert.IsTrue(service.BindChord(commandId, chord)); + _ = service.FindCommandByChord(chord); + _ = service.GetAllChords(); + + if (i % 2 == 0) + { + Assert.IsTrue(service.UnbindChord(commandId)); + } + }); + + Profile profile = profiles.GetProfile("p")!; + Assert.AreEqual(CommandCount / 2, profile.ChordCount); + Assert.HasCount(CommandCount / 2, service.GetAllChords()); + Assert.IsTrue(service.GetAllChords().Keys.All(id => int.Parse(id[1..], CultureInfo.InvariantCulture) % 2 == 1)); + } +} diff --git a/Keybinding/KeybindingManager.cs b/Keybinding/KeybindingManager.cs index 66c4ae3..b73c08d 100644 --- a/Keybinding/KeybindingManager.cs +++ b/Keybinding/KeybindingManager.cs @@ -215,7 +215,7 @@ public KeybindingSummary GetSummary() Profile? activeProfile = Profiles.GetActiveProfile(); int totalCommands = Commands.GetAllCommands().Count; int totalProfiles = Profiles.GetAllProfiles().Count; - int activeKeybindings = activeProfile?.Chords.Count ?? 0; + int activeKeybindings = activeProfile?.ChordCount ?? 0; return new KeybindingSummary { diff --git a/Keybinding/Models/Profile.cs b/Keybinding/Models/Profile.cs index 5891d3f..2e33422 100644 --- a/Keybinding/Models/Profile.cs +++ b/Keybinding/Models/Profile.cs @@ -31,9 +31,14 @@ public Profile(string id, string name, string? description = null) Id = id.Trim(); Name = name.Trim(); Description = description?.Trim(); - Chords = []; } + [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0032:Use auto property", Justification = "The only property over this field is obsolete, and the field is what the lock guards.")] + private readonly Dictionary _chords = []; + + // Every read and write of _chords takes this lock, so bindings can change from any thread. + private readonly Lock _chordsLock = new(); + /// /// Gets the unique profile identifier /// @@ -68,9 +73,29 @@ internal void Rename(string name, string? description) } /// - /// Gets the chord bindings for this profile (command ID to chord mapping) + /// Gets the live chord bindings for this profile (command ID to chord mapping) /// - public Dictionary Chords { get; } + /// + /// Reading or changing this dictionary bypasses the profile's synchronization, so it is not + /// thread-safe. Use , , + /// and instead. + /// + [Obsolete("Chords is not thread-safe. Use GetAllChords, SetChord, RemoveChord or ClearChords instead.")] + public Dictionary Chords => _chords; + + /// + /// Gets the number of chord bindings in this profile + /// + public int ChordCount + { + get + { + lock (_chordsLock) + { + return _chords.Count; + } + } + } /// /// Sets a chord binding for a command in this profile @@ -88,7 +113,10 @@ public void SetChord(string commandId, Chord chord) Ensure.NotNull(chord); - Chords[commandId.Trim()] = chord; + lock (_chordsLock) + { + _chords[commandId.Trim()] = chord; + } } /// @@ -99,18 +127,47 @@ public void SetChord(string commandId, Chord chord) /// Thrown when commandId is null or whitespace public Chord? GetChord(string commandId) { - return string.IsNullOrWhiteSpace(commandId) - ? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)) - : Chords.TryGetValue(commandId.Trim(), out Chord? chord) - ? chord - : null; + if (string.IsNullOrWhiteSpace(commandId)) + { + throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)); + } + + lock (_chordsLock) + { + return _chords.TryGetValue(commandId.Trim(), out Chord? chord) ? chord : null; + } } /// /// Gets all chord bindings for this profile /// /// Dictionary of command ID to chord mappings - public IReadOnlyDictionary GetAllChords() => new Dictionary(Chords).AsReadOnly(); + public IReadOnlyDictionary GetAllChords() + { + lock (_chordsLock) + { + return new Dictionary(_chords).AsReadOnly(); + } + } + + /// + /// Finds the first command bound to a chord in this profile that satisfies a condition + /// + /// The chord to look up + /// An optional condition the command ID must satisfy + /// The command ID if found, null otherwise + internal string? FindCommand(Chord chord, Func? predicate = null) + { + foreach (KeyValuePair binding in GetAllChords()) + { + if (binding.Value.Equals(chord) && (predicate is null || predicate(binding.Key))) + { + return binding.Key; + } + } + + return null; + } /// /// Checks if a command has a chord binding in this profile @@ -120,9 +177,15 @@ public void SetChord(string commandId, Chord chord) /// Thrown when commandId is null or whitespace public bool HasChord(string commandId) { - return string.IsNullOrWhiteSpace(commandId) - ? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)) - : Chords.ContainsKey(commandId.Trim()); + if (string.IsNullOrWhiteSpace(commandId)) + { + throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)); + } + + lock (_chordsLock) + { + return _chords.ContainsKey(commandId.Trim()); + } } /// @@ -133,21 +196,42 @@ public bool HasChord(string commandId) /// Thrown when commandId is null or whitespace public bool RemoveChord(string commandId) { - return string.IsNullOrWhiteSpace(commandId) - ? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)) - : Chords.Remove(commandId.Trim()); + if (string.IsNullOrWhiteSpace(commandId)) + { + throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)); + } + + lock (_chordsLock) + { + return _chords.Remove(commandId.Trim()); + } } /// /// Gets all command IDs that have chord bindings in this profile /// /// Collection of command IDs - public IReadOnlyCollection BoundCommands => [.. Chords.Keys]; + public IReadOnlyCollection BoundCommands + { + get + { + lock (_chordsLock) + { + return [.. _chords.Keys]; + } + } + } /// /// Clears all chord bindings from this profile /// - public void ClearChords() => Chords.Clear(); + public void ClearChords() + { + lock (_chordsLock) + { + _chords.Clear(); + } + } /// /// Returns a string representation of the profile diff --git a/Keybinding/Services/JsonKeybindingRepository.cs b/Keybinding/Services/JsonKeybindingRepository.cs index a98c957..304f36b 100644 --- a/Keybinding/Services/JsonKeybindingRepository.cs +++ b/Keybinding/Services/JsonKeybindingRepository.cs @@ -59,7 +59,7 @@ public async Task SaveProfileAsync(Profile profile) Id = p.Id, Name = p.Name, Description = p.Description, - Chords = p.Chords.ToDictionary( + Chords = p.GetAllChords().ToDictionary( kvp => kvp.Key, kvp => new ChordDto { @@ -137,7 +137,7 @@ public async Task DeleteProfileAsync(string profileId) Id = p.Id, Name = p.Name, Description = p.Description, - Chords = p.Chords.ToDictionary( + Chords = p.GetAllChords().ToDictionary( kvp => kvp.Key, kvp => new ChordDto { diff --git a/Keybinding/Services/KeybindingService.cs b/Keybinding/Services/KeybindingService.cs index 03e2035..647bc27 100644 --- a/Keybinding/Services/KeybindingService.cs +++ b/Keybinding/Services/KeybindingService.cs @@ -247,9 +247,7 @@ public Chord ParseChord(string chordString) // bindings in place, so skip bindings whose command is no longer registered rather than // giving up on the first match. Profile? profile = _profileManager.GetProfile(profileId); - string? commandId = profile?.Chords - .FirstOrDefault(kvp => kvp.Value.Equals(chord) && _commandRegistry.IsCommandRegistered(kvp.Key)) - .Key; + string? commandId = profile?.FindCommand(chord, _commandRegistry.IsCommandRegistered); // In a real implementation, this would trigger command execution // For now, we just return the command ID that would be executed @@ -298,8 +296,6 @@ public bool HasChordBinding(string profileId, string commandId) } Profile? profile = _profileManager.GetProfile(profileId); - return profile?.Chords - .FirstOrDefault(kvp => kvp.Value.Equals(chord)) - .Key; + return profile?.FindCommand(chord); } } diff --git a/Keybinding/Services/ProfileManager.cs b/Keybinding/Services/ProfileManager.cs index 7d46fb1..923182d 100644 --- a/Keybinding/Services/ProfileManager.cs +++ b/Keybinding/Services/ProfileManager.cs @@ -155,7 +155,7 @@ public bool ProfileExists(string profileId) Profile newProfile = new(normalizedNewId, newProfileName.Trim(), newDescription); // Copy all chords from source profile - foreach (KeyValuePair kvp in sourceProfile.Chords) + foreach (KeyValuePair kvp in sourceProfile.GetAllChords()) { newProfile.SetChord(kvp.Key, kvp.Value); } From 1e9a91832cdfc206e6842a4e5dee4da6d2974996 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 28 Sep 2026 17:33:59 +0000 Subject: [PATCH 2/2] Cover Profile's locked chord accessors directly The new code's coverage fell below SonarCloud's gate. These tests exercise the rewritten accessors, their blank-id guards, the obsolete Chords property, and the registration filter ExecuteChord applies. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015BJVibRvmTotkqEGp3695R --- Keybinding.Test/ProfileChordAccessTests.cs | 87 ++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 Keybinding.Test/ProfileChordAccessTests.cs diff --git a/Keybinding.Test/ProfileChordAccessTests.cs b/Keybinding.Test/ProfileChordAccessTests.cs new file mode 100644 index 0000000..3f54cb4 --- /dev/null +++ b/Keybinding.Test/ProfileChordAccessTests.cs @@ -0,0 +1,87 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +[TestClass] +public class ProfileChordAccessTests +{ + private static readonly Chord CtrlA = Chord.Parse("Ctrl+A"); + private static readonly Chord CtrlB = Chord.Parse("Ctrl+B"); + private static readonly string[] BothCommands = ["a", "b"]; + + [TestMethod] + public void ChordAccessors_ReflectSetRemoveAndClear() + { + Profile profile = new("p", "Profile"); + + profile.SetChord(" a ", CtrlA); + profile.SetChord("b", CtrlB); + + Assert.AreEqual(2, profile.ChordCount); + Assert.AreEqual(CtrlA, profile.GetChord("a")); + Assert.IsNull(profile.GetChord("missing")); + Assert.IsTrue(profile.HasChord(" a ")); + Assert.IsFalse(profile.HasChord("missing")); + CollectionAssert.AreEquivalent(BothCommands, profile.BoundCommands.ToArray()); + + Assert.IsTrue(profile.RemoveChord("a")); + Assert.IsFalse(profile.RemoveChord("a")); + Assert.AreEqual(1, profile.ChordCount); + + profile.ClearChords(); + Assert.AreEqual(0, profile.ChordCount); + Assert.IsEmpty(profile.GetAllChords()); + } + + [TestMethod] + public void ChordAccessors_RejectBlankCommandIds() + { + Profile profile = new("p", "Profile"); + + Assert.ThrowsExactly(() => profile.SetChord(" ", CtrlA)); + Assert.ThrowsExactly(() => profile.GetChord(" ")); + Assert.ThrowsExactly(() => profile.HasChord(" ")); + Assert.ThrowsExactly(() => profile.RemoveChord(" ")); + } + + [TestMethod] + public void ObsoleteChords_StillExposesTheLiveBindings() + { + Profile profile = new("p", "Profile"); + profile.SetChord("a", CtrlA); + +#pragma warning disable CS0618 // Type or member is obsolete + Dictionary chords = profile.Chords; +#pragma warning restore CS0618 // Type or member is obsolete + + Assert.HasCount(1, chords); + profile.SetChord("b", CtrlB); + Assert.HasCount(2, chords, "Chords keeps returning the live dictionary for existing callers"); + } + + [TestMethod] + public void FindAndExecuteChord_UseTheLockedSnapshot() + { + CommandRegistry registry = new(); + ProfileManager profiles = new(); + profiles.CreateProfile("p", "Profile"); + profiles.SetActiveProfile("p"); + KeybindingService service = new(registry, profiles); + registry.RegisterCommand(new Command("a", "A")); + registry.RegisterCommand(new Command("b", "B")); + + Assert.IsTrue(service.BindChord("a", CtrlA)); + Assert.IsTrue(service.BindChord("b", CtrlB)); + + Assert.AreEqual("a", service.FindCommandByChord(CtrlA)); + Assert.IsNull(service.FindCommandByChord(Chord.Parse("Ctrl+C"))); + Assert.AreEqual("b", service.ExecuteChord(CtrlB)); + + registry.UnregisterCommand("b"); + Assert.IsNull(service.ExecuteChord(CtrlB), "A binding whose command is unregistered is skipped"); + Assert.AreEqual("b", service.FindCommandByChord(CtrlB), "FindCommandByChord does not filter by registration"); + } +}