Periodically schedule identify push if the address set has changed #597

vyzo · 2019-04-13T11:02:43Z

Adds a background task to basic host, that periodically schedules an identify push if the address set has changed.

vyzo · 2019-04-13T11:03:38Z

ci failed with a data race...

vyzo · 2019-04-13T11:06:10Z

WARNING: DATA RACE
Read at 0x00c0000b6190 by goroutine 48:
  github.com/libp2p/go-libp2p/p2p/host/basic.(*BasicHost).Addrs()
      /home/travis/gopath/src/github.com/libp2p/go-libp2p/p2p/host/basic/basic_host.go:557 +0x6e
  github.com/libp2p/go-libp2p/p2p/host/basic.(*BasicHost).background()
      /home/travis/gopath/src/github.com/libp2p/go-libp2p/p2p/host/basic/basic_host.go:300 +0x201
Previous write at 0x00c0000b6190 by goroutine 46:
  github.com/libp2p/go-libp2p/config.(*Config).NewNode()
      /home/travis/gopath/src/github.com/libp2p/go-libp2p/config/config.go:134 +0x13d1
  github.com/libp2p/go-libp2p.NewWithoutDefaults()
      /home/travis/gopath/src/github.com/libp2p/go-libp2p/libp2p.go:68 +0x17a
  github.com/libp2p/go-libp2p.New()
      /home/travis/gopath/src/github.com/libp2p/go-libp2p/libp2p.go:54 +0x147
  github.com/libp2p/go-libp2p.TestNoTransports()
      /home/travis/gopath/src/github.com/libp2p/go-libp2p/libp2p_test.go:50 +0xc8
  testing.tRunner()
      /home/travis/.gimme/versions/go1.11.9.linux.amd64/src/testing/testing.go:827 +0x162

insanity...

vyzo · 2019-04-13T11:08:40Z

I added a delay in the background task, but maybe we should have an explicit Start method that gets called by the constructor instead.

vyzo · 2019-04-13T11:12:01Z

Now we get this failure:

=== RUN   TestFuzzManyPeers
race: limit on 8128 simultaneously alive goroutines is exceeded, dying
FAIL	github.com/libp2p/go-libp2p/p2p/net/mock	30.895s

I'll gate this test.

vyzo · 2019-04-13T11:17:17Z

It's already gated, so I reduced the peer count further.

Stebalien · 2019-04-13T16:24:20Z

Let's do an explicit start.

vyzo · 2019-04-13T16:25:26Z

Ok, will do.

vyzo · 2019-04-13T16:32:57Z

Added Start method in basic_host.

vyzo · 2019-04-16T18:19:09Z

rebased on master.

vyzo · 2019-04-18T07:47:37Z

summoning @Stebalien

Stebalien

Sorry, forgot about this halfway through my review. The only real issue is the permanent stall with a misbehaving peer.

Stebalien · 2019-04-16T19:00:24Z

p2p/host/basic/basic_host.go

+		return false
+	}
+
+	// this is O(n*m), might be worth using a map to turn into O(n+m)


It probably is as some peers end up with a bunch of addresses. Alternatively, we could sort both lists.

(but we can probably fix that later if it shows up in profiles)

I think I'll fix it now, I don't like having perf bombs waiting to go off.

Implemented the map in b17ba31

Stebalien · 2019-04-19T04:22:03Z

p2p/protocol/identify/id.go

 			s, err := ids.Host.NewStream(ctx, p, IDPush)
 			if err != nil {
-				log.Debugf("error opening push stream: %s", err.Error())
+				log.Debugf("error opening push stream to %s: %s", p, err.Error())
 				return
 			}

 			ids.requestHandler(s)


TODO: Not a bug in this PR but we should probably:

Do this in a goroutine.

Reset the stream if the context expires.

At the moment, we can hang the entire push if a single peer stalls.

Actually, this is now an issue because we're blocking everything else (which we weren't doing before).

I can move the context back into the goroutine and remove the wait group, so that we don't stall.

Note that the only reason I did this dance with the wait group was my desire to reuse the context instead of creating one for each peer.

Ah, you want the ids.requestHandler call in a goroutine, so that we don't stall; much better than exploding the contexts.

Also note we are not blocking anything, there is a supervisory goroutine waiting for the WaitGroup.

~~We might simply get away with setting a Deadline in the stream.~~

Done in 5503cd7
I moved the ids.requestHandler into a goroutine and added a wait for the context cancellation to reset the stream if the handler doesn't complete by then.

Also note we are not blocking anything, there is a supervisory goroutine waiting for the WaitGroup.

Ah, got it. But better to reset the stream anyways.

Yeah, let's not build up goroutines if we have a stuck peer.

Stebalien · 2019-04-19T16:35:51Z

Note: multiple contexts is actually pretty cheap.

Stebalien · 2019-04-19T16:36:47Z

p2p/host/basic/basic_host.go

+		return false
+	}
+
+	bmap := make(map[string]struct{})


Nit: we could preallocate.

ok, will pass a length.

…fy push

… if needed

…tector

vyzo · 2019-04-19T17:06:29Z

rebased on master for merging.

vyzo requested a review from Stebalien April 13, 2019 11:02

ghost assigned vyzo Apr 13, 2019

ghost added the status/in-progress In progress label Apr 13, 2019

vyzo changed the title ~~Peridoically schedule identify push if the address set has changed~~ Periodically schedule identify push if the address set has changed Apr 13, 2019

vyzo force-pushed the feat/address-push branch from 5e900b2 to cb712e6 Compare April 16, 2019 18:18

Stebalien requested changes Apr 19, 2019

View reviewed changes

vyzo requested a review from Stebalien April 19, 2019 10:13

Stebalien approved these changes Apr 19, 2019

View reviewed changes

Stebalien reviewed Apr 19, 2019

View reviewed changes

vyzo added 9 commits April 19, 2019 20:03

use a single, NoDial context in identify push

bf651ca

track context given in identify constructor and use it to gate identi…

8fa257c

…fy push

add a background task in basic host that periodically pushes identify…

7502fc4

… if needed

add initialization delay in basic host background task

5845909

reduce peer count in TestFuzzManyPeers when running under the race de…

c9b2f46

…tector

explicit Start method for basic host

c73f68b

handle misbehaving peers in identify push

3697552

use a map in sameAddrs to avoid quadratic behaviour

ebc5d16

preallocate map in sameAddrs

f17a4a8

vyzo force-pushed the feat/address-push branch from 6b859d6 to f17a4a8 Compare April 19, 2019 17:04

vyzo merged commit b0f4a6f into master Apr 19, 2019

ghost removed the status/in-progress In progress label Apr 19, 2019

vyzo deleted the feat/address-push branch April 19, 2019 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Periodically schedule identify push if the address set has changed #597

Periodically schedule identify push if the address set has changed #597

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

Stebalien commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 16, 2019

vyzo commented Apr 18, 2019

Stebalien left a comment

Stebalien Apr 16, 2019

vyzo Apr 19, 2019

vyzo Apr 19, 2019

Stebalien Apr 19, 2019

Stebalien Apr 19, 2019

vyzo Apr 19, 2019

vyzo Apr 19, 2019

vyzo Apr 19, 2019

vyzo Apr 19, 2019

vyzo Apr 19, 2019 •

edited

Loading

vyzo Apr 19, 2019

Stebalien Apr 19, 2019

vyzo Apr 19, 2019

Stebalien commented Apr 19, 2019

Stebalien Apr 19, 2019

vyzo Apr 19, 2019

vyzo Apr 19, 2019

vyzo commented Apr 19, 2019

Periodically schedule identify push if the address set has changed #597

Periodically schedule identify push if the address set has changed #597

Conversation

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

Stebalien commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 13, 2019

vyzo commented Apr 16, 2019

vyzo commented Apr 18, 2019

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Apr 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Apr 19, 2019

vyzo Apr 19, 2019 •

edited

Loading