Consider sending 'CannotConnect' message after failing to connect to a peer following a 'ConnectToPeer' request

[jpdillingham]

Something @mathiascode brought up. I think I was doing this at one time, but then stopped for some reason. Step 1 will be reviewing old PRs and issues to see if I left any clues for myself.

I vaguely remember something about sending that potentially causing the peer to give up their direct connection attempt, but that could have been something else.

Related commit: nicotine-plus/nicotine-plus@b7dbdfd

[jpdillingham]

If this were to be done, it needs to be done in two places, one for peer connections, and again for distributed connections.

The peer connection can be handled in GetOrAddMessageConnectionAsync(ConnectToPeerResponse) at the point where the logic throws and disposes the connection. It would be much clearer to put this logic inside of the ServerMessageHandler but the method can throw for a number of reasons and we may mistakenly report a failure when the problem wasn't connectivity. It might be prudent to pass an Action in.

The distributed connection would be pretty much identical.

[jpdillingham]

I'll note that I'm hesitant to do this because I'm not sure of the implications of getting it wrong; if we send a CannotConnect for a peer, it may permanently block us from connecting when the problem creating a connection may have been a transient issue.

I feel like this has been working for at least 5 years as is, so unless this would solve a problem or improve something I think I'll leave it as is.

[jpdillingham]

Another thing; handling incoming CannotConnect messages is out of the question; I tried that at one time and it disrupted the rest of the connection negotiation flow due to the asynchronous nature.

[mathiascode]

I'll note that I'm hesitant to do this because I'm not sure of the implications of getting it wrong; if we send a CannotConnect for a peer, it may permanently block us from connecting when the problem creating a connection may have been a transient issue.

The CantConnectToPeer message only tells the user initiating the indirect connection request (sent a ConnectToPeer message to the server) that responding to their request failed after a while, direct connections are unaffected. If implemented correctly, it shouldn't have a negative impact.

The message isn't that useful, since clients already have to keep local timeouts for indirect requests anyway. I mainly implemented it to match the official clients more closely. If it's a lot of trouble to implement, maybe it's better not to.