Announce and discover peers over simple UDP broadcasts #243

ehmry · 2017-11-15T19:31:45Z

This patch adds a broadcastService that broadcasts the local peer ID and listens for IDs from remote peers. IP broadcasting is limited to IPv4, but is simplier than multicasting and only intended for local area networks.

Stebalien

Looks useful. This is a preliminary code review, I don't have enough background to comment much on the discovery mechanism itself.

Stebalien · 2017-11-15T21:04:35Z

p2p/discovery/broadcast.go

+	// fudge the interval to dampen oscillations with other nodes
+	margin := int(interval) >> 2
+	interval += time.Duration(rand.Intn(margin))
+	interval -= time.Duration(rand.Intn(margin))


This looks a bit funky. Any reason not to jus to just add a smaller random delay to the interval (rather than add and then subtract one)?

I add and subtract so the the interval can jitter by a positive or negative amount. And I like getting funky.

Yes, but negative jitter is equivalent to just reducing the default interval (you could also just subtract margin/1).

Stebalien · 2017-11-15T21:45:34Z

p2p/discovery/broadcast.go

+		for _, n := range bs.notifees {
+			go n.HandlePeerFound(pi)
+		}
+		bs.lk.Unlock()


Best to defer this call instead.

Stebalien · 2017-11-15T21:46:59Z

p2p/discovery/broadcast.go

+func (m *broadcastService) RegisterNotifee(n Notifee) {
+	m.lk.Lock()
+	m.notifees = append(m.notifees, n)
+	m.lk.Unlock()


Again, we generally defer unlocks when possible.

Stebalien · 2017-11-15T21:47:34Z

p2p/discovery/broadcast.go

+	if found != -1 {
+		m.notifees = append(m.notifees[:found], m.notifees[found+1:]...)
+	}
+	m.lk.Unlock()


Stebalien · 2017-11-15T21:49:17Z

p2p/discovery/broadcast.go

+		}
+	}
+	if found != -1 {
+		m.notifees = append(m.notifees[:found], m.notifees[found+1:]...)


Since we don't really care about order, you can just swap the entry you want to remove for the last entry. Also, remember to null out the last entry, otherwise, it won't be garbage collected.

Stebalien · 2017-11-15T21:49:49Z

p2p/discovery/broadcast.go

+	found := -1
+	for i, notif := range m.notifees {
+		if notif == n {
+			found = i


Cleaner to just put the removal code inside here and then return (assuming a deferred unlock).

Stebalien · 2017-11-15T21:50:45Z

p2p/discovery/broadcast.go

+		case <-ticker.C:
+			bs.sendBroadcast()
+
+		case <-ctx.Done():


If we're going to make canceling the context equivalent to calling close, we should call close here. Actually, we should defer a call to close so it gets called regardless.

Stebalien · 2017-11-15T21:51:53Z

p2p/discovery/broadcast.go

+}
+
+func (bs *broadcastService) recvBroadcasts(ctx context.Context) {
+	var buf = make([]byte, 48)


There are no exit conditions here.

Stebalien · 2017-11-15T21:52:54Z

p2p/discovery/broadcast.go

+}
+
+func (bs *broadcastService) Close() error {
+	bs.listenSock.Close()


This should also cancel the context (which means you'll need to make the context cancelable and store the cancel function). I'm not really a fan of using contexts in this way but we're already doing it for the other broadcast services so oh well...

Kubuxu · 2017-11-15T22:00:07Z

I was working on similar feature for cjdns but explicitly IPv6, it allows for better granularity but more over it works on mobile. I think if we are to add something like that we are better off using IPv6.

ehmry · 2017-11-16T20:39:34Z

FWIW I haven't written go for few years so I'm open to any review.

@Kubuxu I had considered trying to do this with multicast and the 'all-systems' address, which is supported for IPv6 as well, but multicast seems overly complicated. My intention here is simply discovering nodes within a small physical network so for the sake of simplicity IPv4 only seems justified.

My immediate use case is actually to discover IPFS nodes that are broadcasting on port 4001 and then connect to them on 5001 as an API client.

Broadcast the local peer id from UDP sockets bound to the same IP addresses and ports as the TCP listening sockets. Listen for these broadcast and discover peers by deducing the peer address from received packets.

ehmry · 2017-11-17T23:29:15Z

@Stebalien thanks for the review, I've updated the PR following your recommendations.

Stebalien · 2017-11-18T00:08:32Z

p2p/discovery/broadcast.go

+	// fudge the interval to dampen oscillations with other nodes
+	margin := int(interval) >> 2
+	interval += time.Duration(rand.Intn(margin))
+	interval -= time.Duration(rand.Intn(margin))


Yes, but negative jitter is equivalent to just reducing the default interval (you could also just subtract margin/1).

Stebalien · 2017-11-18T00:10:59Z

p2p/discovery/broadcast.go

+
+func (bs *broadcastService) recvBroadcasts(ctx context.Context) {
+	var buf = make([]byte, 48)
+	for bs.active {


Checking this without a lock isn't guaranteed to work. Try re-running your tests with the -race flag. Assuming that doesn't hit one of our bugs..., that should catch things like this.

Stebalien · 2017-11-21T21:01:15Z

p2p/discovery/broadcast.go

+		IP:   addr.IP,
+		Port: addr.Port,
+	})
+	if err == nil {


Stylistic but... you can reduce the indentation level by using:

if error != nil { return } // ...

Stebalien · 2017-11-21T21:04:44Z

p2p/discovery/broadcast.go

+
+	// send broadcasts on a periodic timeout
+	ticker := time.NewTicker(interval)
+	for bs.active {


Again, can't read/write to a variable from different threads without using some synchronization mechanism (lock, channel, etc.).

Really, I'd just get rid of this variable and use the context.

It's a bool.

It's a bool.

I know, I'm referring to https://golang.org/ref/mem. For example, given:

package main func main() { var active = true go func() { active = false }() for active { } }

Go may choose to allow the second go routine to loop forever (optimizing out the check of active) because no synchronization primitives were used. Try running the above code with the race detector.

This is a race condition, it doesn't matter if its just a bool.

Stebalien · 2017-11-21T21:08:24Z

p2p/discovery/broadcast.go

+	defer m.lk.Unlock()
+	for i, notif := range m.notifees {
+		if notif == n {
+			m.notifees[i] = nil


Having an array filled with nils is probably not the best way to go. Instead, use https://github.com/golang/go/wiki/SliceTricks#delete-without-preserving-order. That will simplify RegisterNotifee and handleBroadcast as well.

Stebalien · 2017-11-21T21:12:09Z

p2p/discovery/broadcast.go

+func (bs *broadcastService) recvBroadcasts(ctx context.Context) {
+	var buf = make([]byte, 48)
+	for bs.active {
+		n, rAddr, err := bs.listenSock.ReadFromUDP(buf)


I'm pretty sure most errors from this function are fatal. That is, we can't continue listening on the socket (we'd end up in a rapid, infinite loop). It's probably better to log the error, stop the service, and return.

You could also try checking to see if the error is temporary (cast to net.Error and check if err.Temporary() is true).

Stebalien · 2017-11-21T21:14:57Z

p2p/discovery/broadcast.go

+//
+func bindBroadcast(ip net.IP, port int) (*net.UDPConn, error) {
+	conn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: ip, Port: port})
+	if err == nil {


The go idiom is to (almost) always write:

if err != nil { return nil, err }

That helps prevent nesting/confusing control flow.

I would argue that typing the same early return idiom over and over and over is worse for understanding control flow.

It's just how go is written (that is, consistency is king). However, I'd disagree:

It makes it very easy to recognize where errors are being returned.

It reduces indentation.

Now, one can argue that there should be a better way to return errors in go but that's a separate issue (and not something we can do anything about).

Perhaps you should think of go's error handling as a monad; when an error occurs you return.
Otherwise the control flow is linear, the less branching the better.

Stebalien · 2017-11-21T21:16:11Z

p2p/discovery/broadcast.go

+				}
+			}
+		}
+	}


We may want to return an error if we fail to bind to anything. I wouldn't stop IPFS from starting in that case but, if we return an error and log it, it'll make debugging easier.

When I looked at the mdns file I saw three different methods of logging errors, so I thought it cleaner not to log at all.

We usually use github.com/ipfs/go-log. Any other methods of logging should be fixed. However, I'd just suggest returning an error and logging it from go-ipfs.

Kubuxu

@ehmry any interest in continuing with this change?

vyzo

it may seem like a small thing, but it's important to reduce control flow indentation for error checking.
It makes code easier to read, and reduces cognitive overhead.
So please fix your control flow/indentation!

vyzo · 2018-03-06T08:53:23Z

p2p/discovery/broadcast.go

+	if err != nil {
+		bs.Close()
+		return nil, err
+	} else {


no need for an else branch here, you are returning in the case of error -- this just adds indentation and cognitive overhead.

vyzo · 2018-03-06T08:55:16Z

p2p/discovery/broadcast.go

+	var buf = make([]byte, 48)
+	for bs.active {
+		n, rAddr, err := bs.listenSock.ReadFromUDP(buf)
+		if err == nil {


if err != nil { continue }

Again, this reduces indentation level and cognitive overhead.

vyzo · 2018-03-06T08:55:42Z

p2p/discovery/broadcast.go

+
+	// send broadcasts on a periodic timeout
+	ticker := time.NewTicker(interval)
+	for bs.active {


This is a race condition, it doesn't matter if its just a bool.

vyzo · 2018-03-06T08:56:53Z

p2p/discovery/broadcast.go

+//
+func bindBroadcast(ip net.IP, port int) (*net.UDPConn, error) {
+	conn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: ip, Port: port})
+	if err == nil {


Perhaps you should think of go's error handling as a monad; when an error occurs you return.
Otherwise the control flow is linear, the less branching the better.

ehmry · 2018-03-07T08:02:57Z

I'm just not going to think of go at all.

New Dialer

* feat: harden encoding/decoding functions against panics Part of #1389 These kinds of functions: 1. Handle user input. 2. Often have out-of-bounds, null pointer, etc bugs. 3. Have completely isolated logic where local panics are unlikely to cause memory corruption elsewhere. * test: add a panic catcher test

ehmry mentioned this pull request Nov 15, 2017

Announce and discover swarm addresses over simple UDP broadcasts ipfs/kubo#4393

Closed

Stebalien self-requested a review November 15, 2017 21:47

Stebalien requested changes Nov 15, 2017

View reviewed changes

Stebalien requested a review from Kubuxu November 15, 2017 21:56

whyrusleeping requested a review from a user November 16, 2017 04:50

Announce and discover peers over simple UDP broadcasts

351b0e0

Broadcast the local peer id from UDP sockets bound to the same IP addresses and ports as the TCP listening sockets. Listen for these broadcast and discover peers by deducing the peer address from received packets.

Stebalien requested changes Nov 21, 2017

View reviewed changes

Kubuxu reviewed Mar 6, 2018

View reviewed changes

vyzo reviewed Mar 6, 2018

View reviewed changes

ehmry closed this Mar 7, 2018

marten-seemann pushed a commit that referenced this pull request Apr 21, 2022

Merge pull request #243 from libp2p/fix/direct-connect

ce7c0bc

New Dialer

marten-seemann added a commit that referenced this pull request Apr 22, 2022

reduce the maximum number of incoming streams to 256 (#243)

05bb49a

Announce and discover peers over simple UDP broadcasts #243

Announce and discover peers over simple UDP broadcasts #243

Conversation

ehmry commented Nov 15, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Nov 15, 2017

ehmry commented Nov 16, 2017

ehmry commented Nov 17, 2017 • 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

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

Stebalien Nov 21, 2017 • 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

Choose a reason for hiding this comment

Kubuxu left a comment

Choose a reason for hiding this comment

vyzo 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

ehmry commented Mar 7, 2018

ehmry commented Nov 17, 2017 •

edited

Loading

Stebalien Nov 21, 2017 •

edited

Loading