Ping protocol closes connection if other party doesn't support /ping

[thomaseizinger]

Issue as per title. Is this intentional? I would have expected a more graceful behaviour where the implementation just ignores a particular connection if the other party doesn't support /ping. In particular, I am talking about ProtocolsHandlerUpgrErr::Upgrade(UpgradeError::Select(NegotiationError::Failed)).

If this shouldn't be the default behaviour (IMO it should be), would you accept a PR with a config option that at least makes this configurable? Something like no_close_on_unsupported?

[mxinden]

Is this intentional?

I don't know whether this was intentional when first implemented. Either way say a user depends on libp2p-ping to close idle connections. Connections to a node that does not support libp2p-ping would never close when idle without this feature.

no_close_on_UNsupported

Adding this to PingConfig is fine by me. Off the top of my head, with the above in mind, I don't think we should make it the default behavior. Do you feel strongly about this @thomaseizinger?

Out of curiosity, what is your reasoning for not running ping on one of your nodes?

[thomaseizinger]

no_close_on_UNsupported

Typo corrected, thanks!

I don't know whether this was intentional when first implemented. Either way say a user depends on libp2p-ping to close idle connections. Connections to a node that does not support libp2p-ping would never close when idle without this feature.

I think there is a difference between how we implement connection_keep_alive based on the idle state of the connection and actively closing the connection because we run into an error.

I am suggesting to not make the handler of /ping do anything if it detects that the other party doesn't support /ping.
We can still have the handler close idle connections by returning KeepAlive::No.

Out of curiosity, what is your reasoning for not running ping on one of your nodes?

I ran into this when writing the examples of rendezvous. I was trying to build the following setup:

• Party A with /ping and /rendezvous, discovers services via rendezvous point
• rendezvous-point with /rendezvous and /identify
• Party B with /ping, /rendezvous and /identify, registers itself on startup based on observed external address from rendezvous point

The idea was for /ping to represent the protocol that party A and B would like to speak and they use the rendezvous-point to find each other. However, if you compose the behaviours like this, the rendezvous node always actively force-closes the connection to either node - disallowing the /rendezvous protocol to do its job - because it runs into the negotiation error with the /ping protocol.

[mxinden]

I don't know whether this was intentional when first implemented. Either way say a user depends on libp2p-ping to close idle connections. Connections to a node that does not support libp2p-ping would never close when idle without this feature.

I think there is a difference between how we implement connection_keep_alive based on the idle state of the connection and actively closing the connection because we run into an error.

I am suggesting to not make the handler of /ping do anything if it detects that the other party doesn't support /ping.
We can still have the handler close idle connections by returning KeepAlive::No.

Mistake on my end. I should have written dead instead of idle connections. There might be situations where users depend on libp2p-ping to close dead connections while one of their other protocols always returns KeepAlive::Yes. Say libp2p-ping does not close connections to peers that don't support the Ping protocol. An attacker could try to DOS a node by opening up a lot of connections, not supporting the Ping protocol, killing them on their side only, using up memory on the victims side.

The above is only problematic if another protocol always returns KeepAlive::Yes. I think that is problematic in itself.

Long story short: I am fine with either way, not closing connections when Ping is unsupported by default or as an option. Only requirement would be for this to be well documented.

[thomaseizinger]

Right, that makes sense. I wonder what a sane default is then. If we end up landing #2110 then we could change the protocol to emit an event instead (i.e. PingUnsupported) and let the application decide what to do. Some applications might want to close the connection as a result whereas others (my examples) wouldn't care too much.

On the other hand, it is less convenient if everyone has to write that line of code themselves.

I have to say, I am tempted to emit an event here and not make a decision on what should happen with dead connections. I think that ideal behaviour will depend on the overall usecase of the application and emitting an event allows us to keep the config of Ping simple.

[mxinden]

An additional PingFailure variant, e.g. ProtocolUnsupported, leaving it up to the user what to do, sounds good to me.

[thomaseizinger]

An additional PingFailure variant, e.g. ProtocolUnsupported, leaving it up to the user what to do, sounds good to me.

Copy that.

To reiterate:

• Remove behaviour of closing connections on unsupported protocol
• Emit an event to let the swarm / parent behaviour know about it

[mxinden]

Sounds good to me @thomaseizinger! Plus a changelog entry please :)