From 016605ac76be7f9c77207cf24e253127d1ad58dd Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Sun, 30 Jun 2024 23:53:54 +0200 Subject: [PATCH] chore: cleaning and readability --- flock.go | 2 ++ flock_example_test.go | 24 +++++++++++---------- flock_internal_test.go | 4 ++-- flock_test.go | 47 +++++++++++++++--------------------------- flock_unix.go | 38 ++++++++++++++++++++-------------- flock_unix_variants.go | 21 ++++++++++++++++++- flock_winapi.go | 24 ++++++++++----------- flock_windows.go | 3 +++ 8 files changed, 91 insertions(+), 72 deletions(-) diff --git a/flock.go b/flock.go index 987a59f..0ca6216 100644 --- a/flock.go +++ b/flock.go @@ -109,6 +109,7 @@ func (f *Flock) Path() string { func (f *Flock) Locked() bool { f.m.RLock() defer f.m.RUnlock() + return f.l } @@ -118,6 +119,7 @@ func (f *Flock) Locked() bool { func (f *Flock) RLocked() bool { f.m.RLock() defer f.m.RUnlock() + return f.r } diff --git a/flock_example_test.go b/flock_example_test.go index 3a1ed47..cbdcab4 100644 --- a/flock_example_test.go +++ b/flock_example_test.go @@ -9,13 +9,14 @@ import ( "context" "fmt" "os" + "path/filepath" "time" "github.com/gofrs/flock" ) func ExampleFlock_Locked() { - f := flock.New(os.TempDir() + "/go-lock.lock") + f := flock.New(filepath.Join(os.TempDir(), "go-lock.lock")) _, err := f.TryLock() if err != nil { @@ -32,53 +33,54 @@ func ExampleFlock_Locked() { } fmt.Printf("locked: %v\n", f.Locked()) + // Output: locked: true // locked: false } func ExampleFlock_TryLock() { // should probably put these in /var/lock - fileLock := flock.New(os.TempDir() + "/go-lock.lock") + f := flock.New(filepath.Join(os.TempDir(), "go-lock.lock")) - locked, err := fileLock.TryLock() + locked, err := f.TryLock() if err != nil { // handle locking error panic(err) } if locked { - fmt.Printf("path: %s; locked: %v\n", fileLock.Path(), fileLock.Locked()) + fmt.Printf("path: %s; locked: %v\n", f.Path(), f.Locked()) - if err := fileLock.Unlock(); err != nil { + if err := f.Unlock(); err != nil { // handle unlock error panic(err) } } - fmt.Printf("path: %s; locked: %v\n", fileLock.Path(), fileLock.Locked()) + fmt.Printf("path: %s; locked: %v\n", f.Path(), f.Locked()) } func ExampleFlock_TryLockContext() { // should probably put these in /var/lock - fileLock := flock.New(os.TempDir() + "/go-lock.lock") + f := flock.New(filepath.Join(os.TempDir(), "go-lock.lock")) lockCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - locked, err := fileLock.TryLockContext(lockCtx, 678*time.Millisecond) + locked, err := f.TryLockContext(lockCtx, 678*time.Millisecond) if err != nil { // handle locking error panic(err) } if locked { - fmt.Printf("path: %s; locked: %v\n", fileLock.Path(), fileLock.Locked()) + fmt.Printf("path: %s; locked: %v\n", f.Path(), f.Locked()) - if err := fileLock.Unlock(); err != nil { + if err := f.Unlock(); err != nil { // handle unlock error panic(err) } } - fmt.Printf("path: %s; locked: %v\n", fileLock.Path(), fileLock.Locked()) + fmt.Printf("path: %s; locked: %v\n", f.Path(), f.Locked()) } diff --git a/flock_internal_test.go b/flock_internal_test.go index 7a9d511..58b4ce7 100644 --- a/flock_internal_test.go +++ b/flock_internal_test.go @@ -17,8 +17,8 @@ func Test(t *testing.T) { tmpFile, err := os.CreateTemp(t.TempDir(), "go-flock-") require.NoError(t, err) - tmpFile.Close() - os.Remove(tmpFile.Name()) + _ = tmpFile.Close() + _ = os.Remove(tmpFile.Name()) lock := New(tmpFile.Name()) diff --git a/flock_test.go b/flock_test.go index 26104a9..5f63e55 100644 --- a/flock_test.go +++ b/flock_test.go @@ -32,55 +32,48 @@ func (s *TestSuite) SetupTest() { s.Require().NotNil(tmpFile) s.path = tmpFile.Name() + _ = tmpFile.Close() defer os.Remove(s.path) - tmpFile.Close() s.flock = flock.New(s.path) } func (s *TestSuite) TearDownTest() { _ = s.flock.Unlock() - os.Remove(s.path) + _ = os.Remove(s.path) } func (s *TestSuite) TestNew() { f := flock.New(s.path) s.Require().NotNil(f) - s.Equal(s.path, f.Path()) + s.Equal(f.Path(), s.path) s.False(f.Locked()) s.False(f.RLocked()) } func (s *TestSuite) TestFlock_Path() { - path := s.flock.Path() - s.Equal(s.path, path) + s.Equal(s.path, s.flock.Path()) } func (s *TestSuite) TestFlock_Locked() { - locked := s.flock.Locked() - s.False(locked) + s.False(s.flock.Locked()) } func (s *TestSuite) TestFlock_RLocked() { - locked := s.flock.RLocked() - s.False(locked) + s.False(s.flock.RLocked()) } func (s *TestSuite) TestFlock_String() { - str := s.flock.String() - s.Equal(s.path, str) + s.Equal(s.path, s.flock.String()) } func (s *TestSuite) TestFlock_TryLock() { s.False(s.flock.Locked()) s.False(s.flock.RLocked()) - var locked bool - var err error - - locked, err = s.flock.TryLock() + locked, err := s.flock.TryLock() s.Require().NoError(err) s.True(locked) s.True(s.flock.Locked()) @@ -101,10 +94,7 @@ func (s *TestSuite) TestFlock_TryRLock() { s.False(s.flock.Locked()) s.False(s.flock.RLocked()) - var locked bool - var err error - - locked, err = s.flock.TryRLock() + locked, err := s.flock.TryRLock() s.Require().NoError(err) s.True(locked) s.False(s.flock.Locked()) @@ -142,8 +132,9 @@ func (s *TestSuite) TestFlock_TryRLock() { } func (s *TestSuite) TestFlock_TryLockContext() { - // happy path ctx, cancel := context.WithCancel(context.Background()) + + // happy path locked, err := s.flock.TryLockContext(ctx, time.Second) s.Require().NoError(err) s.True(locked) @@ -165,14 +156,16 @@ func (s *TestSuite) TestFlock_TryLockContext() { } func (s *TestSuite) TestFlock_TryRLockContext() { - // happy path ctx, cancel := context.WithCancel(context.Background()) + + // happy path locked, err := s.flock.TryRLockContext(ctx, time.Second) s.Require().NoError(err) s.True(locked) // context already canceled cancel() + locked, err = flock.New(s.path).TryRLockContext(ctx, time.Second) s.Require().ErrorIs(err, context.Canceled) s.False(locked) @@ -190,9 +183,7 @@ func (s *TestSuite) TestFlock_TryRLockContext() { } func (s *TestSuite) TestFlock_Unlock() { - var err error - - err = s.flock.Unlock() + err := s.flock.Unlock() s.Require().NoError(err) // get a lock for us to unlock @@ -215,9 +206,7 @@ func (s *TestSuite) TestFlock_Lock() { s.False(s.flock.Locked()) s.False(s.flock.RLocked()) - var err error - - err = s.flock.Lock() + err := s.flock.Lock() s.Require().NoError(err) s.True(s.flock.Locked()) s.False(s.flock.RLocked()) @@ -259,9 +248,7 @@ func (s *TestSuite) TestFlock_RLock() { s.False(s.flock.Locked()) s.False(s.flock.RLocked()) - var err error - - err = s.flock.RLock() + err := s.flock.RLock() s.Require().NoError(err) s.False(s.flock.Locked()) s.True(s.flock.RLocked()) diff --git a/flock_unix.go b/flock_unix.go index f1d4e87..6f3d6f5 100644 --- a/flock_unix.go +++ b/flock_unix.go @@ -53,6 +53,7 @@ func (f *Flock) lock(locked *bool, flag int) error { if err := f.setFh(); err != nil { return err } + defer f.ensureFhState() } @@ -146,6 +147,7 @@ func (f *Flock) try(locked *bool, flag int) (bool, error) { if err := f.setFh(); err != nil { return false, err } + defer f.ensureFhState() } @@ -182,22 +184,26 @@ func (f *Flock) reopenFDOnError(err error) (bool, error) { if !errors.Is(err, syscall.EIO) && !errors.Is(err, syscall.EBADF) { return false, nil } - if st, err := f.fh.Stat(); err == nil { - // if the file is able to be read and written - if st.Mode()&f.perm == f.perm { - _ = f.fh.Close() - f.fh = nil - - // reopen in read-write mode and set the file handle - fh, err := os.OpenFile(f.path, f.flag, f.perm) - if err != nil { - return false, err - } - f.fh = fh - - return true, nil - } + + st, err := f.fh.Stat() + if err != nil { + return false, nil + } + + if st.Mode()&f.perm != f.perm { + return false, nil } - return false, nil + _ = f.fh.Close() + f.fh = nil + + // reopen in read-write mode and set the file handle + fh, err := os.OpenFile(f.path, f.flag, f.perm) + if err != nil { + return false, err + } + + f.fh = fh + + return true, nil } diff --git a/flock_unix_variants.go b/flock_unix_variants.go index 4a439f4..db80675 100644 --- a/flock_unix_variants.go +++ b/flock_unix_variants.go @@ -96,6 +96,7 @@ func (f *Flock) lock(locked *bool, flag lockType) error { if err := f.setFh(); err != nil { return err } + defer f.ensureFhState() } @@ -104,6 +105,7 @@ func (f *Flock) lock(locked *bool, flag lockType) error { } *locked = true + return nil } @@ -116,11 +118,14 @@ func (f *Flock) doLock(cmd cmdType, lt lockType, blocking bool) (bool, error) { if err != nil { return false, err } + ino := inode(fi.Sys().(*syscall.Stat_t).Ino) mu.Lock() + if i, dup := inodes[f]; dup && i != ino { mu.Unlock() + return false, &os.PathError{ Path: f.Path(), Err: errors.New("inode for file changed since last Lock or RLock"), @@ -130,7 +135,9 @@ func (f *Flock) doLock(cmd cmdType, lt lockType, blocking bool) (bool, error) { inodes[f] = ino var wait chan *Flock + l := locks[ino] + if l.owner == f { // This file already owns the lock, but the call may change its lock type. } else if l.owner == nil { @@ -145,7 +152,9 @@ func (f *Flock) doLock(cmd cmdType, lt lockType, blocking bool) (bool, error) { wait = make(chan *Flock) l.queue = append(l.queue, wait) } + locks[ino] = l + mu.Unlock() if wait != nil { @@ -155,9 +164,11 @@ func (f *Flock) doLock(cmd cmdType, lt lockType, blocking bool) (bool, error) { err = setlkw(f.fh.Fd(), cmd, lt) if err != nil { f.doUnlock() + if cmd == tryLock && err == unix.EACCES { return false, nil } + return false, err } @@ -178,7 +189,7 @@ func (f *Flock) Unlock() error { return err } - f.fh.Close() + _ = f.fh.Close() f.l = false f.r = false @@ -189,11 +200,14 @@ func (f *Flock) Unlock() error { func (f *Flock) doUnlock() (err error) { var owner *Flock + mu.Lock() + ino, ok := inodes[f] if ok { owner = locks[ino].owner } + mu.Unlock() if owner == f { @@ -201,7 +215,9 @@ func (f *Flock) doUnlock() (err error) { } mu.Lock() + l := locks[ino] + if len(l.queue) == 0 { // No waiters: remove the map entry. delete(locks, ino) @@ -212,7 +228,9 @@ func (f *Flock) doUnlock() (err error) { l.queue = l.queue[1:] locks[ino] = l } + delete(inodes, f) + mu.Unlock() return err @@ -254,6 +272,7 @@ func (f *Flock) try(locked *bool, flag lockType) (bool, error) { if err := f.setFh(); err != nil { return false, err } + defer f.ensureFhState() } diff --git a/flock_winapi.go b/flock_winapi.go index 9e4df34..f4c7e09 100644 --- a/flock_winapi.go +++ b/flock_winapi.go @@ -43,15 +43,15 @@ func lockFileEx(handle syscall.Handle, flags, reserved, numberOfBytesToLockLow, uintptr(numberOfBytesToLockHigh), uintptr(unsafe.Pointer(offset))) - if r1 != 1 { - if errNo == 0 { - return false, syscall.EINVAL - } + if r1 == 1 { + return true, 0 + } - return false, errNo + if errNo == 0 { + return false, syscall.EINVAL } - return true, 0 + return false, errNo } func unlockFileEx(handle syscall.Handle, reserved, numberOfBytesToLockLow, numberOfBytesToLockHigh uint32, offset *syscall.Overlapped) (bool, syscall.Errno) { @@ -63,13 +63,13 @@ func unlockFileEx(handle syscall.Handle, reserved, numberOfBytesToLockLow, numbe uintptr(numberOfBytesToLockHigh), uintptr(unsafe.Pointer(offset))) - if r1 != 1 { - if errNo == 0 { - return false, syscall.EINVAL - } + if r1 == 1 { + return true, 0 + } - return false, errNo + if errNo == 0 { + return false, syscall.EINVAL } - return true, 0 + return false, errNo } diff --git a/flock_windows.go b/flock_windows.go index 82a24a2..353be2c 100644 --- a/flock_windows.go +++ b/flock_windows.go @@ -47,6 +47,7 @@ func (f *Flock) lock(locked *bool, flag uint32) error { if err := f.setFh(); err != nil { return err } + defer f.ensureFhState() } @@ -56,6 +57,7 @@ func (f *Flock) lock(locked *bool, flag uint32) error { } *locked = true + return nil } @@ -126,6 +128,7 @@ func (f *Flock) try(locked *bool, flag uint32) (bool, error) { if err := f.setFh(); err != nil { return false, err } + defer f.ensureFhState() }