fix: return error when AddChannel fails (#500)

* Return error * add no lint tags * Add error handling * Remove logging that never happens * Add no lint tags * Add logging
Finschia · Nov 22, 2022 · 2828a22 · 2828a22
1 parent 15a55a2
commit 2828a22
Show file tree

Hide file tree

Showing 7 changed files with 193 additions and 9 deletions.
diff --git a/node/node.go b/node/node.go
@@ -198,7 +198,10 @@ func CustomReactors(reactors map[string]p2p.Reactor) Option {
  for _, chDesc := range reactor.GetChannels() {
  if !ni.HasChannel(chDesc.ID) {
  ni.Channels = append(ni.Channels, chDesc.ID)
- n.transport.AddChannel(chDesc.ID)
+ err := n.transport.AddChannel(chDesc.ID)
+ if err != nil {
+ n.Logger.Debug("AddChannel failed", "err", err)
+ }
  }
  }
  n.nodeInfo = ni

diff --git a/node/node_test.go b/node/node_test.go
@@ -27,6 +27,7 @@ import (
  "github.com/line/ostracon/p2p"
  "github.com/line/ostracon/p2p/conn"
  p2pmock "github.com/line/ostracon/p2p/mock"
+ p2pmocks "github.com/line/ostracon/p2p/mocks"
  "github.com/line/ostracon/privval"
  "github.com/line/ostracon/proxy"
  sm "github.com/line/ostracon/state"
@@ -578,3 +579,68 @@ func state(nVals int, height int64) (sm.State, dbm.DB, []types.PrivValidator) {
  }
  return s, stateDB, privVals
 }
+
+func TestNodeInvalidNodeInfoCustomReactors(t *testing.T) {
+ config := cfg.ResetTestRoot("node_new_node_custom_reactors_test")
+ defer os.RemoveAll(config.RootDir)
+
+ cr := p2pmock.NewReactor()
+ cr.Channels = []*conn.ChannelDescriptor{
+ {
+ ID: byte(0x31),
+ Priority: 5,
+ SendQueueCapacity: 100,
+ RecvMessageCapacity: 100,
+ },
+ }
+ customBlockchainReactor := p2pmock.NewReactor()
+
+ nodeKey, err := p2p.LoadOrGenNodeKey(config.NodeKeyFile())
+ require.NoError(t, err)
+
+ pvKey, _ := privval.LoadOrGenFilePV(config.PrivValidatorKeyFile(), config.PrivValidatorStateFile(),
+ config.PrivValidatorKeyType())
+ _, err = NewInvalidNode(config,
+ pvKey,
+ nodeKey,
+ proxy.DefaultClientCreator(config.ProxyApp, config.ABCI, config.DBDir()),
+ DefaultGenesisDocProviderFunc(config),
+ DefaultDBProvider,
+ DefaultMetricsProvider(config.Instrumentation),
+ log.TestingLogger(),
+ CustomReactors(map[string]p2p.Reactor{"FOO": cr, "BLOCKCHAIN": customBlockchainReactor}),
+ )
+ require.NoError(t, err)
+}
+
+func NewInvalidNode(config *cfg.Config,
+ privValidator types.PrivValidator,
+ nodeKey *p2p.NodeKey,
+ clientCreator proxy.ClientCreator,
+ genesisDocProvider GenesisDocProvider,
+ dbProvider DBProvider,
+ metricsProvider MetricsProvider,
+ logger log.Logger,
+ options ...Option) (*Node, error) {
+ n, err := NewNode(config,
+ privValidator,
+ nodeKey,
+ clientCreator,
+ genesisDocProvider,
+ dbProvider,
+ metricsProvider,
+ logger,
+ )
+ if err != nil {
+ return nil, err
+ }
+
+ transport, _ := createTransport(config, &p2pmocks.NodeInfo{}, nodeKey, n.proxyApp)
+ n.transport = transport
+
+ for _, option := range options {
+ option(n)
+ }
+
+ return n, nil
+}
diff --git a/p2p/mocks/node_info.go b/p2p/mocks/node_info.go
diff --git a/p2p/mocks/peer.go b/p2p/mocks/peer.go
diff --git a/p2p/node_info.go b/p2p/node_info.go
@@ -12,6 +12,8 @@ import (
  "github.com/line/ostracon/version"
 )
 
+//go:generate mockery --case underscore --name NodeInfo
+
 const (
  maxNodeInfoSize = 10240 // 10KB
  maxNumChannels = 16 // plenty of room for upgrades, for now

diff --git a/p2p/transport.go b/p2p/transport.go
@@ -265,13 +265,16 @@ func (mt *MultiplexTransport) Listen(addr NetAddress) error {
 // NOTE: NodeInfo must be of type DefaultNodeInfo else channels won't be updated
 // This is a bit messy at the moment but is cleaned up in the following version
 // when NodeInfo changes from an interface to a concrete type
-func (mt *MultiplexTransport) AddChannel(chID byte) {
- if ni, ok := mt.nodeInfo.(DefaultNodeInfo); ok {
- if !ni.HasChannel(chID) {
- ni.Channels = append(ni.Channels, chID)
- }
- mt.nodeInfo = ni
+func (mt *MultiplexTransport) AddChannel(chID byte) error {
+ ni, ok := mt.nodeInfo.(DefaultNodeInfo)
+ if !ok {
+ return fmt.Errorf("nodeInfo type: %T is not supported", mt.nodeInfo)
+ }
+ if !ni.HasChannel(chID) {
+ ni.Channels = append(ni.Channels, chID)
  }
+ mt.nodeInfo = ni
+ return nil
 }
 
 func (mt *MultiplexTransport) acceptPeers() {

diff --git a/p2p/transport_test.go b/p2p/transport_test.go
@@ -14,6 +14,7 @@ import (
  "github.com/line/ostracon/libs/protoio"
  "github.com/line/ostracon/p2p/conn"
  tmp2p "github.com/line/ostracon/proto/ostracon/p2p"
+ "github.com/stretchr/testify/require"
 )
 
 var defaultNodeName = "host_peer"
@@ -630,7 +631,8 @@ func TestTransportAddChannel(t *testing.T) {
  )
  testChannel := byte(0x01)
 
- mt.AddChannel(testChannel)
+ err := mt.AddChannel(testChannel)
+ require.NoError(t, err)
  if !mt.nodeInfo.(DefaultNodeInfo).HasChannel(testChannel) {
  t.Errorf("missing added channel %v. Got %v", testChannel, mt.nodeInfo.(DefaultNodeInfo).Channels)
  }