Fluent interface for atomic file writes

[twz123]

Description

This allows the atomic write functions to be extended in the future without breaking the existing API. Introduces the AtomicOpener struct, which implements the fluent interface and collects all the desired information for an atomic file write. It provides some terminal methods that do the actual writing.

The old workhorse function WriteAtomically has been splitted up in AtomicOpener.Do, AtomicOpener.Open and its companion struct Atomic, especially its Finish and Close methods. Atomic implements io.WriteCloser and io.ReaderFrom and may be used more or less like a usual write-only io.File. The magic happens when it's finished/closed. The write callback of AtomicOpener.Do now accepts the purpose-built interface file.AtomicWriter. This allows the interface to be extended with new methods without having to change all call sites. Setting permissions is now optional, i.e. callers have to opt in by calling WithPermissions(...).

Also address some other shortcomings:

• The temporary file was properly closed on panic, but not removed from disk. Add an additional boolean that indicates whether the write call returned. Use this boolean as an additional criterion in the defer func to find out if the file needs to be removed or not.
• Atomic file writes could get confused when used with non-absolute paths, and the working directory would change while writing files. This can be easily mitigated by using the absolute path internally.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

How Has This Been Tested?

• Manual test
• Auto test added

Checklist:

• My code follows the style guidelines of this project
• My commit messages are signed-off
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[kke]

Looks nice. os.Rename isn't atomic on windows btw.

I saw some implementation that did the replacing on Close() and otherwise it looked like any regular os.File/io.WriteCloser you get from os.OpenFile.

Something like:

import (
	"errors"
	"fmt"
	"io"
	"io/fs"
	"os"
	"path/filepath"
	"strconv"
	"time"
)

var errEmptyWrite = errors.New("atomicwriter: wrote no data")

type atomicWriter struct {
	target string
	w      *os.File
	wrErr  error
	closed bool
	n      int
}

// OpenAtomic opens a file for writing, creating a temporary file in the same directory as the target file which is
// used to write the contents. When the file is closed, the temporary file is renamed to the target file, ensuring
// that the target file is only updated if the write completes successfully. If the write fails, the temporary file
// is removed. The caller is expected to call Close on the returned atomicWriter when done writing. The writer is
// not concurrency safe and should not be used concurrently.
func OpenAtomic(name string, mode fs.FileMode) (*atomicWriter, error) {
        name = filepath.Abs(name)
	tmp := filepath.Join(filepath.Dir(name), "."+filepath.Base(name)+"."+strconv.Itoa(int(time.Now().UnixNano()))+".tmp")
	f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode)
	if err != nil {
		return nil, fmt.Errorf("atomicwriter: open: %w", err)
	}
	return &atomicWriter{target: name, w: f}, nil
}

// Write the data to the temporary file. If the write fails, the temporary file is removed.
// The number of bytes written is returned and any error encountered.
func (a *atomicWriter) Write(p []byte) (n int, err error) {
	if a.closed {
		return 0, fmt.Errorf("atomicwriter: close: %w", fs.ErrClosed)
	}
	n, err = a.w.Write(p)
	if err != nil && !errors.Is(err, io.EOF) {
		// Write failed abnormally, clean up.
		a.cleanup()
		a.wrErr = err
		return n, fmt.Errorf("atomicwriter: write: %w", err)
	}
	a.n += n
	return n, err
}

func (a *atomicWriter) cleanup() {
	if !a.closed {
		_ = a.w.Close()
		a.closed = true
	}
	_ = os.Remove(a.w.Name())
}

// Close the file and rename the temporary file to the target file if the write was successful.
func (a *atomicWriter) Close() error {
	if a.wrErr != nil {
		return fmt.Errorf("atomicwriter: close: write error occured: %w", a.wrErr)
	}
	if a.closed {
		return fmt.Errorf("atomicwriter: close: %w", fs.ErrClosed)
	}
	if err := a.w.Close(); err != nil {
		// Close failed, clean up.
		a.cleanup()
		return fmt.Errorf("atomicwriter: close: %w", err)
	}
	a.closed = true
	if a.n <= 0 {
		// Nothing written, expect the write block to have failed and the file to be invalid, clean up.
		a.cleanup()
		return errEmptyWrite
	}
	if err := os.Rename(a.w.Name(), a.target); err != nil {
		// Replacing the target failed, clean up.
		a.cleanup()
		return fmt.Errorf("atomicwriter: close: %w", err)
	}
	return nil
}

[twz123]

os.Rename isn't atomic on windows btw.

Indeed, as mentioned in the godocs:

// Note that this function is only best-effort on Windows:
// https://github.com/golang/go/issues/22397#issuecomment-498856679

I saw some implementation that did the replacing on Close() and otherwise it looked like any regular os.File/io.WriteCloser you get from os.OpenFile.

I've briefly thought about that as well, but felt that the callback approach is easier to reason about, and also has less possibilities to be misused, like double-close, usage from multiple goroutines and such things. Anyhow, I'll give it a spin and then we can decide by comparing both approaches.

[twz123]

I saw some implementation that did the replacing on Close() and otherwise it looked like any regular os.File/io.WriteCloser you get from os.OpenFile.

I've briefly thought about that as well, but felt that the callback approach is easier to reason about, and also has less possibilities to be misused, like double-close, usage from multiple goroutines and such things. Anyhow, I'll give it a spin and then we can decide by comparing both approaches.

One thing that I overlooked here: With the "return a writer and rename the file on close" approach, we'd need to wrap the write calls and track if there was an error so that we know if we should actually replace the target file when Close() is called. This is actually a bit cumbersome.

[kke]

and also has less possibilities to be misused, like double-close, usage from multiple goroutines and such things

The caller is usually responsible for calling Close() (and/or Flush()) for any file operations and is expected to only do it once and this is in internal/. Multiple go routines concurrently accessing a single file handle or io.Reader/Writer is not done casually.

we'd need to wrap the write calls and track if there was an error so that we know if we should actually replace the target file

Add a closed bool to the struct and check that on Write and Close. The potential for panic on double closing is solved as a byproduct. Protecting Write and Close with a mutex would solve multiple go routines accessing concurrently but I think the caller should be responsible for that.

[twz123]

There you go. I'm not too convinced, though. Especially the core logic is not as easy to grasp as it has been before, as it's now split into multiple methods, and the atomicFile.Failed stuff seems brittle.

[kke]

Yes, I meant to suggest dropping the callback and making it a regular writer that just replaces the final target on Close().

The expected usage pattern would be the same as with any file ops:

a, err := OpenAtomic("foo", 0o600)
if err != nil {
  return err
}
defer a.Close()
// do the writing, return error or nil

[twz123]

That pattern doesn't work, because something™ needs to tell the Close method in the defer statement if it's on the happy path (do the rename), or the err path (discard the temp file). But this could work:

a, err := OpenAtomic("foo", 0o600)
if err != nil {
  return err
}
defer a.Close()

// do the writing, return error when hitting problems

return a.Finish() // or a.Rename() .... This is "Close on happy path with rename", rendering Close in the defer into a no-op.

[kke]

This takes an io.Reader which kind of flips the happy-path logic around.

[twz123]

Next try. This is better than the atomicFile.Failed stuff.

dropping the callback and making it a regular writer

I'd really like to keep the callback functions as higher-level abstractions. They're now implemented in terms of Open/Finish/Close and come in quite handy, as they capture the idiomatic usage pattern, which is 9 lines of code. Callers can still use the Atomic directly if they prefer. Removing those would mean that we'd need to refactor all of the current call sites, which I tried to avoid here. We could mark the Write...Atomically functions as deprecated, though, if you prefer.

This takes an io.Reader which kind of flips the happy-path logic around.

Interesting, although that'd be not really useful for e.g. writing templates. Anyhow, I'll have a look. There's always something to learn.

[twz123]

Btw, the claim that MoveFileEx is atomic under Windows is not correct according to

• cmd/go: TestScript/mod_concurrent is flaky on Windows golang/go#32188 (comment)
• proposal: os: add a package to atomically create or replace a file golang/go#22397 (comment)

[twz123]

That said, I'm not sure if having the Atomic with its Open/Finish/Close approach adds any value here. What would be a scenario that'd benefit from having this, instead of only having the callback using an internalized implementation as before? It's conceptually nice to have something that "feels" like a regular io.File, but in the end, the only thing that's really needed is the io.Writer. The Open/Defer/Finish/Close dance is a bit lengthy.

Anyhow, having the fluent interface here is the main point for the PR, whether it's using a callback or returning a file-like struct. So, happy to move forward in either direction.

[kke]

If there's a callback then the user shouldn't have to bother with anything else, I mentioned the replace on Close() just as the pattern for the callbackless version. The callback is ok but maybe just not as "idiomatic" as a regular io.WriteCloser. The io.Reader version is also a bit "weird" for writing files.

[twz123]

Needs a rebase and adaption for #4289.

[github-actions]

This pull request has merge conflicts that need to be resolved.

[github-actions]

This pull request has merge conflicts that need to be resolved.

[kke]

I'm not sure if these errors should be returned, any errors from trying to close or remove a broken fd here are not why the open failed.

Maybe just something like _ = f.fd.Close(); _ = remove(f.fd) before returning the error below.

[twz123]

Maybe the variable name open is not chosen wisely. Perhaps success would be better. In fact, at this point in the code, the file has already been successfully opened, but we want to do something with it, i.e. change the permissions before writing the contents. This defer basically ensures that if subsequent operations fail before we return to the caller, we still clean everything up.

If the file opening itself had failed, it would indeed be useless to close/remove the file.

[twz123]

Maybe that whole chmod business after CreateTemp is futile anyways. Need to check.

[kke]

Looks like it is. https://cs.opensource.google/go/go/+/refs/tags/go1.22.3:src/os/tempfile.go;l=33

...
	for {
		name := prefix + nextRandom() + suffix
		f, err := OpenFile(name, O_RDWR|O_CREATE|O_EXCL, 0600)
...

[twz123]

Killed with 🔥

[kke]

errors.Is should still work :)

[twz123]

This is now gone.