Option to specify "outbound connections only" in libp2p.hangUp()

[dirkmc]

In peer-base each peer keeps connections open to the peers in its Dias set. The Dias set membership changes as peers come online and go offline.

For example:

• Peer A has a Dias set that includes Peer B
• Membership changes such that Peer A's Dias set no longer includes Peer B
• Peer A should disconnect its outbound connection to peer B
• However if Peer B's Dias set includes Peer A, then that connection should be maintained

So from Peer A's perspective, it only wants to disconnect the outbound connection, leaving any inbound connection intact.

Later, when Peer A shuts down, it wants to disconnect both inbound and outbound connections.

Note that the peer:connect event doesn't indicate if a connection is inbound or outbound, so it's not possible for peer base to keep track of inbound vs outbound connections, and in any case it would simplify the code a lot if we could rely on libp2p to do that instead.

[jacobheun]

Accidentally closed this, this request looks good to me. I think adding inbound only hangups via options would be nice to have as well.

[dirkmc]

Agreed, that makes sense to me.

@jacobheun I see you applied the label difficulty:easy. Maybe I can take a stab at doing this myself. Could you give me a quick rundown of what the code should do exactly and what a good test would look like?

[jacobheun]

The core update will need to happen in switch (so maybe it shouldn't have been an easy label, lol). Luckily the recent updates I did for better connection tracking should help with this.

https://github.com/libp2p/js-libp2p-switch/blob/v0.41.3/src/index.js#L176, when iterating over the connections to close we can check their type against the options before closing.

We'll need to store the type on the connection, right now it's just using it for debug logging https://github.com/libp2p/js-libp2p-switch/blob/v0.41.3/src/connection/index.js#L18. The typing could probably be changed to an enum instead of the strings that are being used for logging.

The dial logic should be updated too, to change the type if the connection is already an fsm connection, https://github.com/libp2p/js-libp2p-switch/blob/v0.41.3/src/dialer.js#L59. Once we dial, we should consider that an outgoing connection even if it was originally incoming.

The recent test I added for parallel dial hangups could just be reused to hangup outgoing/incoming, https://github.com/libp2p/js-libp2p-switch/blob/v0.41.3/test/dial-fsm.node.js#L134.

Thinking through this I think there is a peer disconnect emitter bug. When we close 1 of the connections to a peer, but there is still another connection open, it will still emit the disconnect event for that peer, even though we're actually still connected. I'll create a separate issue for this.

[dirkmc]

Thanks for the detailed response! I have a question:
You said that once we dial, we should consider that an outgoing connection even if it was originally incoming. Should we instead have a third type of connection both?
I'm thinking of the case where a connection is both incoming and outgoing. If the user asks to hang up the outgoing connection, should we just change the connection's type from both to incoming? I'm not sure if that fits with how you've modeled connection handling.

Is there ever more than one outgoing or more than one incoming connection in the ConnectionManager for a particular remote peer?

[jacobheun]

It's possible for us to have more than one incoming/outgoing connection per peer, although it will currently try to minimize that. Thinking through this a bit more I don't think changing the type is the right thing to do, because we're changing the nature of that connection and may act on that inappropriately depending on what we're trying to do, such as closing only outgoing or incoming connections.

If we change an incoming connection to outgoing because we're dialing, and then our peer decides to close it's outgoing connection to us because it doesn't need it anymore, our dial will fail and we'll have to dial out again. Maintaining 1 each direction, as needed, gives us more flexibility. The drawback is the extra open connection. I think for now we can handle both connections, and then reevaluate it if it becomes too resource intensive.

We'll need to update the getOne logic, https://github.com/libp2p/js-libp2p-switch/blob/v0.41.3/src/connection/manager.js#L62, to take an connection type so we can just look for outgoing connections on dial.

What are your thoughts?

[dirkmc]

That seems simpler to me 👍
So just to confirm:

• there are two types: incoming / outgoing
• we don't ever change the type of a connection
• when dialing, we check if there is already an outgoing connection to that peer and if so we just use that one
• when closing connections to a peer of a particular type (incoming/outgoing), we should iterate through the connections and check that they are of the given type then close them

I think it may be useful when emitting peer:connect to also include the type (incoming / outgoing) there, what do you think?

[jacobheun]

Yes, to the logic flow.

I think adding that could be helpful. Along with the peerInfo we can send a metadata object so that we can more easily add additional information to that in the future.

[dirkmc]

@jacobheun it seems that when an outgoing FSMConnection receives an incoming connection, it adds itself to the ConnectionManager. But its connection type is Outgoing.

Our use case is that we want to close connections if they are outbound-only. So if there is a connection that is both inbound and outbound we would leave it alone. I wonder if the approach we've been discussing is the correct one, it seems like it may be a lot of overhead to maintain two connections just for this use case. Maybe a better approach would be to expose an API method such that we can query libp2p for whether we have inbound connections for a particular peerInfo?

[jacobheun]

That code should only run during an outgoing connection. When an incoming connection occurs it's added here https://github.com/libp2p/js-libp2p-switch/blob/v0.41.3/src/connection/manager.js#L183. The incoming connection creates the FSMConnection already muxed, so the upgrading logic never occurs.

Creating the connections is the expensive part of this. Ideally, what we'd like to do in the future is do some disconnect negotiation. So when you want to shut down outgoing connections but keep incoming connections, we'd negotiate with the peer that we'd like to shut the connection down. The peer could then ask us to keep the connection open if it's using it. This would enable us to make better decisions about connections and only need to maintain a single connection.

While the approach we discussed isn't the optimal solution, I think it's a reasonable stop gap until we can actually perform that disconnect handshake.

[dirkmc]

Ah, thanks @jacobheun that makes sense.

As I'm going through updating the tests for this change I've noticed a couple of things:

• When disconnecting we end the muxer which causes it to emit a close event, that in turn calls close() which sets the state to disconnect. In the FSM state mapping we have a link from DISCONNECTING back to itself on disconnect. That causes _onDisconnect to be called again, so we end up firing the peer-mux-closed event twice. Should we remove the link from DISCONNECTING back to itself on disconnect?
• In the swarm-muxing tests, we connect from peer A to peer B and then later from peer A to peer C. When we hang up the connection from peer A to peer B, the tests expect switchA._peerInfo.isConnected() not to exist. But it should still have a connection to peer C, right?

[dirkmc]

Fixed by libp2p/js-libp2p-switch#298

[wemeetagain]

This can be done like so:

await Promise.all(
  libp2p.getConnections
    .filter(c => c.direction === 'outbound')
    .map(c => c.close)
)