Write formatted files atomically to avoid corruption on write failure#5207
Open
ParamChordiya wants to merge 3 commits into
Open
Write formatted files atomically to avoid corruption on write failure#5207ParamChordiya wants to merge 3 commits into
ParamChordiya wants to merge 3 commits into
Conversation
format_file_in_place() previously opened the target file for writing directly, which truncates it immediately. If the write then failed partway through (e.g. disk full), the original file was left wiped or corrupted with no way to recover it. Now the reformatted contents are written to a temporary file in the same directory, fsync'd, and atomically swapped in with os.replace(). On any failure the temporary file is discarded and the original is left completely untouched. Symlinked targets are written through to their resolved real path so the symlink itself is preserved rather than being replaced by a regular file. Fixes psf#2479
for more information, see https://pre-commit.ci
os.readlink() on Windows returns the extended-length \\?\ path form, which doesn't match the plain path string passed to symlink_to(), failing the assertion on every Windows CI job even though the actual fix worked correctly (the symlink was preserved and pointed at the right file). Compare resolved paths instead, which normalizes both sides the same way on every platform.
Collaborator
|
Given that most other leading formatters don't do this, I'm not enthusiastic about the extra complexity here, and I suspect there's some risk of losing metadata associated with the original file. |
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.
Description
Fixes #2479.
Right now
format_file_in_placeopens the target file directly withopen(src, "w")before writing the reformatted contents. That call truncates the file immediately, so if the write fails partway through for any reason (disk fills up, permissions get revoked mid-run, process gets killed, etc.) the original file is left wiped or half-written with no way to get it back. Since most people run Black on files that aren't always safely committed yet, that's a pretty rough way to lose code.This was actually discussed in the issue back in 2021 - the suggestion from qlyoung (with JelleZijlstra agreeing) was to write to a temp file and swap it in, so that's what this PR does:
flush()+fsync()the temp file before doing anything else, so we know the data actually made it to disk.os.replace()it over the original.os.replaceis atomic on both POSIX and Windows, so there's never a window where the file is half-written.One thing that took a bit of extra care: if the path being formatted is a symlink,
os.replaceoperates on the symlink itself rather than what it points to, which would silently turn the symlink into a regular file. So there's a check for that - ifsrcis a symlink, we resolve it first and write through to the real target, leaving the symlink itself alone.Worth calling out (as was already flagged in the issue thread): this does mean Black now needs write permission on the containing directory, not just the file, since it has to create the temp file there. That seems like a reasonable trade-off for not losing people's work, but flagging it in case it's a concern for some setups.
Since JelleZijlstra asked in the thread whether other formatters handle this the same way, I went and checked a few:
TempFileutility and renames it over the target (llvm/lib/Support/Path.cpp).So it's a mixed bag rather than an industry standard - but there's real precedent for it in at least one widely-used formatter, and gofmt clearly cares about the same failure mode even though it solves it differently. Given Black is often run against files that aren't committed yet, I think it's worth fixing here regardless of what the others do.
On cooperlees' question about IO cost on large repos: I wasn't able to put together a reliable large-scale benchmark on my end, so take this as reasoning rather than a measured number. The extra work per file is one additional file creation, one
fsync, and onerename, all constant-time metadata operations against the same directory regardless of the file's size. Parsing and re-rendering the file - which already happens today - should dominate the per-file cost by a good margin for anything but a near-empty file. Happy to help put together a proper before/after benchmark against a large real-world codebase if that would help move this along, or run whatever perf harness the project already uses if there is one.Checklist - did you ...
--previewstyle, following the stability policy? (N/A - this doesn't change any formatting output, just how the result gets written to disk)CHANGES.mdif necessary?