From: YenForYang Date: Tue, 14 Sep 2021 13:01:20 +0000 (-0500) Subject: Create default constructor for Client (#567) X-Git-Tag: v1.32.0~59 X-Git-Url: http://www.git.stargrave.org/?a=commitdiff_plain;h=29638d9e5deca0b0bf6b31d69c4b6b448daa6d3c;p=btrtrc.git 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 4c1b2922..d832c237 100644 --- 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 f170c9e3..084c6b66 100644 --- 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 8ad5c737..2f337958 100644 --- 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 7bb61ecd..caee9d84 100644 --- 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")