Handle errors in renter package #91
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,11 @@ import ( | |
| "unsafe" | ||
|
|
||
| "github.com/aead/chacha20/chacha" | ||
| "github.com/hashicorp/go-multierror" | ||
| "github.com/pkg/errors" | ||
| "gitlab.com/NebulousLabs/Sia/crypto" | ||
| "lukechampine.com/frand" | ||
|
|
||
| "lukechampine.com/us/hostdb" | ||
| "lukechampine.com/us/merkle" | ||
| "lukechampine.com/us/renterhost" | ||
|
|
@@ -190,7 +192,7 @@ func NewMetaFile(mode os.FileMode, size int64, hosts []hostdb.HostPublicKey, min | |
|
|
||
| // WriteMetaFile creates a gzipped tar archive containing m's index and shards, | ||
| // and writes it to filename. The write is atomic. | ||
| func WriteMetaFile(filename string, m *MetaFile) (err error) { | ||
| // validate before writing | ||
| if err := validateShards(m.Shards); err != nil { | ||
| return errors.Wrap(err, "invalid shards") | ||
|
|
@@ -200,7 +202,11 @@ func WriteMetaFile(filename string, m *MetaFile) error { | |
| if err != nil { | ||
| return errors.Wrap(err, "could not create archive") | ||
| } | ||
| defer func() { | ||
| 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 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 |
||
| zip := gzip.NewWriter(f) | ||
| tw := tar.NewWriter(zip) | ||
|
|
||
|
|
@@ -256,12 +262,16 @@ func WriteMetaFile(filename string, m *MetaFile) error { | |
| } | ||
|
|
||
| // ReadMetaFile reads a metafile archive into memory. | ||
| func ReadMetaFile(filename string) (_ *MetaFile, err error) { | ||
| f, err := os.Open(filename) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not open archive") | ||
| } | ||
| defer func() { | ||
| 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.
|
||
| zip, err := gzip.NewReader(f) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not read gzip header") | ||
|
|
@@ -331,18 +341,26 @@ func ReadMetaFile(filename string) (*MetaFile, error) { | |
| } | ||
|
|
||
| // ReadMetaIndex reads the index of a metafile without reading any shards. | ||
| func ReadMetaIndex(filename string) (_ MetaIndex, err error) { | ||
| f, err := os.Open(filename) | ||
| if err != nil { | ||
| return MetaIndex{}, errors.Wrap(err, "could not open archive") | ||
| } | ||
| defer func() { | ||
| if e := f.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() | ||
|
There was a problem hiding this comment. same as above -- not actionable |
||
|
|
||
| zip, err := gzip.NewReader(f) | ||
| if err != nil { | ||
| return MetaIndex{}, errors.Wrap(err, "could not read gzip header") | ||
| } | ||
| defer func() { | ||
| if e := zip.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() | ||
|
|
||
|
|
||
| var index MetaIndex | ||
| tr := tar.NewReader(zip) | ||
|
|
@@ -387,18 +405,26 @@ func MetaFileCanDownload(filename string) (bool, error) { | |
|
|
||
| // readMetaFileShards reads a metafile and returns its index and the number of | ||
| // shards that represent fully-uploaded shards of the erasure-encoded file. | ||
| func readMetaFileShards(filename string) (_ MetaIndex, _ int, err error) { | ||
| f, err := os.Open(filename) | ||
| if err != nil { | ||
| return MetaIndex{}, 0, errors.Wrap(err, "could not open archive") | ||
| } | ||
| defer func() { | ||
| if e := f.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() | ||
|
|
||
|
|
||
| zip, err := gzip.NewReader(f) | ||
| if err != nil { | ||
| return MetaIndex{}, 0, errors.Wrap(err, "could not read gzip header") | ||
| } | ||
| defer func() { | ||
| if e := zip.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() | ||
|
There was a problem hiding this comment. As same as the other errors, if this fails, something really bad is going to happen (mostly physical error). There was a problem hiding this comment.
There was a problem hiding this comment. If the internal checksum is not verified, I think that is a problem... There was a problem hiding this comment. I suppose. I wouldn't guarantee it in the API, but as long as it's there, we might as well use it. I'll add it to the PR. |
||
|
|
||
| var haveIndex bool | ||
| var index MetaIndex | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,12 @@ import ( | |
| "sort" | ||
| "time" | ||
|
|
||
| "github.com/hashicorp/go-multierror" | ||
| "github.com/pkg/errors" | ||
| "gitlab.com/NebulousLabs/Sia/crypto" | ||
| "gitlab.com/NebulousLabs/Sia/modules" | ||
| "gitlab.com/NebulousLabs/Sia/types" | ||
|
|
||
| "lukechampine.com/us/ed25519hash" | ||
| "lukechampine.com/us/hostdb" | ||
| "lukechampine.com/us/renterhost" | ||
|
|
@@ -23,13 +25,17 @@ const ( | |
|
|
||
| // FormContract forms a contract with a host. The resulting contract will have | ||
| // renterPayout coins in the renter output. | ||
| func FormContract(w Wallet, tpool TransactionPool, key ed25519.PrivateKey, host hostdb.ScannedHost, renterPayout types.Currency, startHeight, endHeight types.BlockHeight) (_ ContractRevision, _ []types.Transaction, err error) { | ||
| s, err := NewUnlockedSession(host.NetAddress, host.PublicKey, 0) | ||
| if err != nil { | ||
| return ContractRevision{}, nil, err | ||
| } | ||
| s.host = host | ||
| defer func() { | ||
| if e := s.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() | ||
|
There was a problem hiding this comment. There's no reason to handle this error. If we formed the contract successfully, we don't care whether closing the session fails. If we fail for some other reason and return early, then we also don't care whether closing the session fails. There was a problem hiding this comment. If this fails and the connection keeps alive, the following lock requests will fail, won't it? There was a problem hiding this comment. This session only exists within the There was a problem hiding this comment. I might not understand it correctly but even if There was a problem hiding this comment. Ah, I see. For My suggestion is that you add some internal logging for the errors you're worried about, and observe whether any of them occur in production. I'm not keen on cluttering my code without clear benefit. EDIT: It looks like Go uses a TCP keepalive of 15 seconds by default. So even if the host gets stuck half-open, it should time out within 15 seconds. Perhaps this means the default There was a problem hiding this comment. I think even if there is a half-open socket, the host can accept another connection and then get conflict. If I'm not mistaken, a half-open connection could happen if a switch gets overflowed. Btw, don't hosts change the default keepalive? |
||
| return s.FormContract(w, tpool, key, renterPayout, startHeight, endHeight) | ||
| } | ||
|
|
||
|
|
@@ -168,7 +174,10 @@ func (s *Session) FormContract(w Wallet, tpool TransactionPool, key ed25519.Priv | |
| err = w.SignTransaction(&txn, toSign) | ||
| if err != nil { | ||
| err = errors.Wrap(err, "failed to sign transaction") | ||
| // don't want to reveal too much | ||
| if e := s.sess.WriteResponse(nil, errors.New("internal error")); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
|
There was a problem hiding this comment. We don't care if the host received this message or not. If they did, great. If they didn't, that's unfortunate, but we're not going to attempt to resend it or anything -- the connection is being closed. There was a problem hiding this comment. We should log that the host doesn't receive any request, shouldn't we? There was a problem hiding this comment. I wouldn't recommend it. You should only punish a host for errors that prevent you from completing an RPC. |
||
| return ContractRevision{}, nil, err | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,10 +13,12 @@ import ( | |
| "sort" | ||
| "time" | ||
|
|
||
| "github.com/hashicorp/go-multierror" | ||
| "github.com/pkg/errors" | ||
| "gitlab.com/NebulousLabs/Sia/crypto" | ||
| "gitlab.com/NebulousLabs/Sia/modules" | ||
| "gitlab.com/NebulousLabs/Sia/types" | ||
|
|
||
| "lukechampine.com/us/ed25519hash" | ||
| "lukechampine.com/us/hostdb" | ||
| "lukechampine.com/us/merkle" | ||
|
|
@@ -384,7 +386,11 @@ func (s *Session) Read(w io.Writer, sections []renterhost.RPCReadRequestSection) | |
|
|
||
| // host will now stream back responses; ensure we send RPCLoopReadStop | ||
| // before returning | ||
| defer func() { | ||
| if e := s.sess.WriteResponse(&renterhost.RPCReadStop, nil); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
| }() | ||
|
There was a problem hiding this comment. This one is a little more subtle. In this case, it's true that we're sending a message to the host, and it's not an error message that we're sending immediately before we close the connection. However, the success of this RPC does not depend on this message being sent. If we receive all of the data from the host, but then fail to send |
||
| var hostSig []byte | ||
| for _, sec := range sections { | ||
| // NOTE: normally, we would call ReadResponse here to read an AEAD RPC | ||
|
|
@@ -577,7 +583,9 @@ func (s *Session) Write(actions []renterhost.RPCWriteAction) (err error) { | |
| <-precompChan | ||
| if newFileSize > 0 && !merkle.VerifyDiffProof(actions, s.rev.NumSectors(), proofHashes, leafHashes, oldRoot, newRoot, s.appendRoots) { | ||
| err := ErrInvalidMerkleProof | ||
| if e := s.sess.WriteResponse(nil, err); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
|
|
||
| return err | ||
| } | ||
|
|
||
|
|
@@ -701,11 +709,15 @@ func NewSession(hostIP modules.NetAddress, hostKey hostdb.HostPublicKey, id type | |
| return nil, err | ||
| } | ||
| if err := s.Lock(id, key); err != nil { | ||
| if e := s.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
|
|
||
| return nil, err | ||
| } | ||
| if _, err := s.Settings(); err != nil { | ||
| if e := s.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
|
|
||
| return nil, err | ||
| } | ||
| return s, nil | ||
|
|
@@ -727,10 +739,15 @@ func newUnlockedSession(hostIP modules.NetAddress, hostKey hostdb.HostPublicKey, | |
| } | ||
| latency := time.Since(start) | ||
| conn := &statsConn{Conn: tcpConn} | ||
| 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, However, it's possible that the implementation of |
||
| s, err := renterhost.NewRenterSession(conn, hostKey.Ed25519()) | ||
| if err != nil { | ||
| if e := conn.Close(); e != nil { | ||
| err = multierror.Append(err, e) | ||
| } | ||
|
|
||
| return nil, err | ||
| } | ||
| return &Session{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't add new dependencies lightly. This one does not strike me as necessary.