Skip to content

chore: update ipfs-http-client to the latest version #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jacobheun merged 2 commits into from
Apr 9, 2020
Merged

chore: update ipfs-http-client to the latest version #23

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
ee53933
chore: update ipfs-http-client to the latest version
achingbrain Apr 6, 2020
ef64c20
chore: specify minimum http client version
achingbrain Apr 8, 2020
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
19 changes: 15 additions & 4 deletions .aegir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
'use strict'

const createServer = require('ipfsd-ctl').createServer

const server = createServer()
const { createServer } = require('ipfsd-ctl')
let server

module.exports = {
hooks: {
browser: {
pre: () => server.start(),
pre: async () => {
server = createServer({
host: '127.0.0.1',
port: 57483
}, {
type: 'go',
ipfsHttpModule: require('ipfs-http-client'),
ipfsBin: require('go-ipfs-dep').path(),
test: true
})

await server.start()
},
post: () => server.stop()
}
}
Expand Down
9 changes: 5 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@
"coverage": "aegir coverage"
},
"devDependencies": {
"aegir": "^21.0.2",
"aegir": "^21.4.5",
"chai": "^4.2.0",
"dirty-chai": "^2.0.1",
"go-ipfs-dep": "^0.4.23",
"ipfsd-ctl": "^0.44.1"
"go-ipfs-dep": "0.4.23-3",
"ipfs-utils": "^1.2.1",
"ipfsd-ctl": "^3.0.0"
},
"dependencies": {
"debug": "^4.1.1",
"ipfs-http-client": "^42.0.0",
"ipfs-http-client": "^43.0.1",
"p-queue": "^6.2.1",
"peer-id": "~0.13.5",
"peer-info": "^0.17.1"
Expand Down
4 changes: 3 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ class DelegatedPeerRouting {
try {
return await this._httpQueue.add(async () => {
const { addrs } = await this.dht.findPeer(id, {
timeout: `${options.timeout}ms`// The api requires specification of the time unit (s/ms)
searchParams: {
timeout: `${options.timeout}ms`// The api requires specification of the time unit (s/ms)
}
Comment on lines +52 to +54
Copy link
Copy Markdown
Member

@hugomrdias hugomrdias Apr 7, 2020

Choose a reason for hiding this comment

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

Suggested change
searchParams: {
timeout: `${options.timeout}ms`// The api requires specification of the time unit (s/ms)
}
timeout: options.timeout

i was looking into this in vasco's PR and the string timeout doesn't work correctly for some reason.

Copy link
Copy Markdown
Member Author

@achingbrain achingbrain Apr 7, 2020

Choose a reason for hiding this comment

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

Hmm, are you sure? Against go-ipfs:

$ time curl http://localhost:5001/api/v0/dag/get?arg=/ipfs/QmaA9foTB68jrGgP3dBcyc8nrjbfAji8xrJsrJYo22BiVo\&timeout=5s
{"Message":"failed to get block for QmaA9foTB68jrGgP3dBcyc8nrjbfAji8xrJsrJYo22BiVo: context deadline exceeded","Code":0,"Type":"error"}

real	0m5.044s
user	0m0.006s
sys	0m0.015

$ time curl http://localhost:5001/api/v0/dag/get?arg=/ipfs/QmaA9foTB68jrGgP3dBcyc8nrjbfAji8xrJsrJYo22BiVo\&timeout=5000

real	0m0.057s
user	0m0.007s
sys	0m0.014s

$ time curl http://localhost:5001/api/v0/dag/get?arg=/ipfs/QmaA9foTB68jrGgP3dBcyc8nrjbfAji8xrJsrJYo22BiVo\&timeout=1

real	0m0.048s
user	0m0.007s
sys	0m0.017s

$ ipfs dag get /ipfs/QmaA9foTB68jrGgP3dBcyc8nrjbfAji8xrJsrJYo22BiVo --timeout 1
Error: time: missing unit in duration 1

Copy link
Copy Markdown
Member

@hugomrdias hugomrdias Apr 7, 2020

Choose a reason for hiding this comment

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

i was talking about the client timeout, with string the tests timeout instantly with number they run.

Copy link
Copy Markdown
Member

@vasco-santos vasco-santos Apr 8, 2020

Choose a reason for hiding this comment

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

Context: #25

I did not re-try this yet with the fix hugo's recommended

Copy link
Copy Markdown
Member Author

@achingbrain achingbrain Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think when these tests were written, we only had timeouts in a few HTTP method calls - the argument here is for the remote node, I think.

We could do both and set the timeout in the client and the server, but that could make it non-deterministic, unless we don't inspect which timeout was the one that timed out..

Copy link
Copy Markdown
Contributor

@jacobheun jacobheun Apr 9, 2020

Choose a reason for hiding this comment

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

Ideally we should perform set the timeout for the client and allow the cancel of that to cause the remote context to close. It sounds like there are some downstream issues with this at the moment though, so for now we can use the remote timeout to get around this until context cancelling is working appropriately.

We could do both and set the timeout in the client and the server

Definitely want to avoid that, it would be a nightmare to deal with.

})

const peerInfo = new PeerInfo(PeerId.createFromCID(id))
Expand Down
23 changes: 16 additions & 7 deletions test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,29 @@
const chai = require('chai')
const { expect } = chai
chai.use(require('dirty-chai'))
const IPFSFactory = require('ipfsd-ctl')
const { createFactory } = require('ipfsd-ctl')
const PeerID = require('peer-id')
const { isNode } = require('ipfs-utils/src/env')

const DelegatedPeerRouting = require('../src')
const factory = IPFSFactory.create({ type: 'go' })
const factory = createFactory({
type: 'go',
ipfsHttpModule: require('ipfs-http-client'),
ipfsBin: isNode ? require('go-ipfs-dep').path() : undefined,
test: true,
endpoint: 'http://localhost:57483'
})

async function spawnNode (boostrap = []) {
const node = await factory.spawn({
// Lock down the nodes so testing can be deterministic
config: {
Bootstrap: boostrap,
Discovery: {
MDNS: {
Enabled: false
ipfsOptions: {
config: {
Bootstrap: boostrap,
Discovery: {
MDNS: {
Enabled: false
}
}
}
}
Expand Down