Fix: Improvement to Concurrency Tolerance (#363)

* fix: config corruption on concurrent program runs

* fix: config corruptions during concurrent execution

* conventional err syntax

Author: tonystarkjr3

.gitignore

@@ -1,2 +1,3 @@
 vendor/*
-!vendor/vendor.json
+!vendor/vendor.json
+.vscode

bluemix/configuration/core_config/bx_config.go

@@ -119,8 +119,8 @@ func (c *bxConfig) init() {
 }
 
 func (c *bxConfig) read(cb func()) {
-	c.lock.RLock()
-	defer c.lock.RUnlock()
+	/* concurrency note: init() calls the persistor's Load(), which has a flock,
+	via lockedRead() and lockedWrite(), surrounding the critical sections */
 
 	c.init()
 

bluemix/configuration/core_config/cf_config.go

@@ -86,7 +86,7 @@ type cfConfig struct {
 	data      *CFConfigData
 	persistor configuration.Persistor
 	initOnce  *sync.Once
-	lock      sync.RWMutex
+	lock      sync.Mutex
 	onError   func(error)
 }
 
@@ -119,9 +119,8 @@ func (c *cfConfig) init() {
 }
 
 func (c *cfConfig) read(cb func()) {
-	c.lock.RLock()
-	defer c.lock.RUnlock()
-
+	/* concurrency note: init() calls the persistor's Load(), which has a flock,
+	via lockedRead() and lockedWrite(), surrounding the critical sections */
 	c.init()
 
 	cb()

bluemix/configuration/persistor.go

@@ -1,11 +1,14 @@
 package configuration
 
 import (
+	"context"
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"time"
 
 	"github.com/IBM-Cloud/ibm-cloud-cli-sdk/common/file_helpers"
+	"github.com/gofrs/flock"
 )
 
 const (
@@ -25,33 +28,71 @@ type Persistor interface {
 }
 
 type DiskPersistor struct {
-	filePath string
+	filePath      string
+	fileLock      *flock.Flock
+	parentContext context.Context
 }
 
 func NewDiskPersistor(path string) DiskPersistor {
 	return DiskPersistor{
-		filePath: path,
+		filePath:      path,
+		fileLock:      flock.New(path),
+		parentContext: context.Background(),
 	}
 }
 
 func (dp DiskPersistor) Exists() bool {
 	return file_helpers.FileExists(dp.filePath)
 }
 
+func (dp *DiskPersistor) lockedRead(data DataInterface) error {
+	lockCtx, cancelLockCtx := context.WithTimeout(dp.parentContext, 30*time.Second) /* allotting a 30-second timeout means there can be a maximum of 298 failed retrials (each up to 500 ms, as
+	specified after the deferred call to cancelLockCtx). 30 appears to be a conventional value for a parent context passed to TryLockContext, as per docs */
+	defer cancelLockCtx()
+	_, lockErr := dp.fileLock.TryLockContext(lockCtx, 100*time.Millisecond) /* provide a file lock just while dp.read is called, because it calls an unmarshaling function
+	The boolean (first return value) can be wild-carded because lockErr must be non-nil when the lock-acquiring fails (whereby the boolean will be false) */
+	defer dp.fileLock.Unlock()
+	if lockErr != nil {
+		return lockErr
+	}
+	readErr := dp.read(data)
+	if readErr != nil {
+		return readErr
+	}
+	return nil
+}
+
 func (dp DiskPersistor) Load(data DataInterface) error {
-	err := dp.read(data)
+	err := dp.lockedRead(data)
 	if os.IsPermission(err) {
 		return err
 	}
 
-	if err != nil {
-		err = dp.write(data)
+	if err != nil { /* would happen if there was nothing to read (EOF) */
+		err = dp.lockedWrite(data)
 	}
 	return err
 }
 
+func (dp DiskPersistor) lockedWrite(data DataInterface) error {
+	lockCtx, cancelLockCtx := context.WithTimeout(dp.parentContext, 30*time.Second) /* allotting a 30-second timeout means there can be a maximum of 298 failed retrials (each up to 500 ms, as
+	specified after the deferred call to cancelLockCtx). 30 appears to be a conventional value for a parent context passed to TryLockContext, as per docs */
+	defer cancelLockCtx()
+	_, lockErr := dp.fileLock.TryLockContext(lockCtx, 100*time.Millisecond) /* provide a file lock just while dp.read is called, because it calls an unmarshaling function
+	The boolean (first return value) can be wild-carded because lockErr must be non-nil when the lock-acquiring fails (whereby the boolean will be false) */
+	defer dp.fileLock.Unlock()
+	if lockErr != nil {
+		return lockErr
+	}
+	writeErr := dp.write(data)
+	if writeErr != nil {
+		return writeErr
+	}
+	return nil
+}
+
 func (dp DiskPersistor) Save(data DataInterface) error {
-	return dp.write(data)
+	return dp.lockedWrite(data)
 }
 
 func (dp DiskPersistor) read(data DataInterface) error {

go.mod

@@ -22,6 +22,7 @@ require (
 require (
 	github.com/BurntSushi/toml v1.1.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/gofrs/flock v0.8.1 // indirect
 	github.com/golang/protobuf v1.5.2 // indirect
 	github.com/hpcloud/tail v1.0.0 // indirect
 	github.com/inconshreveable/mousetrap v1.0.0 // indirect

go.sum

@@ -32,6 +32,8 @@ github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2
 github.com/go-logfmt/logfmt v0.3.0/go.mod h1:Qt1PoO58o5twSAckw1HlFXLmHsOX5/0LbT9GBnD5lWE=
 github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V4qmtdjCk=
 github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
+github.com/gofrs/flock v0.8.1 h1:+gYjHKf32LDeiEEFhQaotPbLuUXjY5ZqxKgXy7n59aw=
+github.com/gofrs/flock v0.8.1/go.mod h1:F1TvTiK9OcQqauNUHlbJvyl9Qa1QvF/gOUDKA14jxHU=
 github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
 github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zVXpSg4=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=