Merge pull request #1554 from n1hility/update-atomicwriter

Improve AtomicFileWriter's support for Win and Mac

Author: rhatdan

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
 	}

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
+}

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()
+}

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()