mirror of
https://github.com/rmcrackan/Libation.git
synced 2026-09-12 21:57:19 -04:00
fix(config): outlast a brief lock on Settings.json, and scope the reader test to unix
Windows CI caught the atomic replace failing with UnauthorizedAccessException: renaming over a file is denied while another handle holds it open, however generously that handle shares the file. In production the CLI, a second GUI instance or a virus scanner can each hold Settings.json for a moment, so retry the replace a few times before letting the caller see the failure. The previous File.WriteAllText threw on the same holds, so this is strictly more forgiving. ExternalReaderNeverSeesAPartiallyWrittenFile keeps a handle open almost continuously, which no retry budget can outlast on Windows, so restrict it to unix where it actually tests write atomicity. Write_SurvivesATemporarilyUnwritableDirectory covers the retry instead by revoking write permission on the containing directory. Co-authored-by: rmcrackan <rmcrackan@gmail.com>
This commit is contained in:
2 files changed
+77
-1
No files matched your search
@@ -289,9 +289,27 @@ public class PersistentDictionary : IJsonBackedDictionary
|
||||
/// 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.
|
||||
/// <para/>
|
||||
/// Windows refuses to rename over a file while someone else holds a handle to it, and the CLI,
|
||||
/// another GUI instance or a virus scanner can all hold Settings.json for a moment, so retry
|
||||
/// before giving up and letting the caller see the failure.
|
||||
/// </summary>
|
||||
private void writeFileContents(string contents)
|
||||
=> AtomicFileWriter.WriteAllText(Filepath, contents, validateJsonTempFile);
|
||||
{
|
||||
const int attempts = 5;
|
||||
for (var attempt = 1; ; attempt++)
|
||||
{
|
||||
try
|
||||
{
|
||||
AtomicFileWriter.WriteAllText(Filepath, contents, validateJsonTempFile);
|
||||
return;
|
||||
}
|
||||
catch (Exception ex) when (attempt < attempts && ex is IOException or UnauthorizedAccessException)
|
||||
{
|
||||
Thread.Sleep(20 * attempt);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Throws before the temp file replaces the real one, leaving the real one untouched.</summary>
|
||||
private static void validateJsonTempFile(string tempPath)
|
||||
|
||||
@@ -200,10 +200,17 @@ 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.
|
||||
/// <para/>
|
||||
/// Unix only. These readers hold a handle almost continuously, and Windows denies a rename over
|
||||
/// an open file however generously the reader shares it, so on Windows this would test the
|
||||
/// retry in <c>writeFileContents</c> rather than the atomicity of the write.
|
||||
/// </summary>
|
||||
[TestMethod]
|
||||
public void ExternalReaderNeverSeesAPartiallyWrittenFile()
|
||||
{
|
||||
if (Environment.OSVersion.Platform != PlatformID.Unix)
|
||||
Assert.Inconclusive($"Skipped because OS is not {PlatformID.Unix}.");
|
||||
|
||||
var file = createSettingsFile();
|
||||
try
|
||||
{
|
||||
@@ -261,6 +268,57 @@ public class PersistentDictionaryConcurrencyTests
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Windows denies a rename over a file another handle holds open, and the CLI, a second GUI or a
|
||||
/// virus scanner can all do that for a moment, so a write has to outlast a brief denial. Revoking
|
||||
/// write permission on the containing directory reproduces that denial portably.
|
||||
/// </summary>
|
||||
[TestMethod]
|
||||
public void Write_SurvivesATemporarilyUnwritableDirectory()
|
||||
{
|
||||
if (!OperatingSystem.IsLinux() && !OperatingSystem.IsMacOS())
|
||||
Assert.Inconclusive("Skipped because revoking directory write permission needs unix file modes.");
|
||||
|
||||
var file = createSettingsFile();
|
||||
var directory = Path.GetDirectoryName(file)!;
|
||||
var original = File.GetUnixFileMode(directory);
|
||||
try
|
||||
{
|
||||
var dictionary = new PersistentDictionary(file);
|
||||
File.SetUnixFileMode(directory, UnixFileMode.UserRead | UnixFileMode.UserExecute);
|
||||
|
||||
if (canCreateFileIn(directory))
|
||||
Assert.Inconclusive("Skipped because this user can write to a read-only directory (running as root?).");
|
||||
|
||||
// shorter than the retry budget in writeFileContents
|
||||
using var restore = new Timer(_ => File.SetUnixFileMode(directory, original), null, dueTime: 50, period: Timeout.Infinite);
|
||||
|
||||
dictionary.SetString("WrittenDespiteTheOutage", "value");
|
||||
|
||||
Assert.AreEqual("value", new PersistentDictionary(file).GetString("WrittenDespiteTheOutage"));
|
||||
}
|
||||
finally
|
||||
{
|
||||
try { File.SetUnixFileMode(directory, original); } catch { /* ignore */ }
|
||||
deleteSettingsFile(file);
|
||||
}
|
||||
}
|
||||
|
||||
private static bool canCreateFileIn(string directory)
|
||||
{
|
||||
var probe = Path.Combine(directory, Guid.NewGuid().ToString("N"));
|
||||
try
|
||||
{
|
||||
File.WriteAllText(probe, "");
|
||||
File.Delete(probe);
|
||||
return true;
|
||||
}
|
||||
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
[TestMethod]
|
||||
public void ReadWhileWriting_DoesNotThrow()
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user