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.

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)

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(),

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))

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")