pkg/safe: remove temporary file on failure (#1774)

object-utils.go

@@ -167,7 +167,7 @@ func safeCloseAndRemove(writer io.WriteCloser) error {
 	// If writer is a safe file, Attempt to close and remove.
 	safeWriter, ok := writer.(*safe.File)
 	if ok {
-		return safeWriter.CloseAndRemove()
+		return safeWriter.Abort()
 	}
 	wCloser, ok := writer.(*waitCloser)
 	if ok {

pkg/safe/safe.go

@@ -16,115 +16,109 @@
 
 // NOTE - Rename() not guaranteed to be safe on all filesystems which are not fully POSIX compatible
 
-// Package safe provides safe file write semantics by leveraging Rename's() safeity.
 package safe
 
 import (
-	"io"
+	"errors"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 )
 
-// Vault - vault is an interface for different implementations of safe
+// File represents safe file descriptor.
-// i/o semantics.
-type Vault interface {
-	io.ReadWriteCloser
-	SyncClose() error
-	CloseAndRemove() error
-}
-
-// File provides for safe file writes.
 type File struct {
-	*os.File
+	name    string
-	file string
+	tmpfile *os.File
+	closed  bool
+	aborted bool
 }
 
-// SyncClose sync file to disk and close, returns an error if any
+// Write writes len(b) bytes to the temporary File.  In case of error, the temporary file is removed.
-func (f *File) SyncClose() error {
+func (file *File) Write(b []byte) (n int, err error) {
-	// sync to the disk
+	if file.aborted {
-	if err := f.File.Sync(); err != nil {
+		err = errors.New("write on aborted file")
-		return err
+		return
 	}
-	// Close the fd.
+	if file.closed {
-	if err := f.Close(); err != nil {
+		err = errors.New("write on closed file")
-		return err
+		return
 	}
-	return nil
+
+	defer func() {
+		if err != nil {
+			os.Remove(file.tmpfile.Name())
+			file.aborted = true
+		}
+	}()
+
+	n, err = file.tmpfile.Write(b)
+	return
 }
 
-// Close the file, returns an error if any
+// Close closes the temporary File and renames to the named file.  In case of error, the temporary file is removed.
-func (f *File) Close() error {
+func (file *File) Close() (err error) {
-	// Close the embedded fd.
+	defer func() {
-	if err := f.File.Close(); err != nil {
+		if err != nil {
-		return err
+			os.Remove(file.tmpfile.Name())
+			file.aborted = true
+		}
+	}()
+
+	if file.aborted || file.closed {
+		return
 	}
-	// Safe rename to final destination
+
-	if err := os.Rename(f.Name(), f.file); err != nil {
+	if err = file.tmpfile.Close(); err != nil {
-		return err
+		return
 	}
-	return nil
+
+	err = os.Rename(file.tmpfile.Name(), file.name)
+
+	file.closed = true
+	return
 }
 
-// CloseAndRemove closes the temp file, and safely removes it. Returns
+// Abort aborts the temporary File by closing and removing the temporary file.
-// error if any.
+func (file *File) Abort() (err error) {
-func (f *File) CloseAndRemove() error {
+	if file.aborted || file.closed {
-	// close the embedded fd
+		return
-	f.File.Close()
-
-	// Remove the temp file.
-	if err := os.Remove(f.Name()); err != nil {
-		return err
 	}
-	return nil
+
+	file.tmpfile.Close()
+	err = os.Remove(file.tmpfile.Name())
+	file.aborted = true
+	return
 }
 
-// CreateFile creates a new file at filePath for safe writes, it also
+// CreateFile creates the named file safely from unique temporary file.
-// creates parent directories if they don't exist.
+// The temporary file is renamed to the named file upon successful close
-func CreateFile(filePath string) (*File, error) {
+// to safeguard intermediate state in the named file.  The temporary file
-	return CreateFileWithPrefix(filePath, "$deleteme.")
+// is created in the name of the named file with suffixed unique number
-}
+// and prefixed "$tmpfile" string.  While creating the temporary file,
-
+// missing parent directories are also created.  The temporary file is
-// CreateFileWithSuffix is similar to CreateFileWithPrefix, but the
+// removed if case of any intermediate failure.  Not removed temporary
-// second argument is treated as suffix for the temporary files.
+// files can be cleaned up by identifying them using "$tmpfile" prefix
-func CreateFileWithSuffix(filePath string, suffix string) (*File, error) {
+// string.
-	// If parent directories do not exist, ioutil.TempFile doesn't create them
+func CreateFile(name string) (*File, error) {
-	// handle such a case with os.MkdirAll()
+	// ioutil.TempFile() fails if parent directory is missing.
-	if err := os.MkdirAll(filepath.Dir(filePath), 0700); err != nil {
+	// Create parent directory to avoid such error.
+	dname := filepath.Dir(name)
+	if err := os.MkdirAll(dname, 0700); err != nil {
 		return nil, err
 	}
-	f, err := ioutil.TempFile(filepath.Dir(filePath), filepath.Base(filePath)+suffix)
+
+	fname := filepath.Base(name)
+	tmpfile, err := ioutil.TempFile(dname, "$tmpfile."+fname+".")
 	if err != nil {
 		return nil, err
 	}
-	if err = os.Chmod(f.Name(), 0600); err != nil {
-		if err = os.Remove(f.Name()); err != nil {
-			return nil, err
-		}
-		return nil, err
-	}
-	return &File{File: f, file: filePath}, nil
-}
 
-// CreateFileWithPrefix creates a new file at filePath for safe
+	if err = os.Chmod(tmpfile.Name(), 0600); err != nil {
-// writes, it also creates parent directories if they don't exist.
+		if rerr := os.Remove(tmpfile.Name()); rerr != nil {
-// prefix specifies the prefix of the temporary files so that cleaning
+			err = rerr
-// stale temp files is easy.
-func CreateFileWithPrefix(filePath string, prefix string) (*File, error) {
-	// If parent directories do not exist, ioutil.TempFile doesn't create them
-	// handle such a case with os.MkdirAll()
-	if err := os.MkdirAll(filepath.Dir(filePath), 0700); err != nil {
-		return nil, err
-	}
-	f, err := ioutil.TempFile(filepath.Dir(filePath), prefix+filepath.Base(filePath))
-	if err != nil {
-		return nil, err
-	}
-	if err = os.Chmod(f.Name(), 0600); err != nil {
-		if err = os.Remove(f.Name()); err != nil {
-			return nil, err
 		}
 		return nil, err
 	}
-	return &File{File: f, file: filePath}, nil
+
+	return &File{name: name, tmpfile: tmpfile}, nil
 }

pkg/safe/safe_test.go

@@ -34,13 +34,14 @@ type MySuite struct {
 var _ = Suite(&MySuite{})
 
 func (s *MySuite) SetUpSuite(c *C) {
-	root, err := ioutil.TempDir(os.TempDir(), "safe-")
+	root, err := ioutil.TempDir(os.TempDir(), "safe_test.go.")
 	c.Assert(err, IsNil)
 	s.root = root
 }
 
 func (s *MySuite) TearDownSuite(c *C) {
-	os.RemoveAll(s.root)
+	err := os.Remove(s.root)
+	c.Assert(err, IsNil)
 }
 
 func (s *MySuite) TestSafe(c *C) {
@@ -52,15 +53,17 @@ func (s *MySuite) TestSafe(c *C) {
 	c.Assert(err, IsNil)
 	_, err = os.Stat(filepath.Join(s.root, "testfile"))
 	c.Assert(err, IsNil)
+	err = os.Remove(filepath.Join(s.root, "testfile"))
+	c.Assert(err, IsNil)
 }
 
-func (s *MySuite) TestSafeRemove(c *C) {
+func (s *MySuite) TestSafeAbort(c *C) {
 	f, err := CreateFile(filepath.Join(s.root, "purgefile"))
 	c.Assert(err, IsNil)
 	_, err = os.Stat(filepath.Join(s.root, "purgefile"))
 	c.Assert(err, Not(IsNil))
-	err = f.CloseAndRemove()
+	err = f.Abort()
 	c.Assert(err, IsNil)
-	err = f.Close()
+	_, err = os.Stat(filepath.Join(s.root, "purgefile"))
 	c.Assert(err, Not(IsNil))
 }

posix.go

@@ -358,7 +358,7 @@ func (s fsStorage) CreateFile(volume, path string) (writeCloser io.WriteCloser,
 			return nil, errIsNotRegular
 		}
 	}
-	w, err := safe.CreateFileWithPrefix(filePath, "$tmpfile")
+	w, err := safe.CreateFile(filePath)
 	if err != nil {
 		// File path cannot be verified since one of the parents is a file.
 		if strings.Contains(err.Error(), "not a directory") {