From 38863acf5f26e4b4e0af8e61a53f1e6ec6e708e6 Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 19 Dec 2019 13:16:39 -0500 Subject: [PATCH 1/2] Fix concurrent access to wallet handles cache in goal * In rare cases (i.e. e2e tests run in parallel on the same network) a race cond happens when accessing goal.cache/walletHandles.json file * Introduce advisory locking on the mentioned file * Implementation is extendable by implementing *locker* interface for specific platform and providing a new *newLockedFile* constructor. --- libgoal/lockedFile.go | 120 +++++++++++++++++++++++++++++++++++++++ libgoal/walletHandles.go | 32 ++++++----- 2 files changed, 137 insertions(+), 15 deletions(-) create mode 100644 libgoal/lockedFile.go diff --git a/libgoal/lockedFile.go b/libgoal/lockedFile.go new file mode 100644 index 0000000000..f560b9cdb2 --- /dev/null +++ b/libgoal/lockedFile.go @@ -0,0 +1,120 @@ +// Copyright (C) 2019 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 . + +package libgoal + +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) +} + +// lockedFile implementation +// It uses non-blocking acquisition with repeats +// and supposed to be platform-agnostic with appropriate locker implementation. +// Each platform needs own specific `newLockedFile` +const maxRepeats = 10 +const sleepInterval = 10 * time.Microsecond + +type lockedFile struct { + path string + locker locker +} + +func newLockedFile(path string) *lockedFile { + f := new(lockedFile) + f.path = path + f.locker = new(unixLocker) + return f +} + +func (f *lockedFile) read() ([]byte, error) { + fd, err := os.Open(f.path) + if err != nil { + return nil, err + } + defer fd.Close() + + lockFunc := func() error { return f.locker.tryRLock(fd) } + err = attemptLock(lockFunc) + if err != nil { + return nil, fmt.Errorf("Can't acquire lock for %s: %s", f.path, err.Error()) + } + defer f.locker.unlock(fd) + + return ioutil.ReadAll(fd) +} + +func (f *lockedFile) write(data []byte, perm os.FileMode) error { + fd, err := os.OpenFile(f.path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) + if err != nil { + return err + } + defer fd.Close() + + lockFunc := func() error { return f.locker.tryLock(fd) } + err = attemptLock(lockFunc) + if err != nil { + return fmt.Errorf("Can't acquire lock for %s: %s", f.path, err.Error()) + } + defer f.locker.unlock(fd) + + _, 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 + } + time.Sleep(sleepInterval) + } + return savedError +} diff --git a/libgoal/walletHandles.go b/libgoal/walletHandles.go index cdc1b18ee8..15799d4c48 100644 --- a/libgoal/walletHandles.go +++ b/libgoal/walletHandles.go @@ -18,7 +18,6 @@ package libgoal import ( "encoding/json" - "io/ioutil" "os" "path/filepath" ) @@ -31,14 +30,21 @@ type walletHandles struct { Handles map[string]string } +func readLocked(path string) ([]byte, error) { + lf := newLockedFile(path) + return lf.read() +} + +func writeLocked(path string, data []byte, perm os.FileMode) error { + lf := newLockedFile(path) + return lf.write(data, perm) +} + func (whs *walletHandles) loadFromDisk(cacheDir string) error { - cachePath, err := walletHandlesCachePath(cacheDir) - if err != nil { - return err - } - _, err = os.Stat(cachePath) + path := walletHandlesCachePath(cacheDir) + _, err := os.Stat(path) if !os.IsNotExist(err) { - raw, err := ioutil.ReadFile(cachePath) + raw, err := readLocked(path) if err != nil { return err } @@ -56,20 +62,16 @@ func (whs *walletHandles) dumpToDisk(cacheDir string) error { return err } - path, err := walletHandlesCachePath(cacheDir) - if err != nil { - return err - } - - err = ioutil.WriteFile(path, raw, 0600) + path := walletHandlesCachePath(cacheDir) + err = writeLocked(path, raw, 0600) if err != nil { return err } return nil } -func walletHandlesCachePath(cacheDir string) (string, error) { - return filepath.Join(cacheDir, walletHandlesJSONName), nil +func walletHandlesCachePath(cacheDir string) string { + return filepath.Join(cacheDir, walletHandlesJSONName) } func loadWalletHandleFromDisk(walletID []byte, cacheDir string) ([]byte, error) { From 892dc6d272bbdd0ddf4e90022fe336fae5e30a1a Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 19 Dec 2019 15:04:05 -0500 Subject: [PATCH 2/2] Address PR review notes * Do no truncate before obtaining the lock * Increase waiting interval to 10 ms * Simplify newLockedFile constructor --- libgoal/lockedFile.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/libgoal/lockedFile.go b/libgoal/lockedFile.go index f560b9cdb2..711d1f590a 100644 --- a/libgoal/lockedFile.go +++ b/libgoal/lockedFile.go @@ -59,7 +59,7 @@ func (f *unixLocker) unlock(fd *os.File) error { // and supposed to be platform-agnostic with appropriate locker implementation. // Each platform needs own specific `newLockedFile` const maxRepeats = 10 -const sleepInterval = 10 * time.Microsecond +const sleepInterval = 10 * time.Millisecond type lockedFile struct { path string @@ -67,10 +67,10 @@ type lockedFile struct { } func newLockedFile(path string) *lockedFile { - f := new(lockedFile) - f.path = path - f.locker = new(unixLocker) - return f + return &lockedFile{ + path: path, + locker: &unixLocker{}, + } } func (f *lockedFile) read() ([]byte, error) { @@ -91,7 +91,7 @@ func (f *lockedFile) read() ([]byte, error) { } func (f *lockedFile) write(data []byte, perm os.FileMode) error { - fd, err := os.OpenFile(f.path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) + fd, err := os.OpenFile(f.path, os.O_WRONLY|os.O_CREATE, perm) if err != nil { return err } @@ -104,6 +104,7 @@ func (f *lockedFile) write(data []byte, perm os.FileMode) error { } defer f.locker.unlock(fd) + fd.Truncate(0) _, err = fd.Write(data) return err }