Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings corrupted on Shutdown #7

Open
milos12345 opened this issue Dec 6, 2022 · 2 comments
Open

Settings corrupted on Shutdown #7

milos12345 opened this issue Dec 6, 2022 · 2 comments

Comments

@milos12345
Copy link

I occasionally get error in telemetry reports that settings file is corrupted if user shuts down computer while program is running. I already have in place shutdown blocking - program blocks shutdown with ShutdownBlockReasonCreate, runs Properties.Settings.Default.Save(); and then removes the shutdown block with ShutdownBlockReasonDestroy. I know that the user didn't press "shut down anyway" because the Serilog logs a message that the ShutdownBlockReasonDestroy was reached and completed, and that log is saved to disk and properly flushed, but for some reason the settings are not consistently saved.
Is there a way to ensure that settings are saved?
I am using PortableJsonSettingsProvider 0.2.1 from Nuget

[DllImport("user32.dll")]
public extern static bool ShutdownBlockReasonCreate(IntPtr hWnd, [MarshalAs(UnmanagedType.LPWStr)] string pwszReason);

[DllImport("user32.dll")]
public extern static bool ShutdownBlockReasonDestroy(IntPtr hWnd);
@alxnull
Copy link
Member

alxnull commented Dec 27, 2022

This issue might occur due to file caching happening on the OS side (see documentation here).

As described in the linked article, file caching can be disabled for specific files if necessary. In .NET, this option is realized by passing FileOptions.WriteThrough when opening a file stream.
Since the file stream persisting settings PortableJsonSettingsProvider is not publicly exposed currently, you can copy the implementation of PortableJsonSettingsProvider from the repository and modify the SetPropertyValues() method, e.g. as follows:

public override void SetPropertyValues(SettingsContext context, SettingsPropertyValueCollection collection)
{
    JObject jObject = GetJObject();
    foreach (SettingsPropertyValue value in collection)
        setSettingsValue(jObject, (string) context["GroupName"], value);
    
    try
    {
        using (var fs = new FileStream(ApplicationSettingsFile, FileMode.OpenOrCreate, FileAccess.Write, FileShare.None, 4096, FileOptions.WriteThrough))
        using (var sw = new StreamWriter(fs))
        {
            sw.WriteLine(JsonConvert.SerializeObject(JsonUtility.SortPropertiesAlphabetically(jObject), Formatting.Indented));
        }
    }
    catch { }
}

Please let me know if this resolves the issue you describe. If that's the case, I can consider adding a built-in option to enable this behavior.

@milos12345
Copy link
Author

Saving settings like this I have noticed that the resulting file would sometimes have some duplication

        "UseSmoothScrolling": "False"
      }
    }
  }
}
g": "False"
      }
    }
  }
}
  }
}

I would change settings and just call once
Properties.Settings.Default.Save();

It is not json serialization issue because saving two files at the same time will still produce the correct file with WriteAllText

using (var fs = new FileStream(ApplicationSettingsFile, FileMode.OpenOrCreate, FileAccess.Write, FileShare.None, 40960, FileOptions.WriteThrough))
                using (var sw = new StreamWriter(fs))
                {                    sw.WriteLine(JsonConvert.SerializeObject(JsonUtility.SortPropertiesAlphabetically(jObject), Formatting.Indented));
                }
                File.WriteAllText(ApplicationSettingsFile+"OLD",  JsonConvert.SerializeObject(JsonUtility.SortPropertiesAlphabetically(jObject),
                        Formatting.Indented));

The fix seems to be using FileMode.Create instead of FileMode.OpenOrCreate

I also suggest using Write instead of WriteLine, since the original
WriteAllText uses that (and doesn't produce an extra newline at the end)
https://referencesource.microsoft.com/#mscorlib/system/io/file.cs,f73b33d2cc64e5db,references

But according to this
https://itecnote.com/tecnote/c-file-writealltext-not-flushing-data-to-disk/

FileOptions.WriteThrough can replace Flush but you still need the temp file if your data can exceed a single cluster in size.

so that might be the only way to guarantee that corruption won't happen at some point anyway.

I will release a version with these changes and see if I get any additional corruption issues in telemetry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants