Make settings survive crashes and shutdowns better

DesktopClock.Tests/SettingsTests.cs

@@ -2,6 +2,7 @@
 using System.IO;
 using System.Reflection;
 using System.Windows.Media;
+using System.Windows.Threading;
 using DesktopClock.Properties;
 
 namespace DesktopClock.Tests;
@@ -70,6 +71,132 @@ public void Save_ThenPopulate_ShouldRoundTripWpfAndTimeTypes()
         Assert.Equal(DateTimeKind.Unspecified, loaded.CountdownTo.Kind);
     }
 
+    [Fact]
+    public void Save_OverExistingFile_ShouldReplaceItWithoutLeavingTempFile()
+    {
+        using var _ = new TempSettingsFileScope();
+
+        var settings = CreateSettingsInstance();
+        settings.Format = "first";
+        Assert.True(settings.Save());
+
+        settings.Format = "second";
+        Assert.True(settings.Save());
+
+        var loaded = CreateSettingsInstance();
+        PopulateFromFile(loaded);
+
+        Assert.Equal("second", loaded.Format);
+        Assert.False(File.Exists(Settings.FilePath + ".tmp"));
+    }
+
+    [Fact]
+    public void ChangingASetting_ShouldSaveItShortlyAfterwards()
+    {
+        using var _ = new TempSettingsFileScope();
+
+        using var __ = new CanBeSavedScope();
+
+        var settings = CreateSettingsInstance();
+        settings.Format = "saved without exiting";
+
+        // The save runs on a short timer, so let the dispatcher run for a bit.
+        PumpDispatcher(TimeSpan.FromSeconds(2));
+
+        // Read the file directly; populating another instance here would queue its own save and leak into other tests.
+        Assert.Contains("saved without exiting", File.ReadAllText(Settings.FilePath));
+    }
+
+    [Fact]
+    public void EditingTheFile_ShouldCancelASaveStillWaitingFromAnEarlierChange()
+    {
+        using var _ = new TempSettingsFileScope();
+        using var __ = new CanBeSavedScope();
+
+        var settings = CreateSettingsInstance();
+        settings.Height = 99;
+
+        // Edit the file by hand before that change is saved, then let the watcher report it.
+        const string handEdit = "{ \"Format\": \"edited by hand\" }";
+        File.WriteAllText(Settings.FilePath, handEdit);
+        typeof(Settings).GetMethod("FileChanged", BindingFlags.NonPublic | BindingFlags.Instance)!.Invoke(settings, new object[] { null, null });
+        PumpDispatcher(TimeSpan.FromSeconds(2));
+
+        Assert.Equal("edited by hand", settings.Format);
+        Assert.Equal(handEdit, File.ReadAllText(Settings.FilePath));
+    }
+
+    [Fact]
+    public void Load_WithBrieflyLockedFile_ShouldWaitAndKeepSettings()
+    {
+        using var _ = new TempSettingsFileScope();
+
+        var original = CreateSettingsInstance();
+        original.Format = "kept through a lock";
+        Assert.True(original.Save());
+
+        // Hold the file like antivirus scanning it, then let go shortly after loading starts.
+        var lockStream = new FileStream(Settings.FilePath, FileMode.Open, FileAccess.Read, FileShare.None);
+        var releaser = new System.Threading.Thread(() =>
+        {
+            System.Threading.Thread.Sleep(150);
+            lockStream.Dispose();
+        });
+        releaser.Start();
+
+        var loaded = LoadAndAttemptSave();
+        releaser.Join();
+
+        Assert.Equal("kept through a lock", loaded.Format);
+    }
+
+    /// <summary>
+    /// Runs the app's startup load, restoring the static state it sets so other tests aren't affected.
+    /// </summary>
+    private static Settings LoadAndAttemptSave()
+    {
+        var canBeSaved = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!;
+        var originalCanBeSaved = Settings.CanBeSaved;
+
+        try
+        {
+            canBeSaved.Invoke(null, new object[] { false });
+
+            var loadAndAttemptSave = typeof(Settings).GetMethod("LoadAndAttemptSave", BindingFlags.NonPublic | BindingFlags.Static)!;
+            return (Settings)loadAndAttemptSave.Invoke(null, null)!;
+        }
+        finally
+        {
+            canBeSaved.Invoke(null, new object[] { originalCanBeSaved });
+        }
+    }
+
+    private static void PumpDispatcher(TimeSpan duration)
+    {
+        var frame = new DispatcherFrame();
+        var stopTimer = new DispatcherTimer { Interval = duration };
+        stopTimer.Tick += (_, _) =>
+        {
+            stopTimer.Stop();
+            frame.Continue = false;
+        };
+        stopTimer.Start();
+        Dispatcher.PushFrame(frame);
+    }
+
+    /// <summary>
+    /// Lets settings save on their own during a test, as they do once the app has confirmed the file is writable.
+    /// </summary>
+    private sealed class CanBeSavedScope : IDisposable
+    {
+        private static readonly MethodInfo _setCanBeSaved = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!;
+        private readonly bool _original = Settings.CanBeSaved;
+
+        public CanBeSavedScope() => _setCanBeSaved.Invoke(null, new object[] { true });
+
+        public void Dispose() => _setCanBeSaved.Invoke(null, new object[] { _original });
+    }
+
     private static Settings CreateSettingsInstance() =>
         (Settings)Activator.CreateInstance(typeof(Settings), nonPublic: true)!;
 

DesktopClock/MainWindow.xaml.cs

@@ -311,6 +311,7 @@ private void Window_MouseDown(object sender, MouseButtonEventArgs e)
 
             DragMove();
             PixelShifter?.UpdateBasePosition(this);
+            Settings.Default.Placement = this.GetPlacement();
             UpdateTimeString();
 
             _systemClockTimer.Start();
@@ -452,6 +453,7 @@ private void NudgeWindow(KeyEventArgs e)
         Top += nudge.Y;
 
         PixelShifter?.UpdateBasePosition(this);
+        Settings.Default.Placement = this.GetPlacement();
         e.Handled = true;
     }
 

DesktopClock/Properties/Settings.cs

@@ -1,7 +1,9 @@
-using System;
+using System;
 using System.ComponentModel;
 using System.IO;
+using System.Runtime.InteropServices;
 using System.Windows.Media;
+using System.Windows.Threading;
 using DesktopClock.Utilities;
 using Newtonsoft.Json;
 using WpfWindowPlacement;
@@ -11,6 +13,8 @@ namespace DesktopClock.Properties;
 public sealed class Settings : INotifyPropertyChanged, IDisposable
 {
     private readonly FileSystemWatcher _watcher;
+    private readonly DispatcherTimer _saveTimer;
+    private bool _populatingFromFile;
     private string _resolvedTimeZoneId;
     private TimeZoneInfo _resolvedTimeZone;
     private static readonly Lazy<Settings> _default = new(LoadAndAttemptSave);
@@ -40,6 +44,23 @@ private Settings()
             EnableRaisingEvents = true,
         };
         _watcher.Changed += FileChanged;
+
+        // Save shortly after a change instead of only on exit, so a crash, a forced close, or a shutdown that doesn't let the app exit normally only loses the last moment of changes. The short wait groups rapid changes, like dragging a slider, into one save.
+        _saveTimer = new DispatcherTimer { Interval = TimeSpan.FromSeconds(1) };
+        _saveTimer.Tick += (_, _) =>
+        {
+            _saveTimer.Stop();
+            Save();
+        };
+        PropertyChanged += (_, _) =>
+        {
+            // Values that were just read from the file are already saved.
+            if (!CanBeSaved || _populatingFromFile)
+                return;
+
+            _saveTimer.Stop();
+            _saveTimer.Start();
+        };
     }
 
 #pragma warning disable CS0067 // The event 'Settings.PropertyChanged' is never used. Handled by Fody.
@@ -441,7 +462,7 @@ public bool Save()
             {
                 try
                 {
-                    File.WriteAllText(FilePath, json);
+                    WriteAllTextAtomically(FilePath, json);
                     return true;
                 }
                 catch
@@ -463,6 +484,34 @@ public bool Save()
         return false;
     }
 
+    /// <summary>
+    /// Writes to a temporary file and then swaps it in, so the file is never left empty or half-written if the app is killed mid-save, such as during a Windows shutdown (#7).
+    /// </summary>
+    private static void WriteAllTextAtomically(string path, string contents)
+    {
+        var tempPath = path + ".tmp";
+
+        using (var stream = new FileStream(tempPath, FileMode.Create, FileAccess.Write))
+        using (var writer = new StreamWriter(stream))
+        {
+            writer.Write(contents);
+            writer.Flush();
+
+            // Make sure the new contents are on disk before they replace the old ones.
+            stream.Flush(flushToDisk: true);
+        }
+
+        // Swap it in with a single rename; File.Replace isn't safe here because it renames the old file away first, so being killed in between leaves no settings file at all.
+        if (!MoveFileEx(tempPath, path, MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH))
+            throw new IOException($"Couldn't replace {path}.", Marshal.GetHRForLastWin32Error());
+    }
+
+    private const int MOVEFILE_REPLACE_EXISTING = 0x1;
+    private const int MOVEFILE_WRITE_THROUGH = 0x8;
+
+    [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)]
+    private static extern bool MoveFileEx(string existingFileName, string newFileName, int flags);
+
     /// <summary>
     /// Populates the given settings with values from the default path.
     /// </summary>
@@ -480,15 +529,24 @@ private static void Populate(Settings settings)
     /// </summary>
     private static Settings LoadFromFile()
     {
-        try
-        {
-            var settings = new Settings();
-            Populate(settings);
-            return settings;
-        }
-        catch
+        var settings = new Settings();
+
+        // The file can be locked for a moment, such as while antivirus scans it at sign-in, and falling back to defaults here would save them over every setting. Give it about as long as saving does before giving up.
+        for (var attempt = 1; ; attempt++)
         {
-            return new();
+            try
+            {
+                Populate(settings);
+                return settings;
+            }
+            catch (IOException) when (attempt < 4 && Exists)
+            {
+                System.Threading.Thread.Sleep(250);
+            }
+            catch
+            {
+                return new();
+            }
         }
     }
 
@@ -499,7 +557,7 @@ private static Settings LoadAndAttemptSave()
     {
         var settings = LoadFromFile();
 
-        if (!File.Exists(FilePath))
+        if (!Exists)
         {
             settings.ApplySystemThemeDefaultsIfAvailable();
         }
@@ -514,13 +572,24 @@ private static Settings LoadAndAttemptSave()
     /// </summary>
     private void FileChanged(object sender, FileSystemEventArgs e)
     {
-        try
+        // Reload on the thread that saves and cancel any save still waiting from an earlier change, so it can't write a half-reloaded file or put old values back over the edit.
+        _saveTimer.Dispatcher.BeginInvoke(new Action(() =>
         {
-            Populate(this);
-        }
-        catch
-        {
-        }
+            _saveTimer.Stop();
+
+            try
+            {
+                _populatingFromFile = true;
+                Populate(this);
+            }
+            catch
+            {
+            }
+            finally
+            {
+                _populatingFromFile = false;
+            }
+        }));
     }
 
     private void ApplySystemThemeDefaultsIfAvailable()