From 520c7286c44ff995a85edfb0f6f0f2ec09b2a50d Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 1 Oct 2015 17:48:23 +0100 Subject: [PATCH 1/5] backoff retries for late-failing connections If a connection gets through all the handshake and subsequent checks - i.e. it ends up getting added to the LocalPeer's connection list - but gets terminated within one minute (of the start of the successful connection attempt), then we apply the existing backoff logic to schedule a retry, instead of retrying immediately. In order to do this we retain the 'Target' data structure for such connections, marking it as 'connected' instead of deleting it (as we did previously). Fixes #953. --- router/connection_maker.go | 40 ++++++++++++++++++++++++-------------- router/status.go | 2 ++ 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/router/connection_maker.go b/router/connection_maker.go index 4bef62db..20a5d580 100644 --- a/router/connection_maker.go +++ b/router/connection_maker.go @@ -10,6 +10,7 @@ import ( const ( InitialInterval = 2 * time.Second MaxInterval = 6 * time.Minute + ResetAfter = 1 * time.Minute ) type peerAddrs map[string]*net.TCPAddr @@ -28,6 +29,7 @@ type ConnectionMaker struct { // Information about an address where we may find a peer type Target struct { attempting bool // are we currently attempting to connect there? + connected bool // are we currently connected to there? lastError error // reason for disconnection last time tryAfter time.Time // next time to try this address tryInterval time.Duration // retry delay on next failure @@ -92,7 +94,10 @@ func (cm *ConnectionMaker) ForgetConnections(peers []string) { func (cm *ConnectionMaker) ConnectionAborted(address string, err error) { cm.actionChan <- func() bool { - cm.retry(address, err) + target := cm.targets[address] + target.attempting = false + target.lastError = err + target.retry() return true } } @@ -101,7 +106,9 @@ func (cm *ConnectionMaker) ConnectionCreated(conn Connection) { cm.actionChan <- func() bool { cm.connections[conn] = void if conn.Outbound() { - delete(cm.targets, conn.RemoteTCPAddr()) + target := cm.targets[conn.RemoteTCPAddr()] + target.attempting = false + target.connected = true } return false } @@ -111,7 +118,18 @@ func (cm *ConnectionMaker) ConnectionTerminated(conn Connection, err error) { cm.actionChan <- func() bool { delete(cm.connections, conn) if conn.Outbound() { - cm.retry(conn.RemoteTCPAddr(), err) + target := cm.targets[conn.RemoteTCPAddr()] + target.attempting = false + target.connected = false + target.lastError = err + switch { + case err == ErrConnectToSelf: + target.tryNever() + case time.Now().After(target.tryAfter.Add(ResetAfter)): + target.tryNow() + default: + target.retry() + } } return true } @@ -238,7 +256,7 @@ func (cm *ConnectionMaker) connectToTargets(validTarget map[string]struct{}, dir now := time.Now() // make sure we catch items just added after := MaxDuration for address, target := range cm.targets { - if target.attempting { + if target.attempting || target.connected { continue } if _, valid := validTarget[address]; !valid { @@ -268,12 +286,9 @@ func (cm *ConnectionMaker) attemptConnection(address string, acceptNewPeer bool) } } -func (cm *ConnectionMaker) retry(address string, err error) { - if target, found := cm.targets[address]; found { - target.attempting = false - target.lastError = err - target.retry() - } +func (t *Target) tryNever() { + t.tryAfter = time.Time{} + t.tryInterval = MaxInterval } func (t *Target) tryNow() { @@ -284,11 +299,6 @@ func (t *Target) tryNow() { // The delay at the nth retry is a random value in the range // [i-i/2,i+i/2], where i = InitialInterval * 1.5^(n-1). func (t *Target) retry() { - if t.lastError == ErrConnectToSelf { - t.tryAfter = time.Time{} - t.tryInterval = MaxInterval - return - } t.tryAfter = time.Now().Add(t.tryInterval/2 + time.Duration(rand.Int63n(int64(t.tryInterval)))) t.tryInterval = t.tryInterval * 3 / 2 if t.tryInterval > MaxInterval { diff --git a/router/status.go b/router/status.go index a2e5ad56..7575cd82 100644 --- a/router/status.go +++ b/router/status.go @@ -175,6 +175,8 @@ func NewLocalConnectionStatusSlice(cm *ConnectionMaker) []LocalConnectionStatus for address, target := range cm.targets { var state, info string switch { + case target.connected: + continue case target.lastError == nil: state = "connecting" info = "" From b8b3e6f9cd5b13219736d98d05cc14b28e8eb3af Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 1 Oct 2015 19:03:24 +0100 Subject: [PATCH 2/5] refactor: better representation of 3-valued target state --- router/connection_maker.go | 25 +++++++++++++++---------- router/status.go | 4 ++-- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/router/connection_maker.go b/router/connection_maker.go index 20a5d580..b01c2529 100644 --- a/router/connection_maker.go +++ b/router/connection_maker.go @@ -26,10 +26,17 @@ type ConnectionMaker struct { actionChan chan<- ConnectionMakerAction } +type TargetState int + +const ( + TargetWaiting TargetState = iota // we are waiting to connect there + TargetAttempting // we are attempting to connect there + TargetConnected // we are connected to there +) + // Information about an address where we may find a peer type Target struct { - attempting bool // are we currently attempting to connect there? - connected bool // are we currently connected to there? + state TargetState lastError error // reason for disconnection last time tryAfter time.Time // next time to try this address tryInterval time.Duration // retry delay on next failure @@ -95,7 +102,7 @@ func (cm *ConnectionMaker) ForgetConnections(peers []string) { func (cm *ConnectionMaker) ConnectionAborted(address string, err error) { cm.actionChan <- func() bool { target := cm.targets[address] - target.attempting = false + target.state = TargetWaiting target.lastError = err target.retry() return true @@ -107,8 +114,7 @@ func (cm *ConnectionMaker) ConnectionCreated(conn Connection) { cm.connections[conn] = void if conn.Outbound() { target := cm.targets[conn.RemoteTCPAddr()] - target.attempting = false - target.connected = true + target.state = TargetConnected } return false } @@ -119,8 +125,7 @@ func (cm *ConnectionMaker) ConnectionTerminated(conn Connection, err error) { delete(cm.connections, conn) if conn.Outbound() { target := cm.targets[conn.RemoteTCPAddr()] - target.attempting = false - target.connected = false + target.state = TargetWaiting target.lastError = err switch { case err == ErrConnectToSelf: @@ -169,7 +174,7 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration { if _, found := cm.targets[address]; found { return } - target := &Target{} + target := &Target{state: TargetWaiting} target.tryNow() cm.targets[address] = target } @@ -256,7 +261,7 @@ func (cm *ConnectionMaker) connectToTargets(validTarget map[string]struct{}, dir now := time.Now() // make sure we catch items just added after := MaxDuration for address, target := range cm.targets { - if target.attempting || target.connected { + if target.state != TargetWaiting { continue } if _, valid := validTarget[address]; !valid { @@ -268,7 +273,7 @@ func (cm *ConnectionMaker) connectToTargets(validTarget map[string]struct{}, dir } switch duration := target.tryAfter.Sub(now); { case duration <= 0: - target.attempting = true + target.state = TargetAttempting _, isCmdLineTarget := directTarget[address] go cm.attemptConnection(address, isCmdLineTarget) case duration < after: diff --git a/router/status.go b/router/status.go index 7575cd82..095f42aa 100644 --- a/router/status.go +++ b/router/status.go @@ -175,12 +175,12 @@ func NewLocalConnectionStatusSlice(cm *ConnectionMaker) []LocalConnectionStatus for address, target := range cm.targets { var state, info string switch { - case target.connected: + case target.state == TargetConnected: continue case target.lastError == nil: state = "connecting" info = "" - case target.attempting: + case target.state == TargetAttempting: state = "retrying" info = target.lastError.Error() default: From e3182624fdf74a6724cfd17abb29527f9ff47897 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 1 Oct 2015 19:21:51 +0100 Subject: [PATCH 3/5] refactor: more obviously handle all target states in reporting --- router/status.go | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/router/status.go b/router/status.go index 095f42aa..c56ccd03 100644 --- a/router/status.go +++ b/router/status.go @@ -174,22 +174,29 @@ func NewLocalConnectionStatusSlice(cm *ConnectionMaker) []LocalConnectionStatus } for address, target := range cm.targets { var state, info string - switch { - case target.state == TargetConnected: + switch target.state { + case TargetConnected: continue - case target.lastError == nil: - state = "connecting" - info = "" - case target.state == TargetAttempting: - state = "retrying" - info = target.lastError.Error() - default: - state = "failed" - retry := "never" + case TargetAttempting: + if target.lastError == nil { + state = "connecting" + info = "" + } else { + state = "retrying" + info = target.lastError.Error() + } + case TargetWaiting: + until := "never" if !target.tryAfter.IsZero() { - retry = target.tryAfter.String() + until = target.tryAfter.String() + } + if target.lastError == nil { // shouldn't happen + state = "waiting" + info = "until: " + until + } else { + state = "failed" + info = target.lastError.Error() + ", retry: " + until } - info = target.lastError.Error() + ", retry: " + retry } slice = append(slice, LocalConnectionStatus{address, true, state, info}) } From 41691957d2d730ae7d883a66eba4066e0528df33 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 1 Oct 2015 19:30:42 +0100 Subject: [PATCH 4/5] refactor: simplify target status reporting --- router/status.go | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/router/status.go b/router/status.go index c56ccd03..bbb10d41 100644 --- a/router/status.go +++ b/router/status.go @@ -173,32 +173,28 @@ func NewLocalConnectionStatusSlice(cm *ConnectionMaker) []LocalConnectionStatus slice = append(slice, LocalConnectionStatus{conn.RemoteTCPAddr(), conn.Outbound(), state, conn.Remote().String()}) } for address, target := range cm.targets { - var state, info string + add := func(state, info string) { + slice = append(slice, LocalConnectionStatus{address, true, state, info}) + } switch target.state { - case TargetConnected: - continue - case TargetAttempting: - if target.lastError == nil { - state = "connecting" - info = "" - } else { - state = "retrying" - info = target.lastError.Error() - } case TargetWaiting: until := "never" if !target.tryAfter.IsZero() { until = target.tryAfter.String() } if target.lastError == nil { // shouldn't happen - state = "waiting" - info = "until: " + until + add("waiting", "until: "+until) } else { - state = "failed" - info = target.lastError.Error() + ", retry: " + until + add("failed", target.lastError.Error()+", retry: "+until) } + case TargetAttempting: + if target.lastError == nil { + add("connecting", "") + } else { + add("retrying", target.lastError.Error()) + } + case TargetConnected: } - slice = append(slice, LocalConnectionStatus{address, true, state, info}) } resultChan <- slice return false From 19c44b83d24e3258dd55c8f279e8af0e581cc7a7 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 2 Oct 2015 11:31:11 +0100 Subject: [PATCH 5/5] refactor: better 'try' method names --- router/connection_maker.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/router/connection_maker.go b/router/connection_maker.go index b01c2529..85775fd2 100644 --- a/router/connection_maker.go +++ b/router/connection_maker.go @@ -82,7 +82,7 @@ func (cm *ConnectionMaker) InitiateConnections(peers []string, replace bool) []e cm.directPeers[peer] = addr // curtail any existing reconnect interval if target, found := cm.targets[addr.String()]; found { - target.tryNow() + target.nextTryNow() } } return true @@ -104,7 +104,7 @@ func (cm *ConnectionMaker) ConnectionAborted(address string, err error) { target := cm.targets[address] target.state = TargetWaiting target.lastError = err - target.retry() + target.nextTryLater() return true } } @@ -129,11 +129,11 @@ func (cm *ConnectionMaker) ConnectionTerminated(conn Connection, err error) { target.lastError = err switch { case err == ErrConnectToSelf: - target.tryNever() + target.nextTryNever() case time.Now().After(target.tryAfter.Add(ResetAfter)): - target.tryNow() + target.nextTryNow() default: - target.retry() + target.nextTryLater() } } return true @@ -175,7 +175,7 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration { return } target := &Target{state: TargetWaiting} - target.tryNow() + target.nextTryNow() cm.targets[address] = target } @@ -291,19 +291,19 @@ func (cm *ConnectionMaker) attemptConnection(address string, acceptNewPeer bool) } } -func (t *Target) tryNever() { +func (t *Target) nextTryNever() { t.tryAfter = time.Time{} t.tryInterval = MaxInterval } -func (t *Target) tryNow() { +func (t *Target) nextTryNow() { t.tryAfter = time.Now() t.tryInterval = InitialInterval } // The delay at the nth retry is a random value in the range // [i-i/2,i+i/2], where i = InitialInterval * 1.5^(n-1). -func (t *Target) retry() { +func (t *Target) nextTryLater() { t.tryAfter = time.Now().Add(t.tryInterval/2 + time.Duration(rand.Int63n(int64(t.tryInterval)))) t.tryInterval = t.tryInterval * 3 / 2 if t.tryInterval > MaxInterval {