From 63ce10109888c6eb0376184e8092a4dc6dabc056 Mon Sep 17 00:00:00 2001
From: Luca Moser <moser.luca@gmail.com>
Date: Fri, 7 Feb 2020 10:19:31 +0100
Subject: [PATCH] Log connection failed errors (#238)

* fixes locks not getting unlocked

* only hold the lock in DropNeighbor while deleted the neighbor entry

* log connection failed error

* fixes tests
---
 packages/gossip/events.go       |  6 +++++-
 packages/gossip/manager.go      | 12 +++++++++---
 packages/gossip/manager_test.go |  4 ++--
 packages/gossip/neighbor.go     |  2 +-
 plugins/autopeering/plugin.go   |  2 +-
 plugins/gossip/plugin.go        |  4 ++--
 6 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/packages/gossip/events.go b/packages/gossip/events.go
index 169ec219..5f96044a 100644
--- a/packages/gossip/events.go
+++ b/packages/gossip/events.go
@@ -16,7 +16,7 @@ var Events = struct {
 	// A TransactionReceived event is triggered when a new transaction is received by the gossip protocol.
 	TransactionReceived *events.Event
 }{
-	ConnectionFailed:    events.NewEvent(peerCaller),
+	ConnectionFailed:    events.NewEvent(peerAndErrorCaller),
 	NeighborAdded:       events.NewEvent(neighborCaller),
 	NeighborRemoved:     events.NewEvent(peerCaller),
 	TransactionReceived: events.NewEvent(transactionReceived),
@@ -27,6 +27,10 @@ type TransactionReceivedEvent struct {
 	Peer *peer.Peer // peer that send the transaction
 }
 
+func peerAndErrorCaller(handler interface{}, params ...interface{}) {
+	handler.(func(*peer.Peer, error))(params[0].(*peer.Peer), params[1].(error))
+}
+
 func peerCaller(handler interface{}, params ...interface{}) {
 	handler.(func(*peer.Peer))(params[0].(*peer.Peer))
 }
diff --git a/packages/gossip/manager.go b/packages/gossip/manager.go
index bc3d7222..bc08ab56 100644
--- a/packages/gossip/manager.go
+++ b/packages/gossip/manager.go
@@ -1,6 +1,7 @@
 package gossip
 
 import (
+	"errors"
 	"fmt"
 	"net"
 	"sync"
@@ -17,6 +18,11 @@ const (
 	maxPacketSize = 2048
 )
 
+var (
+	ErrNeighborManagerNotRunning = errors.New("neighbor manager is not running")
+	ErrNeighborAlreadyConnected  = errors.New("neighbor is already connected")
+)
+
 // GetTransaction defines a function that returns the transaction data with the given hash.
 type GetTransaction func(txHash []byte) ([]byte, error)
 
@@ -178,7 +184,7 @@ func (m *Manager) send(b []byte, to ...peer.ID) {
 func (m *Manager) addNeighbor(peer *peer.Peer, connectorFunc func(*peer.Peer) (net.Conn, error)) error {
 	conn, err := connectorFunc(peer)
 	if err != nil {
-		Events.ConnectionFailed.Trigger(peer)
+		Events.ConnectionFailed.Trigger(peer, err)
 		return err
 	}
 
@@ -186,12 +192,12 @@ func (m *Manager) addNeighbor(peer *peer.Peer, connectorFunc func(*peer.Peer) (n
 	defer m.mu.Unlock()
 	if !m.running {
 		_ = conn.Close()
-		Events.ConnectionFailed.Trigger(peer)
+		Events.ConnectionFailed.Trigger(peer, ErrNeighborManagerNotRunning)
 		return ErrClosed
 	}
 	if _, ok := m.neighbors[peer.ID()]; ok {
 		_ = conn.Close()
-		Events.ConnectionFailed.Trigger(peer)
+		Events.ConnectionFailed.Trigger(peer, ErrNeighborAlreadyConnected)
 		return ErrDuplicateNeighbor
 	}
 
diff --git a/packages/gossip/manager_test.go b/packages/gossip/manager_test.go
index f4b25a29..07dda401 100644
--- a/packages/gossip/manager_test.go
+++ b/packages/gossip/manager_test.go
@@ -310,7 +310,7 @@ func TestDropUnsuccessfulAccept(t *testing.T) {
 	_, closeB, peerB := newTestManager(t, "B")
 	defer closeB()
 
-	e.On("connectionFailed", peerB).Once()
+	e.On("connectionFailed", peerB, mock.Anything).Once()
 
 	err := mgrA.AddInbound(peerB)
 	assert.Error(t, err)
@@ -432,7 +432,7 @@ type eventMock struct {
 	mock.Mock
 }
 
-func (e *eventMock) connectionFailed(p *peer.Peer)                    { e.Called(p) }
+func (e *eventMock) connectionFailed(p *peer.Peer, err error)         { e.Called(p, err) }
 func (e *eventMock) neighborAdded(n *Neighbor)                        { e.Called(n) }
 func (e *eventMock) neighborRemoved(p *peer.Peer)                     { e.Called(p) }
 func (e *eventMock) transactionReceived(ev *TransactionReceivedEvent) { e.Called(ev) }
diff --git a/packages/gossip/neighbor.go b/packages/gossip/neighbor.go
index 8a2762af..07509a52 100644
--- a/packages/gossip/neighbor.go
+++ b/packages/gossip/neighbor.go
@@ -99,7 +99,7 @@ func (n *Neighbor) writeLoop() {
 				continue
 			}
 			if _, err := n.BufferedConnection.Write(msg); err != nil {
-				n.log.Warn("Write error", "err", err)
+				n.log.Warnw("Write error", "err", err)
 				_ = n.BufferedConnection.Close()
 				return
 			}
diff --git a/plugins/autopeering/plugin.go b/plugins/autopeering/plugin.go
index 4688b6b0..cad1ba41 100644
--- a/plugins/autopeering/plugin.go
+++ b/plugins/autopeering/plugin.go
@@ -33,7 +33,7 @@ func run(*node.Plugin) {
 
 func configureEvents() {
 	// notify the selection when a connection is closed or failed.
-	gossip.Events.ConnectionFailed.Attach(events.NewClosure(func(p *peer.Peer) {
+	gossip.Events.ConnectionFailed.Attach(events.NewClosure(func(p *peer.Peer, _ error) {
 		Selection.RemoveNeighbor(p.ID())
 	}))
 	gossip.Events.NeighborRemoved.Attach(events.NewClosure(func(p *peer.Peer) {
diff --git a/plugins/gossip/plugin.go b/plugins/gossip/plugin.go
index f6f5717e..20e804a1 100644
--- a/plugins/gossip/plugin.go
+++ b/plugins/gossip/plugin.go
@@ -59,8 +59,8 @@ func configureEvents() {
 		}()
 	}))
 
-	gossip.Events.ConnectionFailed.Attach(events.NewClosure(func(p *peer.Peer) {
-		log.Infof("Connection to neighbor failed: %s / %s", gossip.GetAddress(p), p.ID())
+	gossip.Events.ConnectionFailed.Attach(events.NewClosure(func(p *peer.Peer, err error) {
+		log.Infof("Connection to neighbor %s / %s failed: %s", gossip.GetAddress(p), p.ID(), err)
 	}))
 	gossip.Events.NeighborAdded.Attach(events.NewClosure(func(n *gossip.Neighbor) {
 		log.Infof("Neighbor added: %s / %s", gossip.GetAddress(n.Peer), n.ID())
-- 
GitLab