feat: limit the number of cold calls we can do #316
Conversation
feat: add a backoff to blacklisting
ghost
assigned 
feat: add jitter to blacklist ttl
|
Found an unrelated connection management issue, https://github.com/libp2p/js-libp2p-switch/issues/317, as I was running this against my local js-ipfs daemon logging out some stats. |
jacobheun
changed the title
There was a problem hiding this comment.
Looks great @jacobheun ! Nice work
Approved, but left a minor suggestion to consider
| // Clear if the queue has reached max blacklist | ||
| if (dialQueue.blackListed === Infinity) { | ||
| dialQueue.abort() | ||
| delete this._queues[dialQueue.id] |
There was a problem hiding this comment.
What do you think about doing this._queues[dialQueue.id] = undefined?
This is more efficient than deleting, but than we will need to verify that keys have a value when iterating the array, so I do not have a strong opinion here.
There was a problem hiding this comment.
Since this is only running on a 15min interval I think the performance hit should be fairly insignificant and would avoid us needing to check values.
|
LGTM 👍 |
This creates a separate queue for "cold calls" (dials without a protocol). Applications typically do this on peer discovery to connect to new peers. This is to prevent flooding the normal queue, which needs to be prioritized for normal operations, with dials to new peers that may not respond.
This is a workaround, until we can improve the peer discovery system and connection/peer tagging, to allow for proper priority. Right now all dials are treated the same, which includes just creating a new stream on an existing connection. Existing connections and new dials should not be throttled the same way.
This also adds a very basic backoff to blacklisted peers. On its 5th blacklisting the peer will be added to the blacklist permanently. The backoff time may need to be increased before merging this