diff --git a/pkg/ioutils/fswriters.go b/pkg/ioutils/fswriters.go index 231d1c47b2..2a8c85ad44 100644 --- a/pkg/ioutils/fswriters.go +++ b/pkg/ioutils/fswriters.go @@ -17,6 +17,20 @@ type AtomicFileWriterOptions struct { // On successful return from Close() this is set to the mtime of the // newly written file. ModTime time.Time + // Specifies whether Commit() must be explicitly called to write state + // to the destination. This allows an application to preserve the original + // file when an error occurs during processing (and not just during write) + // The default is false, which will auto-commit on Close + ExplicitCommit bool +} + +type CommittableWriter interface { + io.WriteCloser + + // Commit closes the temporary file associated with this writer, and + // provided no errors (during commit or previously during write operations), + // will publish the completed file under the intended destination. + Commit() error } var defaultWriterOptions = AtomicFileWriterOptions{} @@ -27,16 +41,19 @@ func SetDefaultOptions(opts AtomicFileWriterOptions) { defaultWriterOptions = opts } -// NewAtomicFileWriterWithOpts returns WriteCloser so that writing to it writes to a -// temporary file and closing it atomically changes the temporary file to -// destination path. Writing and closing concurrently is not allowed. -func NewAtomicFileWriterWithOpts(filename string, perm os.FileMode, opts *AtomicFileWriterOptions) (io.WriteCloser, error) { +// NewAtomicFileWriterWithOpts returns a CommittableWriter so that writing to it +// writes to a temporary file, which can later be committed to a destination path, +// either by Closing in the case of auto-commit, or manually calling commit if the +// ExplicitCommit option is enabled. Writing and closing concurrently is not +// allowed. +func NewAtomicFileWriterWithOpts(filename string, perm os.FileMode, opts *AtomicFileWriterOptions) (CommittableWriter, error) { return newAtomicFileWriter(filename, perm, opts) } -// newAtomicFileWriter returns WriteCloser so that writing to it writes to a -// temporary file and closing it atomically changes the temporary file to -// destination path. Writing and closing concurrently is not allowed. +// newAtomicFileWriter returns a CommittableWriter so that writing to it writes to +// a temporary file, which can later be committed to a destination path, either by +// Closing in the case of auto-commit, or manually calling commit if the +// ExplicitCommit option is enabled. Writing and closing concurrently is not allowed. func newAtomicFileWriter(filename string, perm os.FileMode, opts *AtomicFileWriterOptions) (*atomicFileWriter, error) { f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) if err != nil { @@ -50,17 +67,18 @@ func newAtomicFileWriter(filename string, perm os.FileMode, opts *AtomicFileWrit return nil, err } return &atomicFileWriter{ - f: f, - fn: abspath, - perm: perm, - noSync: opts.NoSync, + f: f, + fn: abspath, + perm: perm, + noSync: opts.NoSync, + explicitCommit: opts.ExplicitCommit, }, nil } -// NewAtomicFileWriter returns WriteCloser so that writing to it writes to a -// temporary file and closing it atomically changes the temporary file to -// destination path. Writing and closing concurrently is not allowed. -func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, error) { +// NewAtomicFileWriterWithOpts returns a CommittableWriter, with auto-commit enabled. +// Writing to it writes to a temporary file and closing it atomically changes the +// temporary file to destination path. Writing and closing concurrently is not allowed. +func NewAtomicFileWriter(filename string, perm os.FileMode) (CommittableWriter, error) { return NewAtomicFileWriterWithOpts(filename, perm, nil) } @@ -91,12 +109,14 @@ func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { } type atomicFileWriter struct { - f *os.File - fn string - writeErr error - perm os.FileMode - noSync bool - modTime time.Time + f *os.File + fn string + writeErr error + perm os.FileMode + noSync bool + modTime time.Time + closed bool + explicitCommit bool } func (w *atomicFileWriter) Write(dt []byte) (int, error) { @@ -107,43 +127,73 @@ func (w *atomicFileWriter) Write(dt []byte) (int, error) { return n, err } -func (w *atomicFileWriter) Close() (retErr error) { +func (w *atomicFileWriter) closeTempFile() error { + if w.closed { + return nil + } + + w.closed = true + return w.f.Close() +} + +func (w *atomicFileWriter) Close() error { + return w.complete(!w.explicitCommit) +} + +func (w *atomicFileWriter) Commit() error { + return w.complete(true) +} + +func (w *atomicFileWriter) complete(commit bool) (retErr error) { + if w == nil || w.closed { + return nil + } + defer func() { + w.closeTempFile() if retErr != nil || w.writeErr != nil { os.Remove(w.f.Name()) } }() - if !w.noSync { - if err := fdatasync(w.f); err != nil { - w.f.Close() - return err - } + + if commit { + return w.commitState() } - // fstat before closing the fd - info, statErr := w.f.Stat() - if statErr == nil { - w.modTime = info.ModTime() + return nil +} + +func (w *atomicFileWriter) commitState() error { + // Perform a data only sync (fdatasync()) if supported + if err := w.postDataWrittenSync(); err != nil { + return err } - // We delay error reporting until after the real call to close() - // to match the traditional linux close() behaviour that an fd - // is invalid (closed) even if close returns failure. While - // weird, this allows a well defined way to not leak open fds. - if err := w.f.Close(); err != nil { + // Capture fstat before closing the fd + info, err := w.f.Stat() + if err != nil { return err } + w.modTime = info.ModTime() - if statErr != nil { - return statErr + if err := w.f.Chmod(w.perm); err != nil { + return err + } + + // Perform full sync on platforms that need it + if err := w.preRenameSync(); err != nil { + return err } - if err := os.Chmod(w.f.Name(), w.perm); err != nil { + // Some platforms require closing before rename (Windows) + if err := w.closeTempFile(); err != nil { return err } + if w.writeErr == nil { return os.Rename(w.f.Name(), w.fn) } + return nil } @@ -195,7 +245,7 @@ func (w syncFileCloser) Close() error { if !defaultWriterOptions.NoSync { return w.File.Close() } - err := fdatasync(w.File) + err := dataOrFullSync(w.File) if err1 := w.File.Close(); err == nil { err = err1 } diff --git a/pkg/ioutils/fswriters_linux.go b/pkg/ioutils/fswriters_linux.go index 0da78a063d..10ed48cfd8 100644 --- a/pkg/ioutils/fswriters_linux.go +++ b/pkg/ioutils/fswriters_linux.go @@ -6,6 +6,18 @@ import ( "golang.org/x/sys/unix" ) -func fdatasync(f *os.File) error { +func dataOrFullSync(f *os.File) error { return unix.Fdatasync(int(f.Fd())) } + +func (w *atomicFileWriter) postDataWrittenSync() error { + if w.noSync { + return nil + } + return unix.Fdatasync(int(w.f.Fd())) +} + +func (w *atomicFileWriter) preRenameSync() error { + // On Linux data can be reliably flushed to media without metadata, so defer + return nil +} diff --git a/pkg/ioutils/fswriters_other.go b/pkg/ioutils/fswriters_other.go new file mode 100644 index 0000000000..aec161e0f2 --- /dev/null +++ b/pkg/ioutils/fswriters_other.go @@ -0,0 +1,26 @@ +//go:build !linux +// +build !linux + +package ioutils + +import ( + "os" +) + +func dataOrFullSync(f *os.File) error { + return f.Sync() +} + +func (w *atomicFileWriter) postDataWrittenSync() error { + // many platforms (Mac, Windows) require a full sync to reliably flush to media + return nil +} + +func (w *atomicFileWriter) preRenameSync() error { + if w.noSync { + return nil + } + + // fsync() on Non-linux Unix, FlushFileBuffers (Windows), F_FULLFSYNC (Mac) + return w.f.Sync() +} diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index b1466ffa3c..c96800b8af 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -45,6 +45,56 @@ func TestAtomicWriteToFile(t *testing.T) { } } +func TestAtomicCommitAndRollbackFile(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "foo") + + oldData := "olddata" + newData := "newdata" + + check := func(n int, initData string, writeData string, expected string, explicit bool, commit bool) { + if err := os.WriteFile(path, []byte(initData), 0644); err != nil { + t.Fatalf("Failed creating initial file: %v", err) + } + + opts := &AtomicFileWriterOptions{ExplicitCommit: explicit} + w, err := NewAtomicFileWriterWithOpts(filepath.Join(tmpDir, "foo"), 0644, opts) + + if err != nil { + t.Fatalf("(%d) Failed creating writer: %v", n, err) + } + + if _, err := w.Write([]byte(writeData)); err != nil { + t.Fatalf("(%d) Failed writing content: %v", n, err) + } + + if commit { + if err := w.Commit(); err != nil { + t.Fatalf("(%d) Failed committing writer: %v", n, err) + } + } + + if err := w.Close(); err != nil { + t.Fatalf("(%d) Failed closing writer: %v", n, err) + } + + actual, err := os.ReadFile(filepath.Join(tmpDir, "foo")) + if err != nil { + t.Fatalf("(%d) Error reading from file: %v", n, err) + } + + // Verify write never happened since no call to commit + if !bytes.Equal(actual, []byte(expected)) { + t.Fatalf("(%d) Data mismatch, expected %q, got %q", n, expected, actual) + } + } + + check(1, oldData, newData, oldData, true, false) + check(2, oldData, newData, newData, true, true) + check(3, oldData, newData, newData, false, false) + check(4, oldData, newData, newData, false, true) +} + func TestAtomicWriteSetCommit(t *testing.T) { tmpDir := t.TempDir() diff --git a/pkg/ioutils/fswriters_unsupported.go b/pkg/ioutils/fswriters_unsupported.go deleted file mode 100644 index 635489280d..0000000000 --- a/pkg/ioutils/fswriters_unsupported.go +++ /dev/null @@ -1,12 +0,0 @@ -//go:build !linux -// +build !linux - -package ioutils - -import ( - "os" -) - -func fdatasync(f *os.File) error { - return f.Sync() -}