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

Enh/improve error handling #9

Merged
merged 9 commits into from
Oct 13, 2019
Merged

Enh/improve error handling #9

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fdb2e12
Improve error handling #6434
leiter-jakab Oct 9, 2019
964453e
Remove accidental change #6434
leiter-jakab Oct 9, 2019
e3b1196
Update error generation #6434
leiter-jakab Oct 10, 2019
82265fc
Remove unused import #6434
leiter-jakab Oct 10, 2019
eecf9f4
Improve error handing #6434
leiter-jakab Oct 12, 2019
2b2e04f
feat(error): idiomatic errors
Stebalien Oct 13, 2019
dd69e01
gx: ungxify
Stebalien Oct 13, 2019
51d08d3
ci: remove jenkins support
Stebalien Oct 13, 2019
10a6330
ci: switch to go 1.12
Stebalien Oct 13, 2019
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
51 changes: 32 additions & 19 deletions fslock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,30 @@ func errPerm(path string) error {

// Lock creates the lock.
func Lock(confdir, lockFile string) (io.Closer, error) {
return lock.Lock(filepath.Join(confdir, lockFile))
lk, err := lock.Lock(filepath.Join(confdir, lockFile))
if err != nil {
// EAGAIN == someone else has the lock
if err == syscall.EAGAIN {
return lk, fmt.Errorf("Someone else has the lock: %s", filepath.Join(confdir, lockFile))
}
if strings.Contains(err.Error(), "resource temporarily unavailable") {
return lk, fmt.Errorf("Someone else has the lock: %s", filepath.Join(confdir, lockFile))
}

// we hold the lock ourselves
if strings.Contains(err.Error(), "already locked") {
return lk, fmt.Errorf("Lock is already held by us: %s", filepath.Join(confdir, lockFile))
}

// lock fails on permissions error
if os.IsPermission(err) {
return lk, errPerm(confdir)
}
if isLockCreatePermFail(err) {
return lk, errPerm(confdir)
}
}
return lk, err
}

// Locked checks if there is a lock already set.
Expand All @@ -35,30 +58,20 @@ func Locked(confdir, lockFile string) (bool, error) {

lk, err := Lock(confdir, lockFile)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Given that we're now parsing the error in Lock, what if we just had Lock return a custom error type on failure? We could return an error implementing:

type myerror interface {
  Permission() bool // returns true if this is a permission error.
  Locked() bool // returns true if this error means that the lock has already been taken.
}

Then we can provide two functions to determine why taking the lock failed:

func IsLocked(err) bool { ... }
func IsPermission(err) bool { ... }

This function would be reduced to an IsLocked(err) call (and a debug statement).

Copy link
Author

@leiter-jakab leiter-jakab Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good to me. It felt very convoluted and weird that Lock is producing those specific errors and Locked is checking strings to do a very similar error categorization.

Copy link
Author

Choose a reason for hiding this comment

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

@Stebalien I have been thinking about this and I have to learn more about idiomatic error handling in Go before I make any changes.

On the other hand, I'm not sure that the original issue has been solved by my changes.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The original issue has been solved, I'm just trying to avoid parsing errors in two places.

// EAGAIN == someone else has the lock
if err == syscall.EAGAIN {
log.Debugf("Someone else has the lock: %s", filepath.Join(confdir, lockFile))
return true, nil
}
if strings.Contains(err.Error(), "resource temporarily unavailable") {
log.Debugf("Can't lock file: %s.\n reason: %s", filepath.Join(confdir, lockFile), err.Error())
errCase := err.Error()
if strings.Contains(errCase, "Lock is already held by us:") {
log.Debugf(errCase)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

return true, nil
}

// we hold the lock ourselves
if strings.Contains(err.Error(), "already locked") {
log.Debugf("Lock is already held by us: %s", filepath.Join(confdir, lockFile))
if strings.Contains(errCase, "Someone else has the lock:") {
log.Debugf(errCase)
Copy link
Member

Choose a reason for hiding this comment

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

This should be log.Debug(errCase) (otherwise, we'll parse errCase for string format specifiers).

return true, nil
}

// lock fails on permissions error
if os.IsPermission(err) {
log.Debugf("Lock fails on permissions error")
return false, errPerm(confdir)
}
if isLockCreatePermFail(err) {
log.Debugf("Lock fails on permissions error")
return false, errPerm(confdir)
if strings.Contains(errCase, "permissions denied") {
log.Debugf(errCase)
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

return false, err
}

// otherwise, we cant guarantee anything, error out
Expand Down