Fix: resolve issue #4478 by using a temporary file for non-append writes #4624
Conversation
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
|
Please sign your DCO @onporat |
…n-append writes To address the issue where a failed write operation results in an empty file, we can use a temporary file for non-append writes. This ensures that the original file is only replaced once the new content is fully written and committed. **Key Changes:** 1. **Temporary File Handling:** - For non-append writes, a temporary file is created in the same directory as the target file. - All write operations are performed on the temporary file first. 2. **Atomic Commit:** - The temporary file is only renamed to the target path during `Commit()`, ensuring atomic replacement. - If `Commit()` fails, the temporary file is cleaned up. 3. **Error Handling:** - `Cancel()` properly removes temporary files if the operation is aborted. - `Close()` is made idempotent to handle multiple calls safely. 4. **Data Integrity:** - Directory sync after rename ensures metadata persistence. - Proper file flushing and syncing before rename operations. Signed-off-by: Oded Porat <onporat@gmail.com>
|
@milosgajdos Could you please approve the workflows? Thank you. |
…cess is interrupted, the solution involves writing to a temporary file first and then atomically renaming it to the target file. This ensures that the target file is only updated if the write completes successfully, preventing empty or partially written files. **Explanation:** 1. **Temporary File Creation:** The content is first written to a temporary file (appending `.tmp` to the original path). This ensures that the original file remains intact until the write is complete. 2. **Write to Temporary File:** Using the existing `Writer` with truncation (`false`), the content is written to the temporary file. If the write fails, the temporary file is closed and deleted. 3. **Commit and Rename:** After successfully writing to the temporary file, it is committed. Then, the temporary file is atomically renamed to the target path using `Move`, which is handled by the filesystem's rename operation (atomic on most systems). 4. **Cleanup on Failure:** If any step fails, the temporary file is cleaned up to avoid leaving orphaned files. Signed-off-by: Oded Porat <onporat@gmail.com>
|
@milosgajdos I updated the PR, could you please approve the workflows? Thank you. |
|
@milosgajdos Hello, could you please let me know whom I should ask to review this PR? Thanks. |
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses issue #4478 by introducing a safer file write mechanism that writes data to a temporary file before atomically replacing the target file to prevent empty or partially written files.
- Use a temporary file (subPath + ".tmp") for data writes
- Atomically rename the temporary file to the final target upon successful write
- Clean up the temporary file if any write or rename errors occur
milosgajdos
reviewed
May 3, 2025
thaJeztah
reviewed
May 7, 2025
Comment on lines
+140
to
+141
| // Write to a temporary file to prevent partial writes. | ||
| writer, err := d.Writer(ctx, tempPath, false) |
There was a problem hiding this comment.
Is there anything in the Writer and/or driver that's created that handles concurrency? (Mostly considering here that before this code, the destination would be a deterministic path, which (could be? need to check the implementation) used to prevent concurrent writes to the same file.
With this change, the content is written to a temporary file (no means to check concurrency), then a os.Rename()
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.
To address the issue where empty files are created when the write process is interrupted, the solution involves writing to a temporary file first and then atomically renaming it to the target file. This ensures that the target file is only updated if the write completes successfully, preventing empty or partially written files.
Explanation:
Temporary File Creation: The content is first written to a temporary file (appending
.tmpto the original path). This ensures that the original file remains intact until the write is complete.Write to Temporary File: Using the existing
Writerwith truncation (false), the content is written to the temporary file. If the write fails, the temporary file is closed and deleted.Commit and Rename: After successfully writing to the temporary file, it is committed. Then, the temporary file is atomically renamed to the target path using
Move, which is handled by the filesystem's rename operation (atomic on most systems).Cleanup on Failure: If any step fails, the temporary file 8000 is cleaned up to avoid leaving orphaned files.