Serialize a Profile's bindings from a locked snapshot, so another thread's edits can't break it [patch]

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());
+	}
+}

Keybinding/Models/Profile.cs

@@ -35,16 +35,16 @@
 		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<string, Chord?>? chords)
+	private Profile(string? id, string? name, string? description, Dictionary<string, Chord?>? 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<string, Chord?> binding in chords)
+			foreach (KeyValuePair<string, Chord?> binding in chordsSnapshot)
 			{
 				Chord chord = binding.Value ?? throw new JsonException($"{nameof(Profile)} binding '{binding.Key}' has no chord.");
 				try
@@ -106,9 +106,27 @@
 	/// thread-safe. Use <see cref="GetAllChords"/>, <see cref="SetChord"/>, <see cref="RemoveChord"/>
 	/// and <see cref="ClearChords"/> instead.
 	/// </remarks>
 	[Obsolete("Chords is not thread-safe. Use GetAllChords, SetChord, RemoveChord or ClearChords instead.")]
+	[JsonIgnore]
 	public Dictionary<string, Chord> 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<string, Chord?> ChordsSnapshot
+	{
+		get
+		{
+			lock (_chordsLock)
+			{
+				return _chords.ToDictionary(b => b.Key, b => (Chord?)b.Value);
+			}
+		}
+	}
+
 	/// <summary>
 	/// Gets the number of chord bindings in this profile
 	/// </summary>
@@ -134,7 +152,7 @@
 	{
 		if (string.IsNullOrWhiteSpace(commandId))
 		{
 			throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId));
 		}
 
 		Ensure.NotNull(chord);