Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 4558 from Core CLI: Adding flock #358

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Issue 4558 from Core CLI: Adding flock #358

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5b287e1
test commit
Jan 6, 2023
0326ab6
testing one impl. of code -- flock on persistor
tonystarkjr3 Jan 9, 2023
a110d60
accidental missing init
tonystarkjr3 Jan 10, 2023
28b7dff
missing init
tonystarkjr3 Jan 10, 2023
1e1c94d
lockedwrite method
tonystarkjr3 Jan 13, 2023
3d21fa8
testcommit
tonystarkjr3 Jan 13, 2023
1e04bf2
reversing test commit
tonystarkjr3 Jan 13, 2023
c6cc095
removing commented import
tonystarkjr3 Jan 13, 2023
0e1ead2
slight comment update
tonystarkjr3 Jan 13, 2023
a8a3f6d
TryLockContext() instead of Lock()
tonystarkjr3 Jan 18, 2023
f5a3605
comments
tonystarkjr3 Jan 18, 2023
0790985
more comments
tonystarkjr3 Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion bluemix/configuration/core_config/bx_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,10 @@ func (c *bxConfig) writeRaw(cb func()) {

err := c.persistor.Save(c.data.raw)
if err != nil {
c.onError(err)
c.onError(err) // NOTE: the error could be triggered by a file-locking issue,
// a file-unlocking issue, OR a file-writing issue; currently, the error chain
// ends in `panic("configuration error: " + err.Error()"`, which is somewhat
// generic but sufficient for all three of these error types
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: For readability can we move the comment above line 157.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

}
}

Expand Down
21 changes: 18 additions & 3 deletions bluemix/configuration/persistor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"path/filepath"

"github.com/IBM-Cloud/ibm-cloud-cli-sdk/common/file_helpers"
"github.com/gofrs/flock"
)

const (
Expand All @@ -26,11 +27,13 @@ type Persistor interface {

type DiskPersistor struct {
filePath string
fileLock *flock.Flock
}

func NewDiskPersistor(path string) DiskPersistor {
return DiskPersistor{
filePath: path,
fileLock: flock.New(path),
}
}

Expand All @@ -44,14 +47,26 @@ func (dp DiskPersistor) Load(data DataInterface) error {
return err
}

if err != nil {
err = dp.write(data)
if err != nil { // strange: requiring an error (to allow write attempt to continue), as long as it is not a permission error
Copy link
Collaborator Author

@tonystarkjr3 tonystarkjr3 Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange code.
Requiring a non-permission error to actually carry out the writing to the config? Weird!
Please see my OP for my thoughts on why this is benign despite being pretty obviously counterintuitive.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is strange but the docs strongly recommend using TryLock instead due to the potential blocking issue. Can we try that and see if it resolves this issue

Lock is a blocking call to try and take an exclusive file lock. It will wait until it is able to obtain the exclusive file lock. It's recommended that TryLock() be used over this function. This function may block the ability to query the current Locked() or RLocked() status due to a RW-mutex lock. (doc)

err = dp.lockedWrite(data)
}
return err
}

func (dp DiskPersistor) lockedWrite(data DataInterface) error {
lockErr := dp.fileLock.Lock() // provide a file lock, in addition to the RW mutex (in calling functions), just while dp.write is called
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: For readability can we move the comments above line 57

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

if lockErr != nil {
return lockErr
}
writeErr := dp.write(data)
if writeErr != nil {
return writeErr
}
return dp.fileLock.Unlock()
}

func (dp DiskPersistor) Save(data DataInterface) error {
return dp.write(data)
return dp.lockedWrite(data)
}

func (dp DiskPersistor) read(data DataInterface) error {
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down