From d29940f63d9d44d8b2c8ae6cbb18bb788a4c6f8f Mon Sep 17 00:00:00 2001 From: Corentin Debains Date: Thu, 24 Aug 2017 18:55:54 -0700 Subject: [PATCH] mem/file.go - Fix some races in accessing fields of FileData * Splitting SetModeTime to avoid double locking * Adding locks all over the place. --- .travis.yml | 2 +- appveyor.yml | 2 +- mem/file.go | 40 +++++++++--- mem/file_test.go | 154 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 9 deletions(-) create mode 100644 mem/file_test.go diff --git a/.travis.yml b/.travis.yml index 6c296d2..40d0789 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,5 +16,5 @@ matrix: fast_finish: true script: - - go test -v ./... + - go test -race -v ./... - go build diff --git a/appveyor.yml b/appveyor.yml index 006f315..98ac35c 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -12,4 +12,4 @@ build_script: go build github.com/spf13/afero test_script: -- cmd: go test -v github.com/spf13/afero +- cmd: go test --race -v github.com/spf13/afero diff --git a/mem/file.go b/mem/file.go index e41e012..5401a3b 100644 --- a/mem/file.go +++ b/mem/file.go @@ -74,14 +74,24 @@ func CreateDir(name string) *FileData { } func ChangeFileName(f *FileData, newname string) { + f.Lock() f.name = newname + f.Unlock() } func SetMode(f *FileData, mode os.FileMode) { + f.Lock() f.mode = mode + f.Unlock() } func SetModTime(f *FileData, mtime time.Time) { + f.Lock() + setModTime(f, mtime) + f.Unlock() +} + +func setModTime(f *FileData, mtime time.Time) { f.modtime = mtime } @@ -102,7 +112,7 @@ func (f *File) Close() error { f.fileData.Lock() f.closed = true if !f.readOnly { - SetModTime(f.fileData, time.Now()) + setModTime(f.fileData, time.Now()) } f.fileData.Unlock() return nil @@ -197,7 +207,7 @@ func (f *File) Truncate(size int64) error { } else { f.fileData.data = f.fileData.data[0:size] } - SetModTime(f.fileData, time.Now()) + setModTime(f.fileData, time.Now()) return nil } @@ -236,7 +246,7 @@ func (f *File) Write(b []byte) (n int, err error) { f.fileData.data = append(f.fileData.data[:cur], b...) f.fileData.data = append(f.fileData.data, tail...) } - SetModTime(f.fileData, time.Now()) + setModTime(f.fileData, time.Now()) atomic.StoreInt64(&f.at, int64(len(f.fileData.data))) return @@ -261,17 +271,33 @@ type FileInfo struct { // Implements os.FileInfo func (s *FileInfo) Name() string { + s.Lock() _, name := filepath.Split(s.name) + s.Unlock() return name } -func (s *FileInfo) Mode() os.FileMode { return s.mode } -func (s *FileInfo) ModTime() time.Time { return s.modtime } -func (s *FileInfo) IsDir() bool { return s.dir } -func (s *FileInfo) Sys() interface{} { return nil } +func (s *FileInfo) Mode() os.FileMode { + s.Lock() + defer s.Unlock() + return s.mode +} +func (s *FileInfo) ModTime() time.Time { + s.Lock() + defer s.Unlock() + return s.modtime +} +func (s *FileInfo) IsDir() bool { + s.Lock() + defer s.Unlock() + return s.dir +} +func (s *FileInfo) Sys() interface{} { return nil } func (s *FileInfo) Size() int64 { if s.IsDir() { return int64(42) } + s.Lock() + defer s.Unlock() return int64(len(s.data)) } diff --git a/mem/file_test.go b/mem/file_test.go new file mode 100644 index 0000000..1aeea23 --- /dev/null +++ b/mem/file_test.go @@ -0,0 +1,154 @@ +package mem + +import ( + "testing" + "time" +) + +func Test_FileData_Name_withConcurrence(t *testing.T) { + t.Parallel() + const someName = "someName" + const someOtherName = "someOtherName" + d := FileData{ + name: someName, + } + + if d.Name() != someName { + t.Errorf("Failed to read correct Name, was %v", d.Name()) + } + + ChangeFileName(&d, someOtherName) + if d.Name() != someOtherName { + t.Errorf("Failed to set Name, was %v", d.Name()) + } + + go func() { + ChangeFileName(&d, someName) + }() + + if d.Name() != someName && d.Name() != someOtherName { + t.Errorf("Failed to read either Name, was %v", d.Name()) + } +} + +func Test_FileData_ModTime_withConcurrence(t *testing.T) { + t.Parallel() + someTime := time.Now() + someOtherTime := someTime.Add(1 * time.Minute) + + d := FileData{ + modtime: someTime, + } + + s := FileInfo{ + FileData: &d, + } + + if s.ModTime() != someTime { + t.Errorf("Failed to read correct value, was %v", s.ModTime()) + } + + SetModTime(&d, someOtherTime) + if s.ModTime() != someOtherTime { + t.Errorf("Failed to set ModTime, was %v", s.ModTime()) + } + + go func() { + SetModTime(&d, someTime) + }() + + if s.ModTime() != someTime && s.ModTime() != someOtherTime { + t.Errorf("Failed to read either modtime, was %v", s.ModTime()) + } +} + +func Test_FileData_Mode_withConcurrence(t *testing.T) { + t.Parallel() + const someMode = 0777 + const someOtherMode = 0660 + + d := FileData{ + mode: someMode, + } + + s := FileInfo{ + FileData: &d, + } + + if s.Mode() != someMode { + t.Errorf("Failed to read correct value, was %v", s.Mode()) + } + + SetMode(&d, someOtherMode) + if s.Mode() != someOtherMode { + t.Errorf("Failed to set Mode, was %v", s.Mode()) + } + + go func() { + SetMode(&d, someMode) + }() + + if s.Mode() != someMode && s.Mode() != someOtherMode { + t.Errorf("Failed to read either mode, was %v", s.Mode()) + } +} + +func Test_FileData_IsDir_withConcurrence(t *testing.T) { + t.Parallel() + + d := FileData{ + dir: true, + } + + s := FileInfo{ + FileData: &d, + } + + if s.IsDir() != true { + t.Errorf("Failed to read correct value, was %v", s.IsDir()) + } + + go func() { + s.Lock() + d.dir = false + s.Unlock() + }() + + //just logging the value to trigger a read: + t.Logf("Value is %v", s.IsDir()) +} + +func Test_FileData_Size_withConcurrence(t *testing.T) { + t.Parallel() + + const someData = "Hello" + const someOtherDataSize = "Hello World" + + d := FileData{ + data: []byte(someData), + dir: false, + } + + s := FileInfo{ + FileData: &d, + } + + if s.Size() != int64(len(someData)) { + t.Errorf("Failed to read correct value, was %v", s.Size()) + } + + go func() { + s.Lock() + d.data = []byte(someOtherDataSize) + s.Unlock() + }() + + //just logging the value to trigger a read: + t.Logf("Value is %v", s.Size()) + + //Testing the Dir size case + d.dir = true + if s.Size() != int64(42) { + t.Errorf("Failed to read correct value for dir, was %v", s.Size()) + } +}