From 4a2857d08a1412279c4aeeabfb3510bfa2522435 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 11:27:51 +0000 Subject: [PATCH] Serialize a Profile's bindings from a locked snapshot, so another thread's edits can't break it [patch] System.Text.Json wrote a profile's bindings by enumerating the live Chords dictionary with no lock, so serializing while another thread changed bindings threw "Collection was modified". The obsolete Chords property is now [JsonIgnore], and a private ChordsSnapshot property copies the bindings under the lock and is written as "Chords". The JSON shape is unchanged. Fixes ktsu-dev/Keybinding#229 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW499sBmEvZWVdPTbTv --- .../ProfileJsonConcurrencyTests.cs | 68 +++++++++++++++++++ Keybinding/Models/Profile.cs | 28 ++++++-- 2 files changed, 91 insertions(+), 5 deletions(-) create mode 100644 Keybinding.Test/ProfileJsonConcurrencyTests.cs diff --git a/Keybinding.Test/ProfileJsonConcurrencyTests.cs b/Keybinding.Test/ProfileJsonConcurrencyTests.cs new file mode 100644 index 0000000..eecd023 --- /dev/null +++ b/Keybinding.Test/ProfileJsonConcurrencyTests.cs @@ -0,0 +1,68 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using System.Text.Json; +using ktsu.Keybinding.Core.Models; + +[TestClass] +public class ProfileJsonConcurrencyTests +{ + private static readonly string[] ProfileJsonProperties = ["Id", "Name", "Description", "Chords", "ChordCount", "BoundCommands"]; + + [TestMethod] + [Timeout(60000, CooperativeCancellation = true)] + public void Serialize_WhileAnotherThreadChangesBindings_WritesASnapshot() + { + Profile profile = new("p", "Profile"); + for (int i = 0; i < 200; i++) + { + profile.SetChord($"cmd{i}", Chord.Parse($"Ctrl+F{(i % 12) + 1}")); + } + + using CancellationTokenSource stop = new(); + Task writer = Task.Run(() => + { + int i = 0; + while (!stop.IsCancellationRequested) + { + string id = $"extra{i++ % 50}"; + profile.SetChord(id, Chord.Parse("Ctrl+Q")); + profile.RemoveChord(id); + } + }); + + try + { + for (int i = 0; i < 2000; i++) + { + string json = JsonSerializer.Serialize(profile); + using JsonDocument document = JsonDocument.Parse(json); + JsonElement chords = document.RootElement.GetProperty("Chords"); + int count = chords.EnumerateObject().Count(); + Assert.IsTrue(count is >= 200 and <= 250, $"Serialized {count} bindings."); + } + } + finally + { + stop.Cancel(); + writer.Wait(); + } + } + + [TestMethod] + public void Serialize_KeepsTheJsonShape() + { + Profile profile = new("p", "Profile", "Description"); + profile.SetChord("file.save", Chord.Parse("Ctrl+S")); + + using JsonDocument document = JsonDocument.Parse(JsonSerializer.Serialize(profile)); + JsonElement root = document.RootElement; + + CollectionAssert.AreEquivalent( + ProfileJsonProperties, + root.EnumerateObject().Select(p => p.Name).ToArray()); + Assert.AreEqual(1, root.GetProperty("Chords").EnumerateObject().Count()); + Assert.AreEqual(JsonSerializer.Serialize(Chord.Parse("Ctrl+S")), root.GetProperty("Chords").GetProperty("file.save").GetRawText()); + } +} diff --git a/Keybinding/Models/Profile.cs b/Keybinding/Models/Profile.cs index ec16630..b7217db 100644 --- a/Keybinding/Models/Profile.cs +++ b/Keybinding/Models/Profile.cs @@ -35,16 +35,16 @@ public Profile(string id, string name, string? description = null) Description = description?.Trim(); } - // The Chords property is get-only, so the serializer can only restore the bindings through a - // constructor parameter of the same name and type. Without it a round-tripped profile comes back empty. + // The bindings are written through the get-only ChordsSnapshot, so the serializer can only restore them + // through a constructor parameter of the same CLR name and type. Without it a round-tripped profile comes back empty. [JsonConstructor] [System.Diagnostics.CodeAnalysis.SuppressMessage("CodeQuality", "IDE0051:Remove unused private members", Justification = "Called by System.Text.Json through [JsonConstructor].")] - private Profile(string? id, string? name, string? description, Dictionary? chords) + private Profile(string? id, string? name, string? description, Dictionary? chordsSnapshot) : this(JsonArguments.RequireText(id, nameof(Profile), nameof(Id)), JsonArguments.RequireText(name, nameof(Profile), nameof(Name)), description) { - if (chords is not null) + if (chordsSnapshot is not null) { - foreach (KeyValuePair binding in chords) + foreach (KeyValuePair binding in chordsSnapshot) { Chord chord = binding.Value ?? throw new JsonException($"{nameof(Profile)} binding '{binding.Key}' has no chord."); try @@ -107,8 +107,26 @@ internal void Rename(string name, string? description) /// and instead. /// [Obsolete("Chords is not thread-safe. Use GetAllChords, SetChord, RemoveChord or ClearChords instead.")] + [JsonIgnore] public Dictionary Chords => _chords; + // What System.Text.Json writes and reads as "Chords": a copy taken under the lock, so serializing a profile + // while another thread changes its bindings writes one consistent set instead of enumerating the live + // dictionary. Its type matches the [JsonConstructor] parameter, which is how the bindings are read back. + [JsonInclude] + [JsonPropertyName("Chords")] + [System.Diagnostics.CodeAnalysis.SuppressMessage("CodeQuality", "IDE0051:Remove unused private members", Justification = "Read by System.Text.Json through [JsonInclude].")] + private Dictionary ChordsSnapshot + { + get + { + lock (_chordsLock) + { + return _chords.ToDictionary(b => b.Key, b => (Chord?)b.Value); + } + } + } + /// /// Gets the number of chord bindings in this profile ///