swarm: emit PeerConnectedness event from swarm instead of from hosts #1574

marten-seemann · 2022-05-28T09:43:35Z

This was surprisingly easy. Unfortunately, I can't find a way to test the deduplication (that no event is emitted when a second connection is dialed), since the swarm is too good at preventing dials to hosts we're already connected to.

vyzo · 2022-05-28T10:29:35Z

p2p/host/basic/basic_host.go

@@ -112,6 +112,9 @@ var _ host.Host = (*BasicHost)(nil)
 // HostOpts holds options that can be passed to NewHost in order to
 // customize construction of the *BasicHost.
 type HostOpts struct {
+	// EventBus sets the event bus. Will construct a new event bus if omitted.


I feel uneasy about this.
Maybe we should add the EventBus method to the Network interface and forward from the host so that we cant end up with two of them.

Not really how this could happen, but we can make this change.

Can we defer this until we consolidated all the other repos as well (#1566)? It's happening in the next release.

Another option would be to just make this entire thing private.

I'm not a big fan of having both a Config and HostOpts. I expect us to merge these two (or refactor them) in some other way when we start using Fx for service construction (#1993). I suggest not making any big changes in this PR.

MarcoPolo · 2022-05-31T23:44:05Z

p2p/net/swarm/swarm.go

@@ -310,6 +321,13 @@ func (s *Swarm) addConn(tc transport.CapableConn, dir network.Direction) (*Conn,
 	})
 	c.notifyLk.Unlock()

+	if isFirstConn {


Are we sure we want this outside of the notifyLk? That changes the behavior from what it used to be (inside the notifyAll).

Yeah, I think this needs to be inside. We need to make sure that the disconnect comes after the connect event.

Hm. Actually, I'm not sure if this lock helps us here. We need to make this guarantee across all connections.

MarcoPolo · 2022-05-31T23:50:24Z

p2p/net/swarm/swarm.go

+	s.conns.Unlock()
+
+	if disconnected {
+		s.emitter.Emit(event.EvtPeerConnectednessChanged{


What's the reason to do this here instead of near the old place where this happened: https://github.com/libp2p/go-libp2p/blob/marco/1575/p2p/net/swarm/swarm_conn.go#L85 ?

Probably to ensure that we only emit the event on the last disconnect. But it does look like there's a bit of a race and we can emit a disconnect after a connect (looks like we already have that race, actually).

Stebalien · 2022-06-01T02:21:03Z

So, I think the only way to sanely do this is to write something like: https://github.com/ipfs/go-bitswap/pull/565/files. We probably don't want to block some global queue while sending events.

That is:

On connect/disconnect, mark the peer as connected/disconnected in some tracker. Add the peer to some "todo" queue, and tell the background process that it needs to wake up.
In a background event loop, fire connect/disconnect events.

Even if this background process falls behind (someone is blocking events), that's fine. We'll just always fire the "last" event. This does mean we can get unmatched connected/disconnected events, but, IMO, that's fine.

I.e.:

Connect.
Disconnect.
Connect.
Disconnect.

May be seen as:

Connect.
Connect.
Disconnect.

Or

Disconnect.
Disconnect.

Or

Connect.
Disconnect.

Or

Disconnect.

But never:

Connect.
Disconnect.
Connect.

Importantly, the final state is always accurate, which is what users care about. At the moment, it's possible for the last event fired to be "connect", which could cause memory leaks.

marten-seemann · 2023-02-05T04:18:49Z

Rebased this PR on top of the current master.

I think emitting the event while holding the conns mutex should be fine:

It's correct since the the swarm.conns.m[peer.ID] is the source of truth: Once we're down to 0 connections in this list, we're officially disconnected from the peer.
It's ok performance-wise, because emitting events is cheap, as long as the subscribers consume events quickly enough. Now we could (somewhat) protect against that by introducing a worker Go routine and another tracking data structure as suggested by @Stebalien in swarm: emit PeerConnectedness event from swarm instead of from hosts #1574 (comment), but that data structure would need to be limited in size somehow, so this is no better than using a larger channel for the event bus subscriber. Using the new event bus dashboard (eventbus: add metrics #2038) it should be easy to detect a misconfiguration there.

The following sequence can show up in two different ways:

Connect.
Disconnect.
Connect.
Disconnect.

First way:

Connect
Disconnect
Connect
Disconnect

Second way (when the two connects are processed first):

Connect
Disconnect

I introduced the change in the last commit. @MarcoPolo, @Stebalien does this reasoning make sense to you?

MarcoPolo

Changes look good and reasoning makes sense.

Unfortunately, I can't find a way to test the deduplication (that no event is emitted when a second connection is dialed)

You could try this:
Make a new test transport that wraps an existing transport. Put two multiaddrs for this test transport (they might have to be slightly different if we're deduping these somewhere). In the test transport have them synchronize with a waitgroup to return a dial at the same time.

p2p/net/swarm/swarm_event_test.go

marten-seemann · 2023-02-24T22:54:28Z

Make a new test transport that wraps an existing transport. Put two multiaddrs for this test transport (they might have to be slightly different if we're deduping these somewhere). In the test transport have them synchronize with a waitgroup to return a dial at the same time.

That doesn't work either. The dial worker loop somewhere deduplicates. I have no intention of diving into that mess to figure out where.

This reverts commit 82fbba8. The update breaks the event system. It likely happened after this libp2p/go-libp2p#1574, but we don't need to debug it right now, as a simple revert fixes this

marten-seemann requested review from vyzo and MarcoPolo May 28, 2022 09:43

vyzo reviewed May 28, 2022

View reviewed changes

MarcoPolo reviewed May 31, 2022

View reviewed changes

BigLep added the P3 Low: Not priority right now label Sep 9, 2022

marten-seemann force-pushed the connectedness branch from 331b2b7 to b66075a Compare February 5, 2023 03:28

marten-seemann requested review from MarcoPolo and Stebalien February 5, 2023 04:22

This was referenced Feb 5, 2023

Pass the eventbus to the Swarm constructor #840

Closed

go-libp2p v0.26 #2062

Closed

marten-seemann mentioned this pull request Feb 22, 2023

go-libp2p v0.27 #2123

Closed

21 tasks

MarcoPolo approved these changes Feb 24, 2023

View reviewed changes

p2p/net/swarm/swarm_event_test.go Outdated Show resolved Hide resolved

marten-seemann added 2 commits February 25, 2023 11:12

pass an event bus to the swarm constructor

2318fd2

make the eventbus parameter a required swarm constructor parameter

631d371

marten-seemann force-pushed the connectedness branch from 8a8ebcb to 9c74a76 Compare February 24, 2023 22:13

marten-seemann added 3 commits February 25, 2023 11:16

emit Connectedness notifications from the swarm

b006d83

remove peer connectedness watchers from hosts

9f32bc2

swarm: emit connectedness events when holding the mutex

7889a65

marten-seemann force-pushed the connectedness branch from 9c74a76 to 7889a65 Compare February 24, 2023 22:16

marten-seemann merged commit 581a015 into master Feb 25, 2023

Wondertan mentioned this pull request Apr 27, 2023

Revert "deps: bump libp2p (#2127)" celestiaorg/celestia-node#2136

Merged

Wondertan mentioned this pull request May 4, 2023

Connectedness events are not submitted by mocknet #2267

Closed

Wondertan mentioned this pull request May 16, 2023

deps: Update libp2p to v0.28 celestiaorg/celestia-node#2211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm: emit PeerConnectedness event from swarm instead of from hosts #1574

swarm: emit PeerConnectedness event from swarm instead of from hosts #1574

marten-seemann commented May 28, 2022

vyzo May 28, 2022 •

edited

Loading

marten-seemann May 28, 2022

Stebalien Jun 1, 2022

marten-seemann Feb 5, 2023

MarcoPolo May 31, 2022

Stebalien Jun 1, 2022

Stebalien Jun 1, 2022

MarcoPolo May 31, 2022

Stebalien Jun 1, 2022

Stebalien commented Jun 1, 2022

marten-seemann commented Feb 5, 2023 •

edited

Loading

MarcoPolo left a comment

marten-seemann commented Feb 24, 2023 •

edited

Loading

swarm: emit PeerConnectedness event from swarm instead of from hosts #1574

swarm: emit PeerConnectedness event from swarm instead of from hosts #1574

Conversation

marten-seemann commented May 28, 2022

vyzo May 28, 2022 • 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

Stebalien commented Jun 1, 2022

marten-seemann commented Feb 5, 2023 • edited Loading

MarcoPolo left a comment

Choose a reason for hiding this comment

marten-seemann commented Feb 24, 2023 • edited Loading

vyzo May 28, 2022 •

edited

Loading

marten-seemann commented Feb 5, 2023 •

edited

Loading

marten-seemann commented Feb 24, 2023 •

edited

Loading