-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add WithSyncDirectory to optionally fsync the directory after the rename #42
base: master
Are you sure you want to change the base?
Conversation
Greetings from quis@ :) |
@@ -167,6 +168,16 @@ func (t *PendingFile) CloseAtomicallyReplace() error { | |||
return err | |||
} | |||
t.done = true | |||
if t.syncDirectory { | |||
dh, err := os.OpenFile(filepath.Dir(t.path), os.O_WRONLY, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Linux this doesn't work. (Error: open <file>: is a directory
)
Simply os.Open(filepath.Dir(t.path))
does work instead.
See also:
-
https://linux.die.net/man/2/fsync
On some UNIX systems (but not Linux), fd must be a writable file descriptor.
-
https://austin-group-l.opengroup.narkive.com/vC4Fjvsn/fsync-ing-a-directory-file-descriptor
On Linux, you would
typically open in read-only (O_RDONLY) the directory (O_WRONLY would
fail otherwise), and call fsync() on the returned file descriptor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point — could you add a test so that the feature is exercised in the CI pipeline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for letting this PR linger for so long. It’s been a busy time recently.
The reason why the renameio package currently doesn’t provide this feature is so that the user has more control over when a sync happens (as applications often write many files), but I don’t mind adding this for use-cases where only a single file needs to be written reliably.
@@ -167,6 +168,16 @@ func (t *PendingFile) CloseAtomicallyReplace() error { | |||
return err | |||
} | |||
t.done = true | |||
if t.syncDirectory { | |||
dh, err := os.OpenFile(filepath.Dir(t.path), os.O_WRONLY, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point — could you add a test so that the feature is exercised in the CI pipeline?
WithSyncDirectory configures renameio to fsync the directory after renaming
the file. If that succeeds it's guaranteed the file will be in the new state
even after a crash.