From 15651a931ed1670dd2828fecf1b934f8f151d49a Mon Sep 17 00:00:00 2001 From: Zach Ramsay Date: Thu, 21 Sep 2017 12:38:48 -0400 Subject: [PATCH] linting errors: tackle p2p package --- p2p/addrbook.go | 6 ++-- p2p/connection.go | 6 ++-- p2p/connection_test.go | 62 ++++++++++++++++++++++++++++------- p2p/fuzz.go | 2 +- p2p/listener.go | 11 +++++-- p2p/listener_test.go | 7 +++- p2p/peer.go | 4 ++- p2p/peer_set_test.go | 8 +++-- p2p/peer_test.go | 14 +++++--- p2p/pex_reactor.go | 9 +++-- p2p/pex_reactor_test.go | 30 ++++++++++++++--- p2p/secret_connection.go | 4 +-- p2p/secret_connection_test.go | 20 ++++++++--- p2p/switch.go | 18 +++++++--- p2p/switch_test.go | 26 +++++++++++---- p2p/upnp/upnp.go | 2 +- 16 files changed, 177 insertions(+), 52 deletions(-) diff --git a/p2p/addrbook.go b/p2p/addrbook.go index 0b3301060..4b88fdf6b 100644 --- a/p2p/addrbook.go +++ b/p2p/addrbook.go @@ -130,7 +130,9 @@ func (a *AddrBook) init() { // OnStart implements Service. func (a *AddrBook) OnStart() error { - a.BaseService.OnStart() + if err := a.BaseService.OnStart(); err != nil { + return err + } a.loadFromFile(a.filePath) // wg.Add to ensure that any invocation of .Wait() @@ -369,7 +371,7 @@ func (a *AddrBook) loadFromFile(filePath string) bool { if err != nil { cmn.PanicCrisis(cmn.Fmt("Error opening file %s: %v", filePath, err)) } - defer r.Close() + defer r.Close() // nolint (errcheck) aJSON := &addrBookJSON{} dec := json.NewDecoder(r) err = dec.Decode(aJSON) diff --git a/p2p/connection.go b/p2p/connection.go index 5e4845536..290029423 100644 --- a/p2p/connection.go +++ b/p2p/connection.go @@ -163,7 +163,9 @@ func NewMConnectionWithConfig(conn net.Conn, chDescs []*ChannelDescriptor, onRec // OnStart implements BaseService func (c *MConnection) OnStart() error { - c.BaseService.OnStart() + if err := c.BaseService.OnStart(); err != nil { + return err + } c.quit = make(chan struct{}) c.flushTimer = cmn.NewThrottleTimer("flush", c.config.flushThrottle) c.pingTimer = cmn.NewRepeatTimer("ping", pingTimeout) @@ -182,7 +184,7 @@ func (c *MConnection) OnStop() { if c.quit != nil { close(c.quit) } - c.conn.Close() + c.conn.Close() // nolint (errcheck) // We can't close pong safely here because // recvRoutine may write to it after we've stopped. // Though it doesn't need to get closed at all, diff --git a/p2p/connection_test.go b/p2p/connection_test.go index d74deabfa..b530a0091 100644 --- a/p2p/connection_test.go +++ b/p2p/connection_test.go @@ -32,8 +32,16 @@ func TestMConnectionSend(t *testing.T) { assert, require := assert.New(t), require.New(t) server, client := netPipe() - defer server.Close() - defer client.Close() + defer func() { + if err := server.Close(); err != nil { + t.Error(err) + } + }() + defer func() { + if err := client.Close(); err != nil { + t.Error(err) + } + }() mconn := createTestMConnection(client) _, err := mconn.Start() @@ -44,12 +52,18 @@ func TestMConnectionSend(t *testing.T) { assert.True(mconn.Send(0x01, msg)) // Note: subsequent Send/TrySend calls could pass because we are reading from // the send queue in a separate goroutine. - server.Read(make([]byte, len(msg))) + _, err = server.Read(make([]byte, len(msg))) + if err != nil { + t.Error(err) + } assert.True(mconn.CanSend(0x01)) msg = "Spider-Man" assert.True(mconn.TrySend(0x01, msg)) - server.Read(make([]byte, len(msg))) + _, err = server.Read(make([]byte, len(msg))) + if err != nil { + t.Error(err) + } assert.False(mconn.CanSend(0x05), "CanSend should return false because channel is unknown") assert.False(mconn.Send(0x05, "Absorbing Man"), "Send should return false because channel is unknown") @@ -59,8 +73,16 @@ func TestMConnectionReceive(t *testing.T) { assert, require := assert.New(t), require.New(t) server, client := netPipe() - defer server.Close() - defer client.Close() + defer func() { + if err := server.Close(); err != nil { + t.Error(err) + } + }() + defer func() { + if err := client.Close(); err != nil { + t.Error(err) + } + }() receivedCh := make(chan []byte) errorsCh := make(chan interface{}) @@ -97,8 +119,16 @@ func TestMConnectionStatus(t *testing.T) { assert, require := assert.New(t), require.New(t) server, client := netPipe() - defer server.Close() - defer client.Close() + defer func() { + if err := server.Close(); err != nil { + t.Error(err) + } + }() + defer func() { + if err := client.Close(); err != nil { + t.Error(err) + } + }() mconn := createTestMConnection(client) _, err := mconn.Start() @@ -114,8 +144,16 @@ func TestMConnectionStopsAndReturnsError(t *testing.T) { assert, require := assert.New(t), require.New(t) server, client := netPipe() - defer server.Close() - defer client.Close() + defer func() { + if err := server.Close(); err != nil { + t.Error(err) + } + }() + defer func() { + if err := client.Close(); err != nil { + t.Error(err) + } + }() receivedCh := make(chan []byte) errorsCh := make(chan interface{}) @@ -130,7 +168,9 @@ func TestMConnectionStopsAndReturnsError(t *testing.T) { require.Nil(err) defer mconn.Stop() - client.Close() + if err := client.Close(); err != nil { + t.Error(err) + } select { case receivedBytes := <-receivedCh: diff --git a/p2p/fuzz.go b/p2p/fuzz.go index aefac986a..26a9e10dc 100644 --- a/p2p/fuzz.go +++ b/p2p/fuzz.go @@ -143,7 +143,7 @@ func (fc *FuzzedConnection) fuzz() bool { } else if r < fc.config.ProbDropRW+fc.config.ProbDropConn { // XXX: can't this fail because machine precision? // XXX: do we need an error? - fc.Close() + fc.Close() // nolint (errcheck) return true } else if r < fc.config.ProbDropRW+fc.config.ProbDropConn+fc.config.ProbSleep { time.Sleep(fc.randomDuration()) diff --git a/p2p/listener.go b/p2p/listener.go index 971390974..5b5f60a4a 100644 --- a/p2p/listener.go +++ b/p2p/listener.go @@ -100,19 +100,24 @@ func NewDefaultListener(protocol string, lAddr string, skipUPNP bool, logger log connections: make(chan net.Conn, numBufferedConnections), } dl.BaseService = *cmn.NewBaseService(logger, "DefaultListener", dl) - dl.Start() // Started upon construction + _, err = dl.Start() // Started upon construction + if err != nil { + logger.Error("Error starting base service", "err", err) + } return dl } func (l *DefaultListener) OnStart() error { - l.BaseService.OnStart() + if err := l.BaseService.OnStart(); err != nil { + return err + } go l.listenRoutine() return nil } func (l *DefaultListener) OnStop() { l.BaseService.OnStop() - l.listener.Close() + l.listener.Close() // nolint (errcheck) } // Accept connections and pass on the channel diff --git a/p2p/listener_test.go b/p2p/listener_test.go index c3d33a9ae..92018e0aa 100644 --- a/p2p/listener_test.go +++ b/p2p/listener_test.go @@ -25,7 +25,12 @@ func TestListener(t *testing.T) { } msg := []byte("hi!") - go connIn.Write(msg) + go func() { + _, err := connIn.Write(msg) + if err != nil { + t.Error(err) + } + }() b := make([]byte, 32) n, err := connOut.Read(b) if err != nil { diff --git a/p2p/peer.go b/p2p/peer.go index ec8349551..1d84eb284 100644 --- a/p2p/peer.go +++ b/p2p/peer.go @@ -230,7 +230,9 @@ func (p *peer) PubKey() crypto.PubKeyEd25519 { // OnStart implements BaseService. func (p *peer) OnStart() error { - p.BaseService.OnStart() + if err := p.BaseService.OnStart(); err != nil { + return err + } _, err := p.mconn.Start() return err } diff --git a/p2p/peer_set_test.go b/p2p/peer_set_test.go index e37455256..694300523 100644 --- a/p2p/peer_set_test.go +++ b/p2p/peer_set_test.go @@ -28,7 +28,9 @@ func TestPeerSetAddRemoveOne(t *testing.T) { var peerList []Peer for i := 0; i < 5; i++ { p := randPeer() - peerSet.Add(p) + if err := peerSet.Add(p); err != nil { + t.Error(err) + } peerList = append(peerList, p) } @@ -48,7 +50,9 @@ func TestPeerSetAddRemoveOne(t *testing.T) { // 2. Next we are testing removing the peer at the end // a) Replenish the peerSet for _, peer := range peerList { - peerSet.Add(peer) + if err := peerSet.Add(peer); err != nil { + t.Error(err) + } } // b) In reverse, remove each element diff --git a/p2p/peer_test.go b/p2p/peer_test.go index a027a6b71..b2a01493d 100644 --- a/p2p/peer_test.go +++ b/p2p/peer_test.go @@ -23,7 +23,8 @@ func TestPeerBasic(t *testing.T) { p, err := createOutboundPeerAndPerformHandshake(rp.Addr(), DefaultPeerConfig()) require.Nil(err) - p.Start() + _, err = p.Start() + require.Nil(err) defer p.Stop() assert.True(p.IsRunning()) @@ -49,7 +50,8 @@ func TestPeerWithoutAuthEnc(t *testing.T) { p, err := createOutboundPeerAndPerformHandshake(rp.Addr(), config) require.Nil(err) - p.Start() + _, err = p.Start() + require.Nil(err) defer p.Stop() assert.True(p.IsRunning()) @@ -69,7 +71,9 @@ func TestPeerSend(t *testing.T) { p, err := createOutboundPeerAndPerformHandshake(rp.Addr(), config) require.Nil(err) - p.Start() + _, err = p.Start() + require.Nil(err) + defer p.Stop() assert.True(p.CanSend(0x01)) @@ -148,7 +152,9 @@ func (p *remotePeer) accept(l net.Listener) { } select { case <-p.quit: - conn.Close() + if err := conn.Close(); err != nil { + golog.Fatal(err) + } return default: } diff --git a/p2p/pex_reactor.go b/p2p/pex_reactor.go index 7c799ccae..71c3beb0d 100644 --- a/p2p/pex_reactor.go +++ b/p2p/pex_reactor.go @@ -66,8 +66,13 @@ func NewPEXReactor(b *AddrBook) *PEXReactor { // OnStart implements BaseService func (r *PEXReactor) OnStart() error { - r.BaseReactor.OnStart() - r.book.Start() + if err := r.BaseReactor.OnStart(); err != nil { + return err + } + _, err := r.book.Start() + if err != nil { + return err + } go r.ensurePeersRoutine() go r.flushMsgCountByPeer() return nil diff --git a/p2p/pex_reactor_test.go b/p2p/pex_reactor_test.go index 3efc3c643..d34777e1d 100644 --- a/p2p/pex_reactor_test.go +++ b/p2p/pex_reactor_test.go @@ -20,7 +20,11 @@ func TestPEXReactorBasic(t *testing.T) { dir, err := ioutil.TempDir("", "pex_reactor") require.Nil(err) - defer os.RemoveAll(dir) + defer func() { + if err := os.RemoveAll(dir); err != nil { + t.Error(err) + } + }() book := NewAddrBook(dir+"addrbook.json", true) book.SetLogger(log.TestingLogger()) @@ -36,7 +40,11 @@ func TestPEXReactorAddRemovePeer(t *testing.T) { dir, err := ioutil.TempDir("", "pex_reactor") require.Nil(err) - defer os.RemoveAll(dir) + defer func() { + if err := os.RemoveAll(dir); err != nil { + t.Error(err) + } + }() book := NewAddrBook(dir+"addrbook.json", true) book.SetLogger(log.TestingLogger()) @@ -69,7 +77,11 @@ func TestPEXReactorRunning(t *testing.T) { dir, err := ioutil.TempDir("", "pex_reactor") require.Nil(err) - defer os.RemoveAll(dir) + defer func() { + if err := os.RemoveAll(dir); err != nil { + t.Error(err) + } + }() book := NewAddrBook(dir+"addrbook.json", false) book.SetLogger(log.TestingLogger()) @@ -139,7 +151,11 @@ func TestPEXReactorReceive(t *testing.T) { dir, err := ioutil.TempDir("", "pex_reactor") require.Nil(err) - defer os.RemoveAll(dir) + defer func() { + if err := os.RemoveAll(dir); err != nil { + t.Error(err) + } + }() book := NewAddrBook(dir+"addrbook.json", false) book.SetLogger(log.TestingLogger()) @@ -164,7 +180,11 @@ func TestPEXReactorAbuseFromPeer(t *testing.T) { dir, err := ioutil.TempDir("", "pex_reactor") require.Nil(err) - defer os.RemoveAll(dir) + defer func() { + if err := os.RemoveAll(dir); err != nil { + t.Error(err) + } + }() book := NewAddrBook(dir+"addrbook.json", true) book.SetLogger(log.TestingLogger()) diff --git a/p2p/secret_connection.go b/p2p/secret_connection.go index 0e107ea59..f034b4c08 100644 --- a/p2p/secret_connection.go +++ b/p2p/secret_connection.go @@ -302,7 +302,7 @@ func shareAuthSignature(sc *SecretConnection, pubKey crypto.PubKeyEd25519, signa // sha256 func hash32(input []byte) (res *[32]byte) { hasher := sha256.New() - hasher.Write(input) // does not error + _, _ = hasher.Write(input) // ignoring error resSlice := hasher.Sum(nil) res = new([32]byte) copy(res[:], resSlice) @@ -312,7 +312,7 @@ func hash32(input []byte) (res *[32]byte) { // We only fill in the first 20 bytes with ripemd160 func hash24(input []byte) (res *[24]byte) { hasher := ripemd160.New() - hasher.Write(input) // does not error + _, _ = hasher.Write(input) // ignoring error resSlice := hasher.Sum(nil) res = new([24]byte) copy(res[:], resSlice) diff --git a/p2p/secret_connection_test.go b/p2p/secret_connection_test.go index d0d008529..8b58fb417 100644 --- a/p2p/secret_connection_test.go +++ b/p2p/secret_connection_test.go @@ -70,8 +70,12 @@ func makeSecretConnPair(tb testing.TB) (fooSecConn, barSecConn *SecretConnection func TestSecretConnectionHandshake(t *testing.T) { fooSecConn, barSecConn := makeSecretConnPair(t) - fooSecConn.Close() - barSecConn.Close() + if err := fooSecConn.Close(); err != nil { + t.Error(err) + } + if err := barSecConn.Close(); err != nil { + t.Error(err) + } } func TestSecretConnectionReadWrite(t *testing.T) { @@ -110,7 +114,9 @@ func TestSecretConnectionReadWrite(t *testing.T) { return } } - nodeConn.PipeWriter.Close() + if err := nodeConn.PipeWriter.Close(); err != nil { + t.Error(err) + } }, func() { // Node reads @@ -125,7 +131,9 @@ func TestSecretConnectionReadWrite(t *testing.T) { } *nodeReads = append(*nodeReads, string(readBuffer[:n])) } - nodeConn.PipeReader.Close() + if err := nodeConn.PipeReader.Close(); err != nil { + t.Error(err) + } }) } } @@ -197,6 +205,8 @@ func BenchmarkSecretConnection(b *testing.B) { } b.StopTimer() - fooSecConn.Close() + if err := fooSecConn.Close(); err != nil { + b.Error(err) + } //barSecConn.Close() race condition } diff --git a/p2p/switch.go b/p2p/switch.go index be51d561d..4e22521ae 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -174,7 +174,9 @@ func (sw *Switch) SetNodePrivKey(nodePrivKey crypto.PrivKeyEd25519) { // OnStart implements BaseService. It starts all the reactors, peers, and listeners. func (sw *Switch) OnStart() error { - sw.BaseService.OnStart() + if err := sw.BaseService.OnStart(); err != nil { + return err + } // Start reactors for _, reactor := range sw.reactors { _, err := reactor.Start() @@ -287,7 +289,11 @@ func (sw *Switch) SetPubKeyFilter(f func(crypto.PubKeyEd25519) error) { } func (sw *Switch) startInitPeer(peer *peer) { - peer.Start() // spawn send/recv routines + _, err := peer.Start() // spawn send/recv routines + if err != nil { + sw.Logger.Error("Error starting peer", "err", err) + } + for _, reactor := range sw.reactors { reactor.AddPeer(peer) } @@ -568,7 +574,9 @@ func makeSwitch(cfg *cfg.P2PConfig, i int, network, version string, initSwitch f func (sw *Switch) addPeerWithConnection(conn net.Conn) error { peer, err := newInboundPeer(conn, sw.reactorsByCh, sw.chDescs, sw.StopPeerForError, sw.nodePrivKey, sw.peerConfig) if err != nil { - conn.Close() + if err := conn.Close(); err != nil { + sw.Logger.Error("Error closing connection", "err", err) + } return err } peer.SetLogger(sw.Logger.With("peer", conn.RemoteAddr())) @@ -583,7 +591,9 @@ func (sw *Switch) addPeerWithConnection(conn net.Conn) error { func (sw *Switch) addPeerWithConnectionAndConfig(conn net.Conn, config *PeerConfig) error { peer, err := newInboundPeer(conn, sw.reactorsByCh, sw.chDescs, sw.StopPeerForError, sw.nodePrivKey, config) if err != nil { - conn.Close() + if err := conn.Close(); err != nil { + sw.Logger.Error("Error closing connection", "err", err) + } return err } peer.SetLogger(sw.Logger.With("peer", conn.RemoteAddr())) diff --git a/p2p/switch_test.go b/p2p/switch_test.go index e82eead9c..fb179efe1 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -171,10 +171,14 @@ func TestConnAddrFilter(t *testing.T) { // connect to good peer go func() { - s1.addPeerWithConnection(c1) + if err := s1.addPeerWithConnection(c1); err != nil { + // t.Error(err) FIXME: fails + } }() go func() { - s2.addPeerWithConnection(c2) + if err := s2.addPeerWithConnection(c2); err != nil { + // t.Error(err) FIXME: fails + } }() assertNoPeersAfterTimeout(t, s1, 400*time.Millisecond) @@ -206,10 +210,14 @@ func TestConnPubKeyFilter(t *testing.T) { // connect to good peer go func() { - s1.addPeerWithConnection(c1) + if err := s1.addPeerWithConnection(c1); err != nil { + // t.Error(err) FIXME: fails + } }() go func() { - s2.addPeerWithConnection(c2) + if err := s2.addPeerWithConnection(c2); err != nil { + // t.Error(err) FIXME: fails + } }() assertNoPeersAfterTimeout(t, s1, 400*time.Millisecond) @@ -220,7 +228,10 @@ func TestSwitchStopsNonPersistentPeerOnError(t *testing.T) { assert, require := assert.New(t), require.New(t) sw := makeSwitch(config, 1, "testing", "123.123.123", initSwitchFunc) - sw.Start() + _, err := sw.Start() + if err != nil { + t.Error(err) + } defer sw.Stop() // simulate remote peer @@ -244,7 +255,10 @@ func TestSwitchReconnectsToPersistentPeer(t *testing.T) { assert, require := assert.New(t), require.New(t) sw := makeSwitch(config, 1, "testing", "123.123.123", initSwitchFunc) - sw.Start() + _, err := sw.Start() + if err != nil { + t.Error(err) + } defer sw.Stop() // simulate remote peer diff --git a/p2p/upnp/upnp.go b/p2p/upnp/upnp.go index 328d86f4a..81e1f7a36 100644 --- a/p2p/upnp/upnp.go +++ b/p2p/upnp/upnp.go @@ -197,7 +197,7 @@ func getServiceURL(rootURL string) (url, urnDomain string, err error) { if err != nil { return } - defer r.Body.Close() + defer r.Body.Close() // nolint (errcheck) if r.StatusCode >= 400 { err = errors.New(string(r.StatusCode))