peerRouting.findPeer() trying to find self returns AggregateError instead more suitable error

[a1300]

• Version: v0.29.3
• Platform: Linux
• Subsystem:

Type: Bug

Severity: Low

Description: When I try to find myself with peerRouting.findPeer() I get a AggregateError: No peer found instead of a more suitable error like ERR_DIALED_SELF

Steps to reproduce the error:

git clone https://gist.github.com/a1300/be916e343940c321aae587fd10cc3478 libp2p-peerRouting-find-self
cd libp2p-peerRouting-find-self
npm install
node index

Output:

(node:9829) UnhandledPromiseRejectionWarning: AggregateError: 
    Error: No peer found
        at Object.findPeer (/home/a1300/test/gny/libp2p-peerRouting-find-self/node_modules/libp2p-kad-dht/src/peer-routing/index.js:191:2
3)
    at maybeSettle (/home/a1300/test/gny/libp2p-peerRouting-find-self/node_modules/p-some/index.js:31:11)
    at /home/a1300/test/gny/libp2p-peerRouting-find-self/node_modules/p-some/index.js:69:23
(node:9829) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async fu
nction without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:9829) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not han
dled will terminate the Node.js process with a non-zero exit code.

[jacobheun]

Throwing a dial self error here doesn't really make sense. Ideally the findPeer(self) should never happen, and applications should probably try to avoid this. There is the possibility that we may wish to just return ourselves from local, but I don't think this is a great idea.

Is there a particular reason you're trying to find self, or is this just do to generic findPeer(peer) and peer happens to be self in some instances?

[a1300]

@jacobheun

Background:
I need to know all peers of the network in a few seconds. Therefore every node announces a new found peer to the network (via pubsub broadcast event). I have 1 rendezvous node. If another node tells me about myself, I don't want to try to find myself with peerRouting.findPeer() myself. I have a check in place now.

But thats how I found this bug. The AggregateError Message is in my opinion not very describing. Maybe a new Error message ERR_PEERROUTING_FIND_SELF_ERR?

Is there a more elegant way to find all peers in a network in a few seconds? I tried the randomWalk of the DHT, but it took too long.

[jacobheun]

Is there a more elegant way to find all peers in a network in a few seconds? I tried the randomWalk of the DHT, but it took too long.

DHT performance currently needs to be improved, it's on our short list of improvements to make to get it inline with the recent go updates that were made this year. This won't be viable in browsers, especially on larger networks, but it should work well for Node once it's improved.

But thats how I found this bug. The AggregateError Message is in my opinion not very describing. Maybe a new Error message ERR_PEERROUTING_FIND_SELF_ERR?

I agree that the aggregate error messaging could be improved. It's possible that multiple errors could be associated with a query because we can potentially use multiple peer routers. This could change in the future if we change the routing calls to be more explicit (declaring whether you're using the DHT or something else) thus removing the need to collect all errors. Regardless of this we should make the errors easier to debug/consume.

[zeim839]

@a1300

I don't want to try to find myself with peerRouting.findPeer() myself. I have a check in place now.

Are you sure peerRouting.findPeer() is the source of the issue? I was working on a potential fix that adds a comparison in the findPeer() function like so: if (id == this._peerId) { throw errCode(new Error('Cannot dial self'), 'ERR_DIALED_SELF') }. Running this on your repository still yields the original error with no mention of "ERR_DIALED_SELF".

This is my output:
(node:58522) UnhandledPromiseRejectionWarning: AggregateError: Error: No peer found at Object.findPeer..

Like @jacobheun said, findPeer(self) shouldn't be possible, so this is most likely an issue that's not associated with findPeer(self) that's being thrown as part of an aggregate error.

Edit: The error originates from KadDHT.findPeer(). I was able to resolve the issue by adding: if (id._idB58String == this.peerId._idB58String) { throw errcode(new Error('Cannot find self'), 'ERR_DIAL_SELF') } to the KadDHT index.js file. I will be creating a PR at js-libp2p-kad-dht to resolve the issue.

[vasco-santos]

@zeim839 why not adding this validation in peerRouting.findPeer()? We should not look into self peerId anyway, so this should probably leave on the top level. If libp2p has Delegate Nodes + DHT configured, it will still try to go to the Delegates to find the self peer id.

[zeim839]

@vasco-santos I wasn't sure If that was possible because I was previously testing on @a1300 's version of libp2p. Once updated, I was able to add the validation as mentioned. I created a pull request to address this here

[vasco-santos]

Closed by #942 and will be shipped in libp2p@0.31.4