Handle errors in renter package #91
Conversation
| @@ -4,6 +4,7 @@ go 1.13 | |||
|
|
|||
| require ( | |||
| github.com/aead/chacha20 v0.0.0-20180709150244-8b13a72661da | |||
| github.com/hashicorp/go-multierror v1.1.0 | |||
There was a problem hiding this comment.
I don't add new dependencies lightly. This one does not strike me as necessary.
| if e := f.Close(); e != nil && !errors.Is(e, os.ErrClosed) { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The error from f.Close is already checked at the bottom of this function. And if we return early, then we don't care about this error anyway.
There was a problem hiding this comment.
Actually, it should remove the temporary file here if exists. If writing a metafile fails, no one removes it.
There was a problem hiding this comment.
Failure to Close does not indicate that the file is corrupt, so removing it would be premature. This behavior is in line with standard library functions such as ioutil.WriteFile.
| if e := f.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() |
There was a problem hiding this comment.
We don't care about this error -- there's nothing we can do with this information.
There was a problem hiding this comment.
At least we can know the storage is going to die :)
There was a problem hiding this comment.
Close on a read-only file is basically a no-op anyway, so I don't think it will indicate anything about the health of your disk. Even ioutil.ReadFile ignores this error: https://golang.org/src/io/ioutil/ioutil.go?s=1503:1549#L42
| if e := f.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() |
There was a problem hiding this comment.
same as above -- not actionable
| if e := zip.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() |
| s.Close() | ||
| if e := s.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } |
| err = conn.SetDeadline(time.Now().Add(60 * time.Second)) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Practically speaking, I don't think it's possible for an error to be returned here. Looking at the source, (TCPConn).SetDeadline will only return an error if the underlying file descriptor is closed, and we know that it's open here. And since this is a TCPConn, we don't need to worry about other types that implement SetDeadline with weird behavior.
However, it's possible that the implementation of TCPConn could change in the future. Furthermore, I will probably allow arbitrary net.Conns in Session soon (as per #92). And in the worst case, failing to set a deadline could cause a Read or Write to block indefinitely. So I suppose this error should be handled properly.
| conn.Close() | ||
| if e := conn.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } |
| d.Close() | ||
| if e := d.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } |
There was a problem hiding this comment.
There's rarely any reason to care about Close failing on a directory, but I guess this should be handled. We can use a simple []string and errors.New for this.
There was a problem hiding this comment.
Unfortunately, such a simple implementation with []string and errors.New doesn't work because we cannot retrieve individual errors. As far as I know, go-multierror is one of the best options that allow us to check if a returned error containers some specific error.
There was a problem hiding this comment.
I can't think of any realistic circumstance in which you would need to programmatically handle the individual errors from closing directories. This is the sort of error that you would, at most, log somewhere and then manually inspect later. But in reality if you get an error when closing a directory, it's probably because your disk is failing and your log is gonna have a LOT of errors alerting you to that fact (assuming your system is usable at all).
There was a problem hiding this comment.
We cannot know what error happens in advance. It's always better to prepare for it.
Actually, if the error means the closing directory/file doesn't exist, the user might remove it and we can ignore/warn it, but if disk drive dies, we need to log it.
We need to debug our applications remotely. Any information is helpful.
| lh.s.Close() | ||
| if e := lh.s.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } |
There was a problem hiding this comment.
These errors aren't really actionable either, but I guess we should leave that up to the caller of (HostSet).Close. We can use HostErrorSet for this.
|
I think the most important thing is to log whether an error happens regardless of they're recoverable. We're getting a lot of errors but without details, it's hard to find what happens exactly. In other words, it's worth handling all errors to make sure any strange error doesn't happen and focus on some recoverable errors. |
It might be better to catch errors instead of ignoring them.
See also: Handle Errors - CodeReviewComments