From 6226bdba888d9138a48ba6adcbdaaf0ee33b7fff Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 10 Aug 2017 13:28:05 -0700 Subject: [PATCH 1/2] Create a new "TempDir" option for atomic write The current atomic write is somewhat broken as the temporary files are created inside the base directory, and so count as potential keys (and can conflict). Move these temporary files in the TempDir specified by the user so that they can't conflict with future keys. Note that the TempDir must be on the same device/partition/mount or the atomic Rename will fail across partition. Atomic write will not happen if TempDir is left empty. --- basic_test.go | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++- diskv.go | 50 +++++++++++++++++++++++++++++++++---------- 2 files changed, 97 insertions(+), 12 deletions(-) diff --git a/basic_test.go b/basic_test.go index bd6e7c2..0ef0b17 100644 --- a/basic_test.go +++ b/basic_test.go @@ -259,7 +259,7 @@ func (BrokenReader) Read(p []byte) (n int, err error) { return 0, errors.New("failed to read") } -func TestAtomicWrite(t *testing.T) { +func TestRemovesIncompleteFiles(t *testing.T) { opts := Options{ BasePath: "test-data", CacheSizeMax: 1024, @@ -277,3 +277,60 @@ func TestAtomicWrite(t *testing.T) { t.Fatal("Could read the key, but it shouldn't exist") } } + +func TestTempDir(t *testing.T) { + opts := Options{ + BasePath: "test-data", + TempDir: "test-data-temp", + CacheSizeMax: 1024, + } + d := New(opts) + defer d.EraseAll() + + k, v := "a", []byte{'b'} + if err := d.Write(k, v); err != nil { + t.Fatalf("write: %s", err) + } + if readVal, err := d.Read(k); err != nil { + t.Fatalf("read: %s", err) + } else if bytes.Compare(v, readVal) != 0 { + t.Fatalf("read: expected %s, got %s", v, readVal) + } + if err := d.Erase(k); err != nil { + t.Fatalf("erase: %s", err) + } +} + +type CrashingReader struct{} + +func (CrashingReader) Read(p []byte) (n int, err error) { + panic("System has crashed while reading the stream") +} + +func TestAtomicWrite(t *testing.T) { + opts := Options{ + BasePath: "test-data", + // Test would fail if TempDir is not set here. + TempDir: "test-data-temp", + CacheSizeMax: 1024, + } + d := New(opts) + defer d.EraseAll() + + key := "key" + func() { + defer func() { + recover() // Ignore panicking error + }() + + stream := CrashingReader{} + d.WriteStream(key, stream, false) + }() + + if d.Has(key) { + t.Fatal("Has key, but it shouldn't exist") + } + if _, ok := <-d.Keys(nil); ok { + t.Fatal("Store isn't empty") + } +} diff --git a/diskv.go b/diskv.go index 64611e6..2fa0b5a 100644 --- a/diskv.go +++ b/diskv.go @@ -46,6 +46,7 @@ type Options struct { CacheSizeMax uint64 // bytes PathPerm os.FileMode FilePerm os.FileMode + TempDir string Index Index IndexLess LessFunction @@ -115,21 +116,43 @@ func (d *Diskv) WriteStream(key string, r io.Reader, sync bool) error { return d.writeStreamWithLock(key, r, sync) } +// createKeyFileWithLock either creates the key file directly, or +// creates a temporary file in TempDir if it is set. +func (d *Diskv) createKeyFileWithLock(key string) (*os.File, error) { + if d.TempDir != "" { + if err := os.MkdirAll(d.TempDir, d.PathPerm); err != nil { + return nil, fmt.Errorf("temp mkdir: %s", err) + } + f, err := ioutil.TempFile(d.TempDir, "") + if err != nil { + return nil, fmt.Errorf("temp file: %s", err) + } + + if err := f.Chmod(d.FilePerm); err != nil { + f.Close() // error deliberately ignored + os.Remove(f.Name()) // error deliberately ignored + return nil, fmt.Errorf("chmod: %s", err) + } + return f, nil + } + + mode := os.O_WRONLY | os.O_CREATE | os.O_TRUNC // overwrite if exists + f, err := os.OpenFile(d.completeFilename(key), mode, d.FilePerm) + if err != nil { + return nil, fmt.Errorf("open file: %s", err) + } + return f, nil +} + // writeStream does no input validation checking. func (d *Diskv) writeStreamWithLock(key string, r io.Reader, sync bool) error { if err := d.ensurePathWithLock(key); err != nil { return fmt.Errorf("ensure path: %s", err) } - f, err := ioutil.TempFile(d.pathFor(key), fmt.Sprintf("temp-%s-", key)) + f, err := d.createKeyFileWithLock(key) if err != nil { - return fmt.Errorf("temp file: %s", err) - } - - if err := f.Chmod(d.FilePerm); err != nil { - f.Close() // error deliberately ignored - os.Remove(f.Name()) // error deliberately ignored - return fmt.Errorf("chmod: %s", err) + return fmt.Errorf("create key file: %s", err) } wc := io.WriteCloser(&nopWriteCloser{f}) @@ -166,9 +189,11 @@ func (d *Diskv) writeStreamWithLock(key string, r io.Reader, sync bool) error { return fmt.Errorf("file close: %s", err) } - if err := os.Rename(f.Name(), d.completeFilename(key)); err != nil { - os.Remove(f.Name()) // error deliberately ignored - return fmt.Errorf("rename: %s", err) + if f.Name() != d.completeFilename(key) { + if err := os.Rename(f.Name(), d.completeFilename(key)); err != nil { + os.Remove(f.Name()) // error deliberately ignored + return fmt.Errorf("rename: %s", err) + } } if d.Index != nil { @@ -403,6 +428,9 @@ func (d *Diskv) EraseAll() error { defer d.mu.Unlock() d.cache = make(map[string][]byte) d.cacheSize = 0 + if d.TempDir != "" { + os.RemoveAll(d.TempDir) // errors ignored + } return os.RemoveAll(d.BasePath) } From 40ecb11adeba25b7c2776480c729287664ab5ef3 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Mon, 14 Aug 2017 10:29:00 -0700 Subject: [PATCH 2/2] Document TempDir parameter --- diskv.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/diskv.go b/diskv.go index 2fa0b5a..524dc0a 100644 --- a/diskv.go +++ b/diskv.go @@ -46,7 +46,12 @@ type Options struct { CacheSizeMax uint64 // bytes PathPerm os.FileMode FilePerm os.FileMode - TempDir string + // If TempDir is set, it will enable filesystem atomic writes by + // writing temporary files to that location before being moved + // to BasePath. + // Note that TempDir MUST be on the same device/partition as + // BasePath. + TempDir string Index Index IndexLess LessFunction