exponential backoff for addrs in the address book by melekes · Pull Request #1292 · tendermint/tendermint

melekes · 2018-03-08T10:03:38Z

codecov-io · 2018-03-08T10:13:11Z

Codecov Report

Merging #1292 into develop will decrease coverage by 0.08%.
The diff coverage is 78.12%.

@@             Coverage Diff             @@
##           develop    #1292      +/-   ##
===========================================
- Coverage    60.58%   60.49%   -0.09%     
===========================================
  Files          112      112              
  Lines        10863    10884      +21     
===========================================
+ Hits          6581     6584       +3     
- Misses        3644     3657      +13     
- Partials       638      643       +5

Impacted Files	Coverage Δ
rpc/lib/client/ws_client.go	`56.95% <100%> (-0.19%)`	⬇️
p2p/pex/addrbook.go	`63.82% <100%> (+0.83%)`	⬆️
p2p/pex/pex_reactor.go	`67.28% <75.86%> (+1.02%)`	⬆️
consensus/state.go	`76.72% <0%> (-1.63%)`	⬇️
blockchain/pool.go	`69.64% <0%> (-0.72%)`	⬇️
consensus/reactor.go	`72.26% <0%> (-0.57%)`	⬇️
p2p/pex/file.go	`78.04% <0%> (+4.87%)`	⬆️

xla

👍 🍡

xla · 2018-03-08T11:24:11Z

p2p/pex/pex_reactor.go

 	// Dial picked addresses
-	for _, item := range toDial {
+	for _, addr := range toDial {
 		go func(picked *p2p.NetAddress) {


With ensurePeers growing further maybe we can break this function out and make it a private method on PEXReactor as it only seemed to depend on the reactor itself and the *p2p.NetAddress.

xla · 2018-03-08T11:26:40Z

p2p/pex/pex_reactor.go

+			var attempt int
+			if lAttempt, notFirstAttempt := r.attemptsToDial.Load(picked.DialString()); notFirstAttempt {
+				attempt = lAttempt.(int)
+				jitterSeconds := time.Duration(rand.Float64() * billionNs)


Isn't billionNs equivalent to float64(time.Second) here?

xla · 2018-03-08T11:34:16Z

p2p/pex/pex_reactor.go

 				}
+				// record attempt
+				r.attemptsToDial.Store(picked.DialString(), attempt+1)
+			} else {


This else is unnecessary.

how so? there is no return or break

You are right. I assumed you break out in the if condition.

xla · 2018-03-08T11:39:18Z

p2p/pex/pex_reactor.go

 		go func(picked *p2p.NetAddress) {
+			// exponential backoff if it's not our first attempt to dial picked address
+			var attempt int
+			if lAttempt, notFirstAttempt := r.attemptsToDial.Load(picked.DialString()); notFirstAttempt {


Some minor naming nits. Would call attempt, attempts here as we store the amount of times we attempted to connect. And notFirstAttempt -> attempted for more concise and inverse of the meaning.

xla · 2018-03-08T12:00:37Z

p2p/pex/pex_reactor.go

-			}
-		}(item)
+			r.dialPeer(picked)
+		}(addr)


Can't we call go r.dialPeer(addr) directly?

https://github.com/golang/go/wiki/CommonMistakes#using-goroutines-on-loop-iterator-variables ?

Here addr is passed to dialPeer and evaluate at each iteration and placed on the stack of the routine, similar to how you pass it to the closure at the moment.

okay.. thanks

xla · 2018-03-08T13:44:11Z

p2p/pex/pex_reactor.go

+	// exponential backoff if it's not our first attempt to dial given address
+	var attempts int
+	if lAttempts, attempted := r.attemptsToDial.Load(addr.DialString()); attempted {
+		attempts = lAttempts.(int)


Can we test this behaviour? I think this branch isn't covered with our current tests. It might also surface that we want to make the jitter duration configurable in some form.

How about now? f85c889

Beautiful and simple!

xla · 2018-03-09T15:33:23Z

p2p/pex/pex_reactor.go

+	var attempts int
+	if lAttempts, attempted := r.attemptsToDial.Load(addr.DialString()); attempted {
+		attempts = lAttempts.(int)
+		jitterSeconds := time.Duration(rand.Float64() * billionNs)


billionNs is the same as float64(time.Second), don't think we need that extra variable, but we might benefit from having it configurable.

xla

👍 🍡

ebuchman

Great little PR. Thanks @melekes (and especially for the test cleanup).

A few comments that should be addressed, but otherwise I think it's fine.

One thing to note though is that if we have a lot of peers in the AddrBook, it could be a long time before we retry the same peer, in which case the exponentional backoff may not make sense. So we might want to track a more complex structure that includes the last time we dialed the peer so we can check if enough time has lapsed. Doesn't necessarily need to be in this PR but let's at least open an issue to track that if you're going to merge this.

ebuchman · 2018-03-10T16:52:34Z

p2p/pex/pex_reactor.go

+
+	// exponential backoff if it's not our first attempt to dial given address
+	var attempts int
+	if lAttempts, attempted := r.attemptsToDial.Load(addr.DialString()); attempted {


can we just use r.AttemptsToDial() here ?

ebuchman · 2018-03-10T16:54:58Z

p2p/pex/pex_reactor.go

+		attempts = lAttempts.(int)
+		jitterSeconds := time.Duration(rand.Float64() * billionNs)
+		backoffDuration := jitterSeconds + ((1 << uint(attempts)) * time.Second)
+		r.Logger.Debug(fmt.Sprintf("Dialing %v", addr), "attempts", attempts, "backoff_duration", backoffDuration)


I think this is a bit misleading. Let's just say Sleeping before dialing and including the addr in the structured log too. Then right before we dial we can log "Dialing" with the addr and attempt again.

ebuchman · 2018-03-10T16:55:47Z

p2p/pex/pex_reactor.go

+
+	err := r.Switch.DialPeerWithAddress(addr, false)
+	if err != nil {
+		r.Logger.Error("Dialing failed", "err", err)


include addr and attempt in log

ebuchman · 2018-03-10T16:58:00Z

p2p/pex/pex_reactor.go

 	requestsSent         *cmn.CMap // ID->struct{}: unanswered send requests
 	lastReceivedRequests *cmn.CMap // ID->time.Time: last time peer requested from us
+
+	attemptsToDial sync.Map // dial addr -> number of attempts to dial (for exponential backoff)


can we be more specific with types in this comment?

eg. string->int: number of dial attempts per address; for exponential backoff

ebuchman · 2018-03-10T17:02:12Z

p2p/pex/pex_reactor.go

+		jitterSeconds := time.Duration(rand.Float64() * billionNs)
+		backoffDuration := jitterSeconds + ((1 << uint(attempts)) * time.Second)
+		r.Logger.Debug(fmt.Sprintf("Dialing %v", addr), "attempts", attempts, "backoff_duration", backoffDuration)
+		time.Sleep(backoffDuration)


Do we have logic that limits the number of attempts per address?

If this backoffDuration gets big, then these go-routines will just hang around for a while. So maybe we should be using a select block here with a ticker so we can break out on Quit ?

melekes · 2018-03-11T10:02:54Z

@ebuchman thanks for suggestion. implemented in 264bce4. PTAL

ebuchman

Great thanks!

We should update the docs.

I think I'll just merge this and open a new issue to mark a few things :)

ebuchman · 2018-03-11T18:53:04Z

p2p/pex/pex_reactor.go

+		lastDialed = lAttempts.(_attemptsToDial).lastDialed
+	}
+
+	if attempts > maxAttemptsToDial {


Looks like we do have a maxAttempts mechanism already in place: https://github.com/tendermint/tendermint/blob/develop/p2p/pex/known_address.go#L130

Maybe we can consolidate to make sure they're working right together ? feel free to leave this an just open an issue to note the redundancy

ebuchman · 2018-03-11T18:54:31Z

p2p/pex/pex_reactor.go

+		backoffDuration := jitterSeconds + ((1 << uint(attempts)) * time.Second)
+		sinceLastDialed := time.Since(lastDialed)
+		if sinceLastDialed < backoffDuration {
+			r.Logger.Debug("Too early to dial", "addr", addr, "backoff_duration", backoffDuration, "last_dialed", lastDialed, "time_since", sinceLastDialed)


this is cool - it solves the "hanging go-routine" issue but it does mean we might not end up dialing this peer for much longer than the backoff duration. I suppose that's fine

ebuchman · 2018-03-11T19:01:16Z

See #1304

melekes requested a review from ebuchman as a code owner March 8, 2018 10:03

exponential backoff for addrs in the address book

21e2c41

Refs #1125

melekes force-pushed the 1125-exp-backoff-for-ensure-peers branch from 6c09c69 to 21e2c41 Compare March 8, 2018 10:04

xla approved these changes Mar 8, 2018

View reviewed changes

xla reviewed Mar 8, 2018

View reviewed changes

fixes from @xla's review

1941b5c

melekes force-pushed the 1125-exp-backoff-for-ensure-peers branch from 51f71a6 to 1941b5c Compare March 8, 2018 12:31

xla suggested changes Mar 8, 2018

View reviewed changes

xla mentioned this pull request Mar 8, 2018

P2P: validator mode #865

Closed

2 tasks

xla assigned melekes Mar 8, 2018

melekes added 2 commits March 9, 2018 16:02

refactor pex_reactor tests

f0d4f56

test pex_reactor's dialPeer

f85c889

xla reviewed Mar 9, 2018

View reviewed changes

xla approved these changes Mar 9, 2018

View reviewed changes

ebuchman reviewed Mar 10, 2018

View reviewed changes

melekes added 2 commits March 11, 2018 13:22

fixes from bucky's review

0f41570

skip dialing based on last time dialed

264bce4

use time.Since

fc5b047

ebuchman approved these changes Mar 11, 2018

View reviewed changes

ebuchman merged commit f3000d0 into develop Mar 11, 2018

ebuchman mentioned this pull request Mar 11, 2018

p2p: exponential back-off followup #1304

Closed

3 tasks

ebuchman deleted the 1125-exp-backoff-for-ensure-peers branch March 11, 2018 19:01

Conversation

melekes commented Mar 8, 2018

Uh oh!

codecov-io commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented Mar 11, 2018

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Mar 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Mar 8, 2018 •

edited

Loading

xla Mar 8, 2018 •

edited

Loading