mirror of
https://github.com/rmcrackan/Libation.git
synced 2026-09-12 21:57:19 -04:00
fix(config): replace Settings.json atomically instead of truncating it
File.WriteAllText truncates the destination before writing, so an interrupted write leaves a half-written or empty Settings.json, and the in-process lock added in the previous commit cannot help a reader in another process - the GUI and the CLI share this file. Route every write through Dinah.Core.IO.AtomicFileWriter, which writes a sibling temp file, flushes to disk and renames it over the destination. Validate the temp file parses as json before the swap, the same way JsonFilePersister<T> already saves AccountsSettings.json, so a bad payload leaves the existing file untouched. Co-authored-by: rmcrackan <rmcrackan@gmail.com>
This commit is contained in:
2 files changed
+92
-5
No files matched your search
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="Dinah.Core.IO.JsonFilePersister{T}"/> saves AccountsSettings.json.
|
||||
/// </summary>
|
||||
private void writeFileContents(string contents)
|
||||
=> AtomicFileWriter.WriteAllText(Filepath, contents, validateJsonTempFile);
|
||||
|
||||
/// <summary>Throws before the temp file replaces the real one, leaving the real one untouched.</summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>Caller must hold <see cref="locker"/>.</summary>
|
||||
private JObject readFile()
|
||||
{
|
||||
@@ -315,7 +336,7 @@ public class PersistentDictionary : IJsonBackedDictionary
|
||||
|
||||
private void createNewFile()
|
||||
{
|
||||
File.WriteAllText(Filepath, "{}");
|
||||
writeFileContents("{}");
|
||||
}
|
||||
|
||||
public JObject GetJObject()
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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<JObject>(contents));
|
||||
}
|
||||
});
|
||||
|
||||
// no temp files left behind
|
||||
Assert.AreEqual(1, Directory.GetFiles(Path.GetDirectoryName(file)!).Length);
|
||||
}
|
||||
finally
|
||||
{
|
||||
deleteSettingsFile(file);
|
||||
}
|
||||
}
|
||||
|
||||
[TestMethod]
|
||||
public void ReadWhileWriting_DoesNotThrow()
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user