Create default constructor for Client (#567)

Allow for certain values to always be properly initialized on construction -- namely the maps for now. I'm currently working on a change that requires a baseline constructor; this change would make the use of `chansync.BroadcastCond` and `chansync.SetOnce` obsolete -- i.e. one can have channel members without worrying about proper initialization/destruction of a `chan struct{}`.

As for why `makeClient` returns a value instead of a pointer: returning a value gives us more options -- you can always take a pointer from a value later on cheaply, and have things moved to the heap if they weren't already. The same can't be said about getting a value back from a pointer. GC also could potentially have less work to do. Plus I personally find ownership to be an important concept (semi-borrowed from rust) -- use of values make ownership clear.

diff --git a/client.go b/client.go
index 4c1b292245d283506ede19a522524f7129d2e692..d832c237d268a1295640fe9d849cbe3f120afcc9 100644 (file)
--- a/client.go
+++ b/client.go
@@ -191,32 +191,35 @@ func (cl *Client) announceKey() int32 {
        return int32(binary.BigEndian.Uint32(cl.peerID[16:20]))
 }
 
+// Initializes a bare minimum Client. *Client and *ClientConfig must not be nil.
+func (cl *Client) init(cfg *ClientConfig) {
+       cl.config = cfg
+       cl.dopplegangerAddrs = make(map[string]struct{})
+       cl.torrents = make(map[metainfo.Hash]*Torrent)
+       cl.dialRateLimiter = rate.NewLimiter(10, 10)
+       cl.activeAnnounceLimiter.SlotsPerKey = 2
+       
+       cl.event.L = cl.locker()
+       cl.ipBlockList = cfg.IPBlocklist
+}
+
 func NewClient(cfg *ClientConfig) (cl *Client, err error) {
        if cfg == nil {
                cfg = NewDefaultClientConfig()
                cfg.ListenPort = 0
        }
-       defer func() {
-               if err != nil {
-                       cl = nil
-               }
-       }()
-       cl = &Client{
-               config:            cfg,
-               dopplegangerAddrs: make(map[string]struct{}),
-               torrents:          make(map[metainfo.Hash]*Torrent),
-               dialRateLimiter:   rate.NewLimiter(10, 10),
-       }
-       cl.activeAnnounceLimiter.SlotsPerKey = 2
+       var client Client
+       client.init(cfg)
+       cl = &client
        go cl.acceptLimitClearer()
        cl.initLogger()
        defer func() {
-               if err == nil {
-                       return
+               if err != nil {
+                       cl.Close()
+                       cl = nil
                }
-               cl.Close()
        }()
-       cl.event.L = cl.locker()
+       
        storageImpl := cfg.DefaultStorage
        if storageImpl == nil {
                // We'd use mmap by default but HFS+ doesn't support sparse files.
@@ -229,9 +232,6 @@ func NewClient(cfg *ClientConfig) (cl *Client, err error) {
                storageImpl = storageImplCloser
        }
        cl.defaultStorage = storage.NewClient(storageImpl)
-       if cfg.IPBlocklist != nil {
-               cl.ipBlockList = cfg.IPBlocklist
-       }
 
        if cfg.PeerID != "" {
                missinggo.CopyExact(&cl.peerID, cfg.PeerID)

diff --git a/client_test.go b/client_test.go
index f170c9e3f60f70ef891c07ab6c04a69a32a1acda..084c6b666f7ec467d6805092250f7b717aa34784 100644 (file)
--- a/client_test.go
+++ b/client_test.go
@@ -79,9 +79,8 @@ func TestPieceHashSize(t *testing.T) {
 func TestTorrentInitialState(t *testing.T) {
        dir, mi := testutil.GreetingTestTorrent()
        defer os.RemoveAll(dir)
-       cl := &Client{
-               config: TestingConfig(t),
-       }
+       var cl Client
+       cl.init(TestingConfig(t))
        cl.initLogger()
        tor := cl.newTorrent(
                mi.HashInfoBytes(),

diff --git a/peerconn_test.go b/peerconn_test.go
index 8ad5c737e177ad985286ce6df048d63772f8321c..2f337958a6f9a2e1483c7731fb8f0b1787037b7b 100644 (file)
--- a/peerconn_test.go
+++ b/peerconn_test.go
@@ -18,15 +18,14 @@ import (
 // Ensure that no race exists between sending a bitfield, and a subsequent
 // Have that would potentially alter it.
 func TestSendBitfieldThenHave(t *testing.T) {
-       cl := Client{
-               config: TestingConfig(t),
-       }
+       var cl Client
+       cl.init(TestingConfig(t))
        cl.initLogger()
        c := cl.newConnection(nil, false, nil, "io.Pipe", "")
        c.setTorrent(cl.newTorrent(metainfo.Hash{}, nil))
-       c.t.setInfo(&metainfo.Info{
-               Pieces: make([]byte, metainfo.HashSize*3),
-       })
+       if err := c.t.setInfo(&metainfo.Info{ Pieces: make([]byte, metainfo.HashSize*3) }); err != nil {
+               t.Log(err)
+       }
        r, w := io.Pipe()
        //c.r = r
        c.w = w
@@ -87,15 +86,14 @@ func (me *torrentStorage) WriteAt(b []byte, _ int64) (int, error) {
 
 func BenchmarkConnectionMainReadLoop(b *testing.B) {
        c := quicktest.New(b)
-       cl := &Client{
-               config: &ClientConfig{
-                       DownloadRateLimiter: unlimited,
-               },
-       }
+       var cl Client
+       cl.init(&ClientConfig{
+               DownloadRateLimiter: unlimited,
+       })
        cl.initLogger()
        ts := &torrentStorage{}
        t := &Torrent{
-               cl:                cl,
+               cl:                &cl,
                storage:           &storage.Torrent{TorrentImpl: storage.TorrentImpl{Piece: ts.Piece, Close: ts.Close}},
                pieceStateChanges: pubsub.NewPubSub(),
        }
@@ -125,7 +123,6 @@ func BenchmarkConnectionMainReadLoop(b *testing.B) {
        wb := msg.MustMarshalBinary()
        b.SetBytes(int64(len(msg.Piece)))
        go func() {
-               defer w.Close()
                ts.writeSem.Lock()
                for i := 0; i < b.N; i += 1 {
                        cl.lock()
@@ -139,6 +136,9 @@ func BenchmarkConnectionMainReadLoop(b *testing.B) {
                        require.EqualValues(b, len(wb), n)
                        ts.writeSem.Lock()
                }
+               if err := w.Close(); err != nil {
+                       panic(err)
+               }
        }()
        c.Assert([]error{nil, io.EOF}, quicktest.Contains, <-mrlErr)
        c.Assert(cn._stats.ChunksReadUseful.Int64(), quicktest.Equals, int64(b.N))

diff --git a/pexconn_test.go b/pexconn_test.go
index 7bb61ecdaaccb6461ae384f367e5387486ff94af..caee9d849fd4d601661eb38ce8a1cff5b242dc57 100644 (file)
--- a/pexconn_test.go
+++ b/pexconn_test.go
@@ -12,9 +12,8 @@ import (
 )
 
 func TestPexConnState(t *testing.T) {
-       cl := Client{
-               config: TestingConfig(t),
-       }
+       var cl Client
+       cl.init(TestingConfig(t))
        cl.initLogger()
        torrent := cl.newTorrent(metainfo.Hash{}, nil)
        addr := &net.TCPAddr{IP: net.IPv6loopback, Port: 4747}
@@ -23,7 +22,9 @@ func TestPexConnState(t *testing.T) {
        c.PeerExtensionIDs[pp.ExtensionNamePex] = pexExtendedId
        c.messageWriter.mu.Lock()
        c.setTorrent(torrent)
-       torrent.addPeerConn(c)
+       if err := torrent.addPeerConn(c); err != nil {
+               t.Log(err)
+       }
 
        c.pex.Init(c)
        require.True(t, c.pex.IsEnabled(), "should get enabled")