Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

chore(dht): add API to allow options in findProvs() #337

Merged
merged 2 commits into from
Aug 2, 2018
Merged

chore(dht): add API to allow options in findProvs() #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
feb8cb2
chore(dht): add API to allow options in `findProvs()`
0x-r4bbit Jul 20, 2018
4269de1
fix: typo in test name
alanshaw Aug 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion SPEC/DHT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,13 @@ A great source of [examples][] can be found in the tests for this API.

##### `Go` **WIP**

##### `JavaScript` - ipfs.dht.findprovs(hash, [callback])
##### `JavaScript` - ipfs.dht.findprovs(hash, [options], callback])

Where `hash` is a multihash.

`options` an optional object with the following properties
- `timeout` - a maximum timeout in milliseconds

`callback` must follow `function (err, peerInfos) {}` signature, where `err` is an error if the operation was not successful. `peerInfos` is an array of objects of type [PeerInfo](https://github.com/libp2p/js-peer-info)

If no `callback` is passed, a promise is returned.
Expand All @@ -51,6 +54,8 @@ If no `callback` is passed, a promise is returned.

```JavaScript
ipfs.dht.findprovs(multihash, function (err, peerInfos) {})

ipfs.dht.findprovs(multihash, { timeout: 4000 }, function (err, peerInfos) {})
Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to add a test for this, but I'm not sure how to test this timeout.

Appreciate any pointers, in case anybody wants this as well :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd set the timeout really low and give it a hash that no-one will have, the callback should be called with an error before mocha times out the test.

Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm looking into this now and - sorry if this is a dumb question - but I'm wondering how I should know what hash would be a hash that no one will have? I mean I can't really know this upfront right?

Or is it enough to know, that no one of the connected peers will have it in the testing scenario, which in this case would be nodeB, I guess? Considering this already existing testing setup:

common.setup((err, factory) => {
        expect(err).to.not.exist()

        spawnNodesWithId(2, factory, (err, nodes) => {
          expect(err).to.not.exist()

          nodeA = nodes[0]
          nodeB = nodes[1]

          connect(nodeB, nodeA.peerId.addresses[0], done)
        })
})

What I've tried now, was to create an empty dagNode object within nodeB to get hold of its CID, but I don't actually announce the content in the network with provide():

it('should take options to override timout config', function (done) {
  const options = {
    timeout: 1
  }

  waterfall([
    (cb) => nodeB.object.new(cb),
    (dagNode, cb) => {

      // Notice I'm not announcing the hash in the network using `provide()`
      // but simply return CID

      const cidV0 = new CID(dagNode.toJSON().multihash)
      cb(cidV0)
    },
    (cidV0, cb) => nodeA.dht.findprovs(cidV0, options, (err) => {

      // I'd expect this to be truthy as nodeA shouldn't find a provider 
      // for the given hash

      expect(err).to.exist()
      cb(err)
    }),
  ], done) // this fails due to non-error being passed to done
})

So this test fails because done() doesn't get called with an error object, which means that findProvs doesn't throw and error here.

Can nodes find providers, even though other nodes didn't announce their content to the network?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that by adding content via object.new that you become a provider for that CID automatically.

We have a helper for the webui tests that creates a CID from random bytes without adding content to IPFS. Perhaps something similar here might work?

https://github.com/ipfs-shipyard/ipfs-webui/blob/revamp/test/helpers/cid.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that could work. I'll give it a spin

```

A great source of [examples][] can be found in the tests for this API.
Expand Down
25 changes: 25 additions & 0 deletions js/src/dht/findprovs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
/* eslint-env mocha */
'use strict'

const multihashing = require('multihashing-async')
const Crypto = require('crypto')
const waterfall = require('async/waterfall')
const CID = require('cids')
const { spawnNodesWithId } = require('../utils/spawn')
const { getDescribe, getIt, expect } = require('../utils/mocha')
const { connect } = require('../utils/swarm')

function fakeCid (cb) {
const bytes = Crypto.randomBytes(Math.round(Math.random() * 1000))
multihashing(bytes, 'sha2-256', (err, mh) => {
if (err) {
cb(err)
}
cb(null, new CID(0, 'dag-pb', mh))
})
}

module.exports = (createCommon, options) => {
const describe = getDescribe(options)
const it = getIt(options)
Expand Down Expand Up @@ -54,5 +66,18 @@ module.exports = (createCommon, options) => {
}
], done)
})

it('should take options to override timeout config', function (done) {
const options = {
timeout: 1
}
waterfall([
(cb) => fakeCid(cb),
(cidV0, cb) => nodeA.dht.findprovs(cidV0, options, (err) => {
expect(err).to.exist()
cb(null)
})
], done)
})
})
}
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"chai": "^4.1.2",
"cids": "~0.5.3",
"concat-stream": "^1.6.2",
"crypto": "^1.0.1",
"dirty-chai": "^2.0.1",
"hat": "0.0.3",
"ipfs-block": "~0.7.1",
Expand Down