From 290d5443cc27e7185fe35cbd7f8d0f766588e0d9 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 14 Apr 2015 08:48:56 +0100 Subject: [PATCH 1/2] more discriminate connectivity If a peer was specified on the command line w/o a port, then do not connect to it if we have inbound connections from that IP. This eliminates unnecessary (and ultimately failing) connection attempts in the common scenario of several peers having been told about each other on their command lines. We represent a command line peer address with a TCPAddr, which avoids back and forth conversion to string and extra fields to carry the "did the original include a port?" information. --- router/connection_maker.go | 74 +++++++++++++++++++++++++------------- router/router.go | 2 +- 2 files changed, 50 insertions(+), 26 deletions(-) diff --git a/router/connection_maker.go b/router/connection_maker.go index 0f7e6959..dc93d8f3 100644 --- a/router/connection_maker.go +++ b/router/connection_maker.go @@ -15,12 +15,12 @@ const ( ) type ConnectionMaker struct { - ourself *LocalPeer - peers *Peers - normalisePeerAddr func(string) string - targets map[string]*Target - cmdLinePeers map[string]string - actionChan chan<- ConnectionMakerAction + ourself *LocalPeer + peers *Peers + port int + targets map[string]*Target + cmdLinePeers map[string]*net.TCPAddr + actionChan chan<- ConnectionMakerAction } // Information about an address where we may find a peer @@ -33,13 +33,13 @@ type Target struct { type ConnectionMakerAction func() bool -func NewConnectionMaker(ourself *LocalPeer, peers *Peers, normalisePeerAddr func(string) string) *ConnectionMaker { +func NewConnectionMaker(ourself *LocalPeer, peers *Peers, port int) *ConnectionMaker { return &ConnectionMaker{ - ourself: ourself, - peers: peers, - normalisePeerAddr: normalisePeerAddr, - cmdLinePeers: make(map[string]string), - targets: make(map[string]*Target)} + ourself: ourself, + peers: peers, + port: port, + cmdLinePeers: make(map[string]*net.TCPAddr), + targets: make(map[string]*Target)} } func (cm *ConnectionMaker) Start() { @@ -49,16 +49,19 @@ func (cm *ConnectionMaker) Start() { } func (cm *ConnectionMaker) InitiateConnection(peer string) error { - addr, err := net.ResolveTCPAddr("tcp4", cm.normalisePeerAddr(peer)) + host, port, err := net.SplitHostPort(peer) + if err != nil { + host = peer + port = "0" // we use that as an indication that "no port was supplied" + } + addr, err := net.ResolveTCPAddr("tcp4", fmt.Sprintf("%s:%s", host, port)) if err != nil { return err } - address := addr.String() - cm.actionChan <- func() bool { - cm.cmdLinePeers[peer] = address + cm.cmdLinePeers[peer] = addr // curtail any existing reconnect interval - if target, found := cm.targets[address]; found { + if target, found := cm.targets[addr.String()]; found { target.tryAfter, target.tryInterval = tryImmediately() } return true @@ -138,7 +141,7 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration { // Copy the set of things we are connected to, so we can access // them without locking. Also clear out any entries in cm.targets // for existing connections. - ourConnectedPeers, ourConnectedTargets := cm.ourConnections() + ourConnectedPeers, ourConnectedTargets, ourInboundIPs := cm.ourConnections() addTarget := func(address string) { if _, connected := ourConnectedTargets[address]; connected { @@ -154,9 +157,23 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration { } // Add command-line targets that are not connected - for _, address := range cm.cmdLinePeers { - addTarget(address) + for _, addr := range cm.cmdLinePeers { + completeAddr := *addr + attempt := true + if completeAddr.Port == 0 { + completeAddr.Port = cm.port + // If a peer was specified w/o a port, then we do not + // attempt to connect to it if we have any inbound + // connections from that IP. + if _, connected := ourInboundIPs[completeAddr.IP.String()]; connected { + attempt = false + } + } + address := completeAddr.String() cmdLineTarget[address] = void + if attempt { + addTarget(address) + } } // Add targets for peers that someone else is connected to, but we @@ -166,18 +183,25 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration { return cm.connectToTargets(validTarget, cmdLineTarget) } -func (cm *ConnectionMaker) ourConnections() (PeerNameSet, map[string]struct{}) { +func (cm *ConnectionMaker) ourConnections() (PeerNameSet, map[string]struct{}, map[string]struct{}) { var ( ourConnectedPeers = make(PeerNameSet) ourConnectedTargets = make(map[string]struct{}) + ourInboundIPs = make(map[string]struct{}) ) for conn := range cm.ourself.Connections() { address := conn.RemoteTCPAddr() + delete(cm.targets, address) ourConnectedPeers[conn.Remote().Name] = void ourConnectedTargets[address] = void - delete(cm.targets, address) + if conn.Outbound() { + continue + } + if ip, _, err := net.SplitHostPort(address); err == nil { // should always succeed + ourInboundIPs[ip] = void + } } - return ourConnectedPeers, ourConnectedTargets + return ourConnectedPeers, ourConnectedTargets, ourInboundIPs } func (cm *ConnectionMaker) addPeerTargets(ourConnectedPeers PeerNameSet, addTarget func(string)) { @@ -196,8 +220,8 @@ func (cm *ConnectionMaker) addPeerTargets(ourConnectedPeers PeerNameSet, addTarg if conn.Outbound() { addTarget(address) } - if host, _, err := net.SplitHostPort(address); err == nil { - addTarget(cm.normalisePeerAddr(host)) + if ip, _, err := net.SplitHostPort(address); err == nil { + addTarget(fmt.Sprintf("%s:%d", ip, cm.port)) } } }) diff --git a/router/router.go b/router/router.go index 21fbd750..33547742 100644 --- a/router/router.go +++ b/router/router.go @@ -67,7 +67,7 @@ func NewRouter(config RouterConfig, name PeerName, nickName string) *Router { router.Peers = NewPeers(router.Ourself.Peer, onPeerGC) router.Peers.FetchWithDefault(router.Ourself.Peer) router.Routes = NewRoutes(router.Ourself.Peer, router.Peers) - router.ConnectionMaker = NewConnectionMaker(router.Ourself, router.Peers, router.NormalisePeerAddr) + router.ConnectionMaker = NewConnectionMaker(router.Ourself, router.Peers, router.Port) router.TopologyGossip = router.NewGossip("topology", router) return router } From 339f85cbcb185fe9fbc5b4dfca674a3c50bf2d45 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 14 Apr 2015 13:18:40 +0100 Subject: [PATCH 2/2] get rid of Router.NormalisePeerAddr There was only one call site left and it always gets invoked with an ip:port, so normalisation is a no-op. Also remove mysterious comment. --- router/local_peer.go | 6 ++---- router/router.go | 10 ---------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/router/local_peer.go b/router/local_peer.go index 651d055b..63e93c73 100644 --- a/router/local_peer.go +++ b/router/local_peer.go @@ -93,10 +93,8 @@ func (peer *LocalPeer) CreateConnection(peerAddr string, acceptNewPeer bool) err if err := peer.checkConnectionLimit(); err != nil { return err } - // We're dialing the remote so that means connections will come from random ports - addrStr := peer.router.NormalisePeerAddr(peerAddr) - tcpAddr, tcpErr := net.ResolveTCPAddr("tcp4", addrStr) - udpAddr, udpErr := net.ResolveUDPAddr("udp4", addrStr) + tcpAddr, tcpErr := net.ResolveTCPAddr("tcp4", peerAddr) + udpAddr, udpErr := net.ResolveUDPAddr("udp4", peerAddr) if tcpErr != nil || udpErr != nil { // they really should have the same value, but just in case... if tcpErr == nil { diff --git a/router/router.go b/router/router.go index 33547742..450216ef 100644 --- a/router/router.go +++ b/router/router.go @@ -417,13 +417,3 @@ func (router *Router) applyTopologyUpdate(update []byte) (PeerNameSet, PeerNameS } return origUpdate, newUpdate, nil } - -// given an address like '1.2.3.4:567', return the address if it has a port, -// otherwise return the address with the default port number for the router -func (router *Router) NormalisePeerAddr(peerAddr string) string { - _, _, err := net.SplitHostPort(peerAddr) - if err == nil { - return peerAddr - } - return fmt.Sprintf("%s:%d", peerAddr, router.Port) -}