Always return errors when handshakes or dialing fails

I intend to use xerrors-style error handling for special cases.

diff --git a/client.go b/client.go
index 9941598d9c555e6b6da3970817f4b08f451f50e1..ecedccfe8f94bc73f0afbd003d80b21557e13f15 100644 (file)
--- a/client.go
+++ b/client.go
@@ -37,6 +37,7 @@ import (
        "github.com/dustin/go-humanize"
        "github.com/google/btree"
        "golang.org/x/time/rate"
+       "golang.org/x/xerrors"
 )
 
 // Clients contain zero or more Torrents. A Client manages a blocklist, the
@@ -617,16 +618,13 @@ func (cl *Client) handshakesConnection(ctx context.Context, nc net.Conn, t *Torr
        if err != nil {
                panic(err)
        }
-       ok, err = cl.initiateHandshakes(c, t)
-       if !ok {
-               c = nil
-       }
+       err = cl.initiateHandshakes(c, t)
        return
 }
 
 // Returns nil connection and nil error if no connection could be established
 // for valid reasons.
-func (cl *Client) establishOutgoingConnEx(t *Torrent, addr IpPort, obfuscatedHeader bool) (c *connection, err error) {
+func (cl *Client) establishOutgoingConnEx(t *Torrent, addr IpPort, obfuscatedHeader bool) (*connection, error) {
        ctx, cancel := context.WithTimeout(context.Background(), func() time.Duration {
                cl.rLock()
                defer cl.rUnlock()
@@ -636,14 +634,16 @@ func (cl *Client) establishOutgoingConnEx(t *Torrent, addr IpPort, obfuscatedHea
        dr := cl.dialFirst(ctx, addr.String())
        nc := dr.Conn
        if nc == nil {
-               return
-       }
-       defer func() {
-               if c == nil || err != nil {
-                       nc.Close()
+               if ctx.Err() != nil {
+                       return nil, ctx.Err()
                }
-       }()
-       return cl.handshakesConnection(ctx, nc, t, obfuscatedHeader, addr, dr.Network)
+               return nil, errors.New("dial failed")
+       }
+       c, err := cl.handshakesConnection(ctx, nc, t, obfuscatedHeader, addr, dr.Network)
+       if err != nil {
+               nc.Close()
+       }
+       return c, err
 }
 
 // Returns nil connection and nil error if no connection could be established
@@ -652,13 +652,11 @@ func (cl *Client) establishOutgoingConn(t *Torrent, addr IpPort) (c *connection,
        torrent.Add("establish outgoing connection", 1)
        obfuscatedHeaderFirst := cl.config.HeaderObfuscationPolicy.Preferred
        c, err = cl.establishOutgoingConnEx(t, addr, obfuscatedHeaderFirst)
-       if err != nil {
-               //cl.logger.Printf("error establish connection to %s (obfuscatedHeader=%t): %v", addr, obfuscatedHeaderFirst, err)
-       }
-       if c != nil {
+       if err == nil {
                torrent.Add("initiated conn with preferred header obfuscation", 1)
                return
        }
+       //cl.logger.Printf("error establishing connection to %s (obfuscatedHeader=%t): %v", addr, obfuscatedHeaderFirst, err)
        if cl.config.HeaderObfuscationPolicy.RequirePreferred {
                // We should have just tried with the preferred header obfuscation. If it was required,
                // there's nothing else to try.
@@ -666,9 +664,10 @@ func (cl *Client) establishOutgoingConn(t *Torrent, addr IpPort) (c *connection,
        }
        // Try again with encryption if we didn't earlier, or without if we did.
        c, err = cl.establishOutgoingConnEx(t, addr, !obfuscatedHeaderFirst)
-       if c != nil {
+       if err == nil {
                torrent.Add("initiated conn with fallback header obfuscation", 1)
        }
+       //cl.logger.Printf("error establishing fallback connection to %v: %v", addr, err)
        return
 }
 
@@ -688,9 +687,6 @@ func (cl *Client) outgoingConnection(t *Torrent, addr IpPort, ps peerSource) {
                }
                return
        }
-       if c == nil {
-               return
-       }
        defer c.Close()
        c.Discovery = ps
        cl.runHandshookConn(c, t)
@@ -702,9 +698,10 @@ func (cl *Client) incomingPeerPort() int {
        return cl.LocalPort()
 }
 
-func (cl *Client) initiateHandshakes(c *connection, t *Torrent) (ok bool, err error) {
+func (cl *Client) initiateHandshakes(c *connection, t *Torrent) error {
        if c.headerEncrypted {
                var rw io.ReadWriter
+               var err error
                rw, c.cryptoMethod, err = mse.InitiateHandshake(
                        struct {
                                io.Reader
@@ -716,14 +713,17 @@ func (cl *Client) initiateHandshakes(c *connection, t *Torrent) (ok bool, err er
                )
                c.setRW(rw)
                if err != nil {
-                       return
+                       return xerrors.Errorf("header obfuscation handshake: %w", err)
                }
        }
-       ih, ok, err := cl.connBTHandshake(c, &t.infoHash)
+       ih, err := cl.connBtHandshake(c, &t.infoHash)
+       if err != nil {
+               return xerrors.Errorf("bittorrent protocol handshake: %w", err)
+       }
        if ih != t.infoHash {
-               ok = false
+               return errors.New("bittorrent protocol handshake: peer infohash didn't match")
        }
-       return
+       return nil
 }
 
 // Calls f with any secret keys.
@@ -775,12 +775,9 @@ func (cl *Client) receiveHandshakes(c *connection) (t *Torrent, err error) {
                err = errors.New("connection not have required header obfuscation")
                return
        }
-       ih, ok, err := cl.connBTHandshake(c, nil)
+       ih, err := cl.connBtHandshake(c, nil)
        if err != nil {
-               err = fmt.Errorf("error during bt handshake: %s", err)
-               return
-       }
-       if !ok {
+               err = xerrors.Errorf("during bt handshake: %w", err)
                return
        }
        cl.lock()
@@ -789,10 +786,9 @@ func (cl *Client) receiveHandshakes(c *connection) (t *Torrent, err error) {
        return
 }
 
-// Returns !ok if handshake failed for valid reasons.
-func (cl *Client) connBTHandshake(c *connection, ih *metainfo.Hash) (ret metainfo.Hash, ok bool, err error) {
-       res, ok, err := pp.Handshake(c.rw(), ih, cl.peerID, cl.extensionBytes)
-       if err != nil || !ok {
+func (cl *Client) connBtHandshake(c *connection, ih *metainfo.Hash) (ret metainfo.Hash, err error) {
+       res, err := pp.Handshake(c.rw(), ih, cl.peerID, cl.extensionBytes)
+       if err != nil {
                return
        }
        ret = res.Hash

diff --git a/go.mod b/go.mod
index cda6b77f2e4425701a18350b704beed72a5b4c98..80b1aa2c3c1352795e883c89d19ea2c67ce1f9a4 100644 (file)
--- a/go.mod
+++ b/go.mod
@@ -34,6 +34,7 @@ require (
        golang.org/x/net v0.0.0-20190628185345-da137c7871d7
        golang.org/x/sys v0.0.0-20190712062909-fae7ac547cb7 // indirect
        golang.org/x/time v0.0.0-20190308202827-9d24e82272b4
+       golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7
 )
 
 go 1.13

diff --git a/go.sum b/go.sum
index 17f99ebce00a20d0219a96c191e0170cb44115ec..0b820a0c0fc65c9e2335dd5def3ea10dc0062382 100644 (file)
--- a/go.sum
+++ b/go.sum
@@ -189,3 +189,5 @@ golang.org/x/time v0.0.0-20181108054448-85acf8d2951c h1:fqgJT0MGcGpPgpWU7VRdRjuA
 golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4 h1:SvFZT6jyqRaOeXpc5h/JSfZenJ2O330aBsf7JfSUXmQ=
 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
+golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7 h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc=
+golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

diff --git a/peer_protocol/handshake.go b/peer_protocol/handshake.go
index b8cff0ff00761a122d0ea450ac7a785175520586..d165f7a0e726cdf07db03161929e77f16b9d7702 100644 (file)
--- a/peer_protocol/handshake.go
+++ b/peer_protocol/handshake.go
@@ -5,6 +5,8 @@ import (
        "fmt"
        "io"
 
+       "golang.org/x/xerrors"
+
        "github.com/anacrolix/missinggo"
        "github.com/anacrolix/torrent/metainfo"
 )
@@ -75,7 +77,7 @@ type HandshakeResult struct {
 func Handshake(
        sock io.ReadWriter, ih *metainfo.Hash, peerID [20]byte, extensions PeerExtensionBits,
 ) (
-       res HandshakeResult, ok bool, err error,
+       res HandshakeResult, err error,
 ) {
        // Bytes to be sent to the peer. Should never block the sender.
        postCh := make(chan []byte, 4)
@@ -86,11 +88,8 @@ func Handshake(
 
        defer func() {
                close(postCh) // Done writing.
-               if !ok {
-                       return
-               }
                if err != nil {
-                       panic(err)
+                       return
                }
                // Wait until writes complete before returning from handshake.
                err = <-writeDone
@@ -116,10 +115,11 @@ func Handshake(
        var b [68]byte
        _, err = io.ReadFull(sock, b[:68])
        if err != nil {
-               err = nil
+               err = xerrors.Errorf("while reading: %w", err)
                return
        }
        if string(b[:20]) != Protocol {
+               err = xerrors.Errorf("unexpected protocol string")
                return
        }
        missinggo.CopyExact(&res.PeerExtensionBits, b[20:28])
@@ -135,6 +135,5 @@ func Handshake(
                post(peerID[:])
        }
 
-       ok = true
        return
 }

diff --git a/torrent_test.go b/torrent_test.go
index 22646ef2005ed391084b7965be8e6ff3b704e3e6..212838b53737d850567a9dff0e028a72be773fff 100644 (file)
--- a/torrent_test.go
+++ b/torrent_test.go
@@ -183,9 +183,8 @@ func TestTorrentMetainfoIncompleteMetadata(t *testing.T) {
 
        var pex PeerExtensionBits
        pex.SetBit(pp.ExtensionBitExtended)
-       hr, ok, err := pp.Handshake(nc, &ih, [20]byte{}, pex)
+       hr, err := pp.Handshake(nc, &ih, [20]byte{}, pex)
        require.NoError(t, err)
-       assert.True(t, ok)
        assert.True(t, hr.PeerExtensionBits.GetBit(pp.ExtensionBitExtended))
        assert.EqualValues(t, cl.PeerID(), hr.PeerID)
        assert.EqualValues(t, ih, hr.Hash)