ipfs.dht.findprovs is passing cb in timeout position to libp2p findProviders

[olizilla]

• Version: 0.28.2
• Platform: all
• Subsystem: dht

Type: Bug

Severity: High

Description:

ipfs.dht.findprovs is passing cb in timeout position to libp2p findProviders, and so the call fails to return anything.

In the dht component in js-ipfs

js-ipfs/src/core/components/dht.js

Line 64 in 37801c1

self._libp2pNode.contentRouting.findProviders(key, callback)

and the js-libp2p findProviders impl https://github.com/libp2p/js-libp2p/blob/03faf69212b0f3501291527f85b2bb85a8c5427e/src/content-routing.js#L5

Steps to reproduce the error:

In a browser with ipfs-companion installed and pointing at an embedded ipfs node:

ipfs.dht.findprovs('QmYPNmahJAvkMTU6tDx5zvhEkoLzEFeTDz6azDCSNqzKkW', console.log)
// Error: callback is not a function at Object.findProviders

jsipfs findprovs QmYPNmahJAvkMTU6tDx5zvhEkoLzEFeTDz6azDCSNqzKkW
// returns nothing, but exits cleanly

[daviddias]

@olizilla wanna submit a PR to fix that? thanks for reporting :)

[olizilla]

Sure! I think an ideal fix for this would result in a change to the interface; the caller to findprovs should be able to pass in options to specify a timeout duration. Is that a reasonable approach @diasdavid ? What's the proceedure for proposing an interface change?

…

On Mon, 30 Apr 2018, 08:29 David Dias, ***@***.***> wrote: @olizilla <https://github.com/olizilla> wanna submit a PR to fix that? thanks for reporting :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1322 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADl91Lg5yQZtOBMfc8_absj--wDsegnks5ttr1GgaJpZM4Tf9NF> .

[daviddias]

The drill is to makes sure there is a PR and a test that standardizes the API call on https://github.com/ipfs/interface-ipfs-core and that it passes both js-ipfs and js-ipfs-api. Thanks @olizilla 👍

[0x-r4bbit]

@olizilla have you started implementing this already, or do you need any help with this?

[olizilla]

@PascalPrecht I haven't started on this, please feel free to PR it if you're looking at it!

[0x-r4bbit]

@olizilla I'll do that.

@diasdavid just to clarify the API. We wanna turn:

    findprovs: promisify((key, callback) => {

to

    findprovs: promisify((key, options, callback) => {

where options can have a timeout property, is that correct?

[0x-r4bbit]

I guess on that note, we will have to introduce a decent default for timeout

[0x-r4bbit]

@alanshaw do you know what the timeout default has to be here?

As https://github.com/libp2p/js-libp2p/blob/03faf69212b0f3501291527f85b2bb85a8c5427e/src/content-routing.js#L5 expects timeout in any case, we should have a default when callback is passed but no options.

I would also update interface-ipfs-core's spec accordingly (which doesn't provide that information either).

[alanshaw]

Unless we want to explicitly set a default when not provided (IMHO I don't think we need to) I'd leave it to the implementation to set an appropriate value (you can pass null).

https://github.com/libp2p/js-libp2p-kad-dht/blob/012d2c3b30f3eb59a2ed68606090cd9757bfde52/src/index.js#L177-L197

[0x-r4bbit]

Ah perfect, okay. Thanks!