diff --git a/Source/FileManager/PersistentDictionary.cs b/Source/FileManager/PersistentDictionary.cs
index a148abf4..2a40a4fc 100644
--- a/Source/FileManager/PersistentDictionary.cs
+++ b/Source/FileManager/PersistentDictionary.cs
@@ -1,4 +1,5 @@
-using Newtonsoft.Json;
+using Dinah.Core.IO;
+using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using System;
using System.Collections.Generic;
@@ -21,6 +22,9 @@ public class PersistentDictionary : IJsonBackedDictionary
// the UI thread, BackgroundWorker callbacks and download workers simultaneously. Every cache and
// file access below must be serialized: unsynchronized Dictionary writes corrupt the cache, and
// unsynchronized file access lets a reader observe a half-written file.
+ // This lock cannot reach a second process (the GUI and the CLI share Settings.json), which is why
+ // every write goes through AtomicFileWriter: an outside reader sees either the old file or the new
+ // one, never a truncated one.
private Lock locker { get; } = new();
public PersistentDictionary(string filepath, bool isReadOnly = false)
@@ -182,7 +186,7 @@ public class PersistentDictionary : IJsonBackedDictionary
var endContents = JsonConvert.SerializeObject(jObject, Formatting.Indented);
- File.WriteAllText(Filepath, endContents);
+ writeFileContents(endContents);
success = true;
}
Serilog.Log.Logger.Information("Removed property. {propertyName}", propertyName);
@@ -209,7 +213,7 @@ public class PersistentDictionary : IJsonBackedDictionary
if (startContents == endContents)
return false;
- File.WriteAllText(Filepath, endContents);
+ writeFileContents(endContents);
return true;
}
@@ -252,7 +256,7 @@ public class PersistentDictionary : IJsonBackedDictionary
return false;
token[propertyName] = newValue;
- File.WriteAllText(Filepath, JsonConvert.SerializeObject(jObject, Formatting.Indented));
+ writeFileContents(JsonConvert.SerializeObject(jObject, Formatting.Indented));
}
}
catch (Exception exDebug)
@@ -281,6 +285,23 @@ public class PersistentDictionary : IJsonBackedDictionary
: value.Length > 100 ? $"[Length={value.Length}] {value[0..50]}...{value[^50..^0]}"
: value;
+ ///
+ /// Replaces the settings file in one step, so a concurrent reader - including one in another
+ /// Libation process - sees either the whole old file or the whole new one. Mirrors how
+ /// saves AccountsSettings.json.
+ ///
+ private void writeFileContents(string contents)
+ => AtomicFileWriter.WriteAllText(Filepath, contents, validateJsonTempFile);
+
+ /// Throws before the temp file replaces the real one, leaving the real one untouched.
+ private static void validateJsonTempFile(string tempPath)
+ {
+ var contents = File.ReadAllText(tempPath);
+ if (string.IsNullOrWhiteSpace(contents))
+ throw new JsonSerializationException($"Refusing to write an empty settings file to {tempPath}");
+ JToken.Parse(contents);
+ }
+
/// Caller must hold .
private JObject readFile()
{
@@ -315,7 +336,7 @@ public class PersistentDictionary : IJsonBackedDictionary
private void createNewFile()
{
- File.WriteAllText(Filepath, "{}");
+ writeFileContents("{}");
}
public JObject GetJObject()
diff --git a/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs b/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs
index 215896a0..8e86aa49 100644
--- a/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs
+++ b/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs
@@ -4,6 +4,7 @@ using System.Collections.Generic;
using System.IO;
using System.Threading;
using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
namespace FileManager.Tests;
@@ -195,6 +196,71 @@ public class PersistentDictionaryConcurrencyTests
}
}
+ ///
+ /// The lock cannot reach a second process - the GUI and the CLI share one Settings.json - so a
+ /// write must never leave the file truncated. Reading it straight off disk, bypassing the
+ /// dictionary, stands in for that outside reader.
+ ///
+ [TestMethod]
+ public void ExternalReaderNeverSeesAPartiallyWrittenFile()
+ {
+ var file = createSettingsFile();
+ try
+ {
+ var dictionary = new PersistentDictionary(file);
+
+ // a payload big enough that a non-atomic write has a window to be caught mid-flight
+ var padding = new string('x', 64 * 1024);
+ var done = false;
+
+ runConcurrently(thread =>
+ {
+ if (thread == 0)
+ {
+ try
+ {
+ for (var i = 0; i < Keys; i++)
+ dictionary.SetString("Padded", $"{padding}{i}");
+ }
+ finally
+ {
+ Volatile.Write(ref done, true);
+ }
+ return;
+ }
+
+ while (!Volatile.Read(ref done))
+ {
+ string contents;
+ try
+ {
+ // share the file the way a cooperative outside reader would, so an atomic
+ // replace is never blocked by this test
+ using var stream = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete);
+ using var reader = new StreamReader(stream);
+ contents = reader.ReadToEnd();
+ }
+ catch (IOException)
+ {
+ // a sharing violation is the OS refusing the read, not a corrupt file
+ continue;
+ }
+
+ Assert.IsFalse(string.IsNullOrWhiteSpace(contents), "read an empty Settings.json mid-write");
+ // throws JsonReaderException on a truncated file
+ Assert.IsNotNull(JsonConvert.DeserializeObject(contents));
+ }
+ });
+
+ // no temp files left behind
+ Assert.AreEqual(1, Directory.GetFiles(Path.GetDirectoryName(file)!).Length);
+ }
+ finally
+ {
+ deleteSettingsFile(file);
+ }
+ }
+
[TestMethod]
public void ReadWhileWriting_DoesNotThrow()
{