Ideally we wouldn't try to perform routing if we're just going to wait on an ongoing connection. But that's not a new issue.

We should calculate newState just after we add it to conns map with connectednessUnlocked. We already have the lock it's a small set to check anyway.
There's a problem here that if someone makes a relayed connection after we have a direct connection this will send a limited connectivity event.

I am not sure I addressed it correctly, can you please double check @sukunrt ?

We need.

	oldState := s.connectednessUnlocked(p)
	s.conns.m[p] = append(s.conns.m[p], c)
	newState := s.connectednessUnlocked(p)

This prevents us from sending a PeerConnectedness Limited event when we make a relayed connection after a direct connection.

This comment doesn't make sense to me. I don't really understand the point of the body either.

It seems like we have two cases:

1. We have multiple conns to a peer and we close one. If any other conn exists and is open then we are connected.
2. We only have one conn to a peer a close it. The peer is no longer connected.

Why are we potentially returning Connected in the case that every conn in the map returns c.IsClosed()=true with considerClosed=true

We may also have limited connections to the peer. So once a connection is closed, we check:

• If there is at least 1 unlimited connection, return Connected
• Else, if there is at least 1 limited connection, return Limited
• Else, return NotConnected

Why are we potentially returning Connected in the case that every conn in the map returns c.IsClosed()=true with considerClosed=true

It is true. However, considerClosed is true only when called from addConn and removeConn, which hold the lock. removeConn makes sure to remove the connection from s.conns.m, so the function will not return Connected if we removed the last unlimited connection.

Though, I don't really understand why considerClosed is needed at all. @sukunrt could you provide some context?

I agree this is confusing.

There are two different states this function is called from:

When closing the connection from our side by calling Close, this function is called when the underlying transport connection is not closed.

When closing the connection because the remote Closed the connection, this function is called when the underlying transport connection has already been closed.

Consider that we are connected to the peer over a QUIC and a relayed connection.

Now the peer closes the QUIC connection. So we have moved from a Connected state to Limited connectivity state.

To make this work, when calculating oldState in removeConn we need to consider connectivity assuming QUIC connection was still connected.

Alternatively we can store the last sent connectedness event for a peer. That would make this calculation cleaner at the expense of more state for an edge case.

Alternatively we can store the last sent connectedness event for a peer. That would make this calculation cleaner at the expense of more state for an edge case.

I think we should do this.

done

I would really like to avoid spawning a go routine on close

Yeah, we shouldn't do this. I haven't read the rest of the patch so I might be missing an obvious solution, but it looks like we're just trying to "raise" a flag for this peer.

I'd just avoid channels entirely and use the pattern I used in https://github.com/ipfs/boxo/blob/2559ec2c7b93ac6905fe83d7a4b38b8e9a703b0a/bitswap/network/connecteventmanager.go#L69-L81, https://github.com/ipfs/boxo/blob/2559ec2c7b93ac6905fe83d7a4b38b8e9a703b0a/bitswap/network/connecteventmanager.go#L97-L122. It's not simple, but it works and avoids stacking goroutines.

I haven’t looked closely, but it seems like changeQueue is unbounded. Is that right?

Technically, but it shouldn't be an issue in practice. We only add a peer to the queue the first time the connectivity state changes. If we end up with a lot of "dead" peers in this queue, we should burn through them very quickly (transition from disconnected -> disconnected doesn't require any events).

We can do better by removing peers that have transitioned back to the same state from the queue, but it's likely not worth the complexity.

I think a queue with this will work

We can do better by removing peers that have transitioned back to the same state from the queue, but it's likely not worth the complexity.

We do need the complexity because a malicious peer can connect, disconnect very quickly causing this queue to grow quickly if a consumer is slow. It can also do this with different peer ids.

As a general rule we should have no unbounded queues in the core logic. In this case I can think of a way that a reasonable application can fall victim to a dos attack.

Example: An application opens a new stream and does an exchange on a newly connected peer. If this is done by the same go routine that reads events then the attacker controls the rate events are popped off, and thus exploit this.

Ideally, I'd agree. But in cases like this we can argue that the queue will hit a steady-state max size and stop growing (assuming the read side isn't deadlocked forever). It's not great, but if someone can DoS us due to this queue, they can also DoS us by running us out of memory before GC can kick in.

But ideally we'd just have some form of backpressure somewhere. And I think that's what we'll need given #2696 (comment)

I am trying an approach where we have backpressure on the add side and to have an unbounded queue on the remove side(close connection) because the remove side is effectively bounded by the total number of connections since on the add connection side we do have backpressure.

How does 5271dd8 1abc063 look?

We apply backpressure when adding connections to the swarm. This allows us to enqueue remove conn events in a slice without spawning a goroutine. Back pressure on adding connections limits the slice length. We also ensure that we send at least 1 disconnection event for every peer we connect to.

I don't really have time for a full review but:

1. I'd be careful to avoid too many channels/goroutines. Go tends to push you in that direction, but it can quickly get harder to understand than simple locks & conditions.
2. Make sure backpressure is applied all the way to accepting connections in the first place. It sounds like this might be a gap in libp2p and may be something we want to fix in a separate patch.

(I defer to @MarcoPolo on all of this, I'm just kibitzing from the sidelines)