Enh/improve error handling #9
Conversation
As described in #6434, I moved error handing to the Lock function.
I changed go.mod by accident. Removing the change with this commit.
fslock.go
Outdated
| if err != nil { | ||
| // EAGAIN == someone else has the lock | ||
| if err == syscall.EAGAIN { | ||
| return lk, errors.New(fmt.Sprintf("Someone else has the lock: %s", filepath.Join(confdir, lockFile))) |
There was a problem hiding this comment.
| return lk, errors.New(fmt.Sprintf("Someone else has the lock: %s", filepath.Join(confdir, lockFile))) | |
| return lk, fmt.Errorf("Someone else has the lock: %s", filepath.Join(confdir, lockFile)) |
fslock.go
Outdated
| @@ -35,30 +59,20 @@ func Locked(confdir, lockFile string) (bool, error) { | |||
|
|
|||
| lk, err := Lock(confdir, lockFile) | |||
| if err != nil { | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
The original issue has been solved, I'm just trying to avoid parsing errors in two places.
Use fmt.Errorf to create error messages in a clear, idiomatic way.
I did not remove the errors package after changing error formatting. Fixing it with this commit.
fslock.go
Outdated
| @@ -35,30 +59,20 @@ func Locked(confdir, lockFile string) (bool, error) { | |||
|
|
|||
| lk, err := Lock(confdir, lockFile) | |||
| if err != nil { | |||
There was a problem hiding this comment.
The original issue has been solved, I'm just trying to avoid parsing errors in two places.
fslock.go
Outdated
| 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) |
There was a problem hiding this comment.
This should be log.Debug(errCase) (otherwise, we'll parse errCase for string format specifiers).
fslock.go
Outdated
| log.Debugf("Lock fails on permissions error") | ||
| return false, errPerm(confdir) | ||
| if strings.Contains(errCase, "permissions denied") { | ||
| log.Debugf(errCase) |
fslock.go
Outdated
| 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) |
Introduced custom errors: * LockError * PermError They implement the error interface. IsLocked function now checks error type insead of content of the error message.
* Use PathError to: * Keep `os.IsPermission` working. * Help with tooling that looks for path errors. * Tell users to use `errors.As` to determine if an error is a "locked" error.
|
I went digging through some go error documentation and realized we could use the built-in error types. I took the liberty of making those changes rather than try to change direction on you again. |
Very nice, I learned quite a bit. Could you please point me to the documentation where you found the relevant info? Thanks. |
|
Hi IPFS Team, great project. Hoping to help with this change. Not sure if this is the correct solution, but I would love to hear your feedback.
Regards,
Gergo