Improve AtomicFileWriter's support for Win and Mac

Add ExplicitCommit option to provide calling code the ability to
preserve original files when an application error occurs, not just
an error that occurs during writing.

Change Close() impl to safely handle redundant calls (for defer
usage safety)

Signed-off-by: Jason T. Greene <[email protected]>

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