Make settings survive crashes and shutdowns better - #165
Merged
Merged
Conversation
Saving truncated the settings file and then wrote it, so an app killed mid-save, such as by a Windows shutdown, could leave the file empty and every setting lost (#7). Killing the app around saves reproduced this in 2 of 150 runs. Now the contents go to a temporary file that is flushed to disk and then renamed over the old one in a single step, and the same test kept the file intact in 150 of 150 runs. File.Replace isn't used because it renames the old file away first, and a kill in between left no settings file at all in testing. A swapped-in file is reported to the watcher as a rename rather than a change, so the watcher now handles renames too. That keeps live reload working for other instances sharing the file, and for editors that save the same way (0 of 8 such edits reloaded before, 8 of 8 now). The file is often still locked for a moment when the event arrives, so reloading retries briefly instead of giving up on the first attempt.
Settings were written only when the clock closed normally, so a crash, a forced close, or a shutdown that didn't let the app exit lost every change made that session. Now a change is saved a second after the last one, which also groups rapid changes like dragging a slider into one save. The clock's position is saved after dragging or nudging it too, since it used to be read only on exit. Values reloaded from the file don't trigger a save, so editing the file by hand is never rewritten by the app. Verified live: after changing a setting and nudging the clock, an unhandled exception two seconds later kept both on disk (both were lost before), and a hand edit was applied without the file being touched.
When the settings file couldn't be read at startup, the app loaded defaults and immediately saved them over it, so a single typo made while editing it by hand wiped every setting with nothing left to recover. Now startup follows one rule: - A missing or empty file starts fresh with colors from the system theme. Older versions could leave an empty file behind when closed mid-save (#7). - A file that can't be read is moved to DesktopClock.settings.bak and a notification says so, then the app starts fresh the same way. - A file that's only locked for a moment, such as while antivirus scans it, is retried briefly instead of being treated as unreadable. Before, that also reset every setting. Reloading after an edit uses the same retry.
Keep this PR focused on settings surviving crashes and shutdowns. Removed: - Moving an unreadable file to a .bak with a notification. It only helped someone who breaks the file by hand and then restarts the app. - Reloading when the file is renamed into place, along with the retry after that event. That only mattered for editors that save that way, and Notepad edits in place. - Treating an empty file as a fresh start. Saves can't leave empty files anymore. Startup still retries a briefly locked file instead of saving defaults over it, now waiting about as long as saving does.
A change made in the app starts a one-second save timer on the UI thread, but reloading a hand edit ran on the file watcher's thread. If the file was edited in that second, the waiting save could write a half-reloaded file or put the app's values back over the edit. Now the reload runs on the UI thread and stops the waiting save first. The timer ticks at a lower priority than the reload, so a queued reload always runs before a queued save. Added a test that fails without this: a pending save rewrote a hand-edited file with the full settings.
danielchalmers
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes settings survive crashes, forced closes, and shutdowns:
Saving truncated the file and then wrote it, so being killed mid-save left it empty. Killing the app around saves reproduced a 0-byte file in 2 of 150 runs. Now it writes a temp file, flushes it to disk, and renames it over the old one in a single step with
MoveFileEx: 150 of 150 intact.File.Replaceisn't used because it renames the old file away first; a kill in between left no settings file at all in testing.Changes are saved a second after the last one; dragging a slider still produces one save. The position is also saved after dragging or nudging the clock. After changing a setting and nudging the clock, an unhandled exception 2 seconds later kept both on disk (both were lost before). Reloading a hand edit cancels any save still waiting, so the app doesn't write over the edit.
If the file was locked for a moment at startup (antivirus at sign-in), the app loaded defaults and saved them over every setting. It now retries for about as long as saving does first.