Improve lockFile error handling

libgoal/lockedFile.go

@@ -20,102 +20,85 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
-	"syscall"
-	"time"
 )
 
-// Platform-dependant locker implementation
-// How to extend
-// 1. Create two new files locker.go and locker_platform.go
-// 2. Put appropriate build tags
-// 3. Move unixLocker implementation and `newLockedFile` method to locker.go
-// 4. Implement platform-specific locker in locker_platform.go
-// 5. Ensure `newLockedFile` sets platform-specific locker
-//    so that lockedFile.read and lockedFile.write work correctly
-
 type locker interface {
 	tryRLock(fd *os.File) error
 	tryLock(fd *os.File) error
 	unlock(fd *os.File) error
 }
 
-type unixLocker struct {
-}
-
-func (f *unixLocker) tryRLock(fd *os.File) error {
-	return syscall.Flock(int(fd.Fd()), syscall.LOCK_SH|syscall.LOCK_NB)
-}
-
-func (f *unixLocker) tryLock(fd *os.File) error {
-	return syscall.Flock(int(fd.Fd()), syscall.LOCK_EX|syscall.LOCK_NB)
-}
-
-func (f *unixLocker) unlock(fd *os.File) error {
-	return syscall.Flock(int(fd.Fd()), syscall.LOCK_UN)
+func newLockedFile(path string) *lockedFile {
+	return &lockedFile{
+		path:   path,
+		locker: makeLocker(),
+	}
 }
 
 // lockedFile implementation
-// It uses non-blocking acquisition with repeats
-// and supposed to be platform-agnostic with appropriate locker implementation.
+// It a platform-agnostic with appropriate locker implementation.
 // Each platform needs own specific `newLockedFile`
-const maxRepeats = 10
-const sleepInterval = 10 * time.Millisecond
 
 type lockedFile struct {
 	path   string
 	locker locker
 }
 
-func newLockedFile(path string) *lockedFile {
-	return &lockedFile{
-		path:   path,
-		locker: &unixLocker{},
-	}
-}
-
-func (f *lockedFile) read() ([]byte, error) {
+func (f *lockedFile) read() (bytes []byte, err error) {
 	fd, err := os.Open(f.path)
 	if err != nil {
-		return nil, err
+		return
 	}
-	defer fd.Close()
+	defer func() {
+		err2 := fd.Close()
+		if err2 != nil {
+			err = err2
+		}
+	}()
 
-	lockFunc := func() error { return f.locker.tryRLock(fd) }
-	err = attemptLock(lockFunc)
+	err = f.locker.tryRLock(fd)
 	if err != nil {
-		return nil, fmt.Errorf("Can't acquire lock for %s: %s", f.path, err.Error())
+		err = fmt.Errorf("Can't acquire read lock for %s: %s", f.path, err.Error())
+		return
 	}
-	defer f.locker.unlock(fd)
+	defer func() {
+		err2 := f.locker.unlock(fd)
+		if err2 != nil {
+			err = fmt.Errorf("Can't unlock for %s: %s", f.path, err2.Error())
+		}
+	}()
 
-	return ioutil.ReadAll(fd)
+	bytes, err = ioutil.ReadAll(fd)
+	return
 }
 
-func (f *lockedFile) write(data []byte, perm os.FileMode) error {
+func (f *lockedFile) write(data []byte, perm os.FileMode) (err error) {
 	fd, err := os.OpenFile(f.path, os.O_WRONLY|os.O_CREATE, perm)
 	if err != nil {
-		return err
+		return
 	}
-	defer fd.Close()
+	defer func() {
+		err2 := fd.Close()
+		if err2 != nil {
+			err = err2
+		}
+	}()
 
-	lockFunc := func() error { return f.locker.tryLock(fd) }
-	err = attemptLock(lockFunc)
+	err = f.locker.tryLock(fd)
 	if err != nil {
 		return fmt.Errorf("Can't acquire lock for %s: %s", f.path, err.Error())
 	}
-	defer f.locker.unlock(fd)
-
-	fd.Truncate(0)
-	_, err = fd.Write(data)
-	return err
-}
-
-func attemptLock(lockFunc func() error) error {
-	var savedError error
-	for repeatCounter := 0; repeatCounter < maxRepeats; repeatCounter++ {
-		if savedError = lockFunc(); savedError == nil {
-			break
+	defer func() {
+		err2 := f.locker.unlock(fd)
+		if err2 != nil {
+			err = fmt.Errorf("Can't unlock for %s: %s", f.path, err2.Error())
 		}
-		time.Sleep(sleepInterval)
+	}()
+
+	err = fd.Truncate(0)
+	if err != nil {
+		return
 	}
-	return savedError
+	_, err = fd.Write(data)
+	return
 }

libgoal/lockedFileLinux.go

@@ -0,0 +1,71 @@
+// Copyright (C) 2019-2020 Algorand, Inc.
+// This file is part of go-algorand
+//
+// go-algorand is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as
+// published by the Free Software Foundation, either version 3 of the
+// License, or (at your option) any later version.
+//
+// go-algorand is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with go-algorand.  If not, see <https://www.gnu.org/licenses/>.
+
+// +build linux
+
+package libgoal
+
+import (
+	"io"
+	"os"
+	"syscall"
+
+	"golang.org/x/sys/unix"
+)
+
+type linuxLocker struct {
+}
+
+// makeLocker create a unix file locker.
+// note that the desired way is to use the OFD locker, which locks on the file descriptor level.
+// falling back to the non-OFD lock would allow obtaining two locks by the same process. If this becomes
+// and issue, we might want to use flock, which wouldn't work across NFS.
+func makeLocker() *linuxLocker {
+	locker := &linuxLocker{}
+	return locker
+}
+
+// the FcntlFlock has the most consistent behaviour across platforms,
+// and supports both local and network file systems.
+func (f *linuxLocker) tryRLock(fd *os.File) error {
+	flock := &syscall.Flock_t{
+		Type:   syscall.F_RDLCK,
+		Whence: int16(io.SeekStart),
+		Start:  0,
+		Len:    0,
+	}
+	return syscall.FcntlFlock(fd.Fd(), unix.F_OFD_SETLKW, flock)
+}
+
+func (f *linuxLocker) tryLock(fd *os.File) error {
+	flock := &syscall.Flock_t{
+		Type:   syscall.F_WRLCK,
+		Whence: int16(io.SeekStart),
+		Start:  0,
+		Len:    0,
+	}
+	return syscall.FcntlFlock(fd.Fd(), unix.F_OFD_SETLKW, flock)
+}
+
+func (f *linuxLocker) unlock(fd *os.File) error {
+	flock := &syscall.Flock_t{
+		Type:   syscall.F_UNLCK,
+		Whence: int16(io.SeekStart),
+		Start:  0,
+		Len:    0,
+	}
+	return syscall.FcntlFlock(fd.Fd(), unix.F_OFD_SETLKW, flock)
+}

libgoal/lockedFileUnix.go

@@ -0,0 +1,77 @@
+// Copyright (C) 2019-2020 Algorand, Inc.
+// This file is part of go-algorand
+//
+// go-algorand is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as
+// published by the Free Software Foundation, either version 3 of the
+// License, or (at your option) any later version.
+//
+// go-algorand is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with go-algorand.  If not, see <https://www.gnu.org/licenses/>.
+
+// +build !linux,!windows
+
+package libgoal
+
+import (
+	"io"
+	"os"
+	"syscall"
+)
+
+type unixLocker struct {
+	setLockWait int
+}
+
+// makeLocker create a unix file locker.
+// note that the desired way is to use the OFD locker, which locks on the file descriptor level.
+// falling back to the non-OFD lock would allow obtaining two locks by the same process. If this becomes
+// and issue, we might want to use flock, which wouldn't work across NFS.
+func makeLocker() *unixLocker {
+	locker := &unixLocker{}
+	getlk := syscall.Flock_t{Type: syscall.F_RDLCK}
+	if err := syscall.FcntlFlock(0, 36 /*F_OFD_GETLK*/, &getlk); err == nil {
+		// constants from /usr/include/bits/fcntl-linux.h
+		locker.setLockWait = 38 // F_OFD_SETLKW
+	} else {
+		locker.setLockWait = syscall.F_SETLKW
+	}
+	return locker
+}
+
+// the FcntlFlock has the most unixLocker behaviour across platforms,
+// and supports both local and network file systems.
+func (f *unixLocker) tryRLock(fd *os.File) error {
+	flock := &syscall.Flock_t{
+		Type:   syscall.F_RDLCK,
+		Whence: int16(io.SeekStart),
+		Start:  0,
+		Len:    0,
+	}
+	return syscall.FcntlFlock(fd.Fd(), f.setLockWait, flock)
+}
+
+func (f *unixLocker) tryLock(fd *os.File) error {
+	flock := &syscall.Flock_t{
+		Type:   syscall.F_WRLCK,
+		Whence: int16(io.SeekStart),
+		Start:  0,
+		Len:    0,
+	}
+	return syscall.FcntlFlock(fd.Fd(), f.setLockWait, flock)
+}
+
+func (f *unixLocker) unlock(fd *os.File) error {
+	flock := &syscall.Flock_t{
+		Type:   syscall.F_UNLCK,
+		Whence: int16(io.SeekStart),
+		Start:  0,
+		Len:    0,
+	}
+	return syscall.FcntlFlock(fd.Fd(), f.setLockWait, flock)
+}