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

Should write to mimeapps.list atomically to prevent intermediate states or corruption #43

Open
nyanpasu64 opened this issue Jun 10, 2021 · 2 comments · May be fixed by #44
Open

Should write to mimeapps.list atomically to prevent intermediate states or corruption #43

nyanpasu64 opened this issue Jun 10, 2021 · 2 comments · May be fixed by #44

Comments

@nyanpasu64
Copy link

I think it makes sense to atomically write to mimeapps.list, by writing to a temporary file, then renaming it on top of mimeapps.list.

  • This prevents other apps running concurrently from seeing empty or partial mimeapps.list contents if it reads while handlr is writing to the file.
  • Additionally, you can fsync the temporary file before renaming it on top of mimeapps.list. If you don't do this, the risk is that a power outage will result in the temporary file being renamed to mimeapps.list, but the contents of the temporary file not being written to disk. Performing a fsync reduces the chances of this happening, but has a performance penalty on ext3 (which isn't commonly used anymore I hope), and may not necessarily work on flash memory (because apparently some disks lie about having persisted blocks to nonvolatile storage).

https://danluu.com/file-consistency/ describes the pitfalls of filesystems and fsync. It describes using log files for atomicity, and advises against rename-based atomic saves. However, rename-based saving is our only option, since apps don't know how to read log files but only mimeapps.list. And GLib uses rename and fsync, so it should be fine...

  • GLib writing to mimeapps.list calls g_file_set_contents_full().
    • Passes G_FILE_SET_CONTENTS_CONSISTENT (create temporary file, fsync the temp file, then rename). Does not pass G_FILE_SET_CONTENTS_DURABLE (fsync the directory after renaming). I agree with these choices.
    • Passes G_FILE_SET_CONTENTS_ONLY_EXISTING (don't write to a temp file if the file if it doesn't exist yet). I disagree with this choice, because it only improves performance in the rare situation where mimeapps.list doesn't exist at all, but introduces an edge case where a corrupted file can be written to disk or read by another app, and the exact reason why is hard to diagnose.
  • g_file_set_contents_full() docs
  • GFileSetContentsFlags flags docs

You can implement atomic saving yourself, or shell out to a crate like atomicwrites or tempfile's persist method (not sure why there are two of them, link, link). Note that tempfile's persist method mentions the risk of the tempfile being deleted or replaced with a different file under the same name. I haven't researched the best approach to take, the crate options out there, or where to create a temp file, or the optimal syscall for renaming. (capnproto's kj filesystem library creates a temporary file in the same directory and uses renameat().)

@nyanpasu64 nyanpasu64 linked a pull request Jun 11, 2021 that will close this issue
@chmln
Copy link
Owner

chmln commented Jul 7, 2021

Hey @nyanpasu64 thanks a lot for your extensive research and contribution ! Do you think you could publish #44 as a library instead and have handlr simply use it ?

@nyanpasu64
Copy link
Author

If you want to take the easy way out, you can just use the upstream atomicwrites library, and if they add support for disabling fsync on directory, you can add that as an optimization later. I think that's a better outcome than me publishing a new crate solely serving as an edited version of atomicwrites that only works on Linux.

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

Successfully merging a pull request may close this issue.

2 participants