feat: Ping #1342

daviddias · 2018-05-08T07:03:22Z

The long time coming promised API :)

Based on the work @JGAntunes, @vasco-santos and @melvin0008

depends on ipfs-inactive/js-ipfs-http-client#768

daviddias · 2018-05-08T07:35:08Z

@JGAntunes got the following error running the tests:

Debug: internal, implementation, error
    AssertionError: expected 400 to equal 200
    at api.inject (/Users/imp/code/js-ipfs/test/http-api/inject/ping.js:44:35)
    at Shot.inject (/Users/imp/code/js-ipfs/node_modules/hapi/lib/connection.js:329:16)
    at process.nextTick (/Users/imp/code/js-ipfs/node_modules/shot/lib/response.js:27:36)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)
        1) returns 200 and the response stream with the ping result

JGAntunes · 2018-05-08T09:30:09Z

Sorry @diasdavid, made a change to simplify the Joi validation object but turns out didn't work. Reverted it, should be fine now. This reminds me, any reason for why we are rewriting all the errors - https://github.com/ipfs/js-ipfs/blob/master/src/http/error-handler.js#L16? Just because this overwrites the Joi validation default response which is quite handy to be honest.

alanshaw · 2018-05-08T11:34:58Z

src/cli/commands/ping.js

+      argv.ipfs.pingPullStream(peerId, { count }),
+      pullCatch(err => {
+        throw err
+      }),


As far as I can tell pull-catch is for catching an error in the stream without ending the stream.

If you want to throw an error when it occurs then just remove this, drain will throw if you don't pass a second arg (done(err)). Or if you want to be explicit and/or do some logging, pass the second arg and then throw the error if it exists.

i.e.

pull( argv.ipfs.pingPullStream(peerId, { count }), drain(({ Time, Text }) => { /* ... */ }, (err) => { if (err) throw err }) )

Thanks @alanshaw I was a bit confused with how drain handled errors, hence the pull-catch. Cool 👍 going to update it.

alanshaw · 2018-05-08T11:36:58Z

src/cli/commands/ping.js

+'use strict'
+
+const pull = require('pull-stream/pull')
+const drain = require('pull-stream/sinks/drain')


Minor, you can just const pull = require('pull-stream') and then use pull() and pull.drain()

alanshaw · 2018-05-08T11:41:29Z

src/core/components/ping.js

+  let peer
+  try {
+    peer = libp2pNode.peerBook.get(peerId)
+    return cb(null, peer)


It's best not to call a callback in a try block because if the callback function throws, it'll be caught by the catch block below and you'll get phantom errors that are difficult to debug and even worse your callback will be called again in the catch block. Instead, return in the catch block and move the success callback call outside the try block.

alanshaw · 2018-05-08T11:42:57Z

src/core/components/ping.js

+function getPacket (msg) {
+  // Default msg
+  const basePacket = {Success: false, Time: 0, Text: ''}
+  return Object.assign({}, basePacket, msg)


Super minor, but you don't need to assign to an empty object here because you're creating a new basePacket for every call.

alanshaw · 2018-05-08T11:43:46Z

src/core/components/ping.js

+
+function runPing (libp2pNode, statusStream, count, peer, cb) {
+  libp2pNode.ping(peer, (err, p) => {
+    log('Got peer', peer)


Move after the if (err)? Might be misleading otherwise...

alanshaw · 2018-05-08T11:48:15Z

src/http/api/resources/ping.js

+          then: Joi.any().forbidden(),
+          otherwise: Joi.number().greater(0)
+        }),
+      count: Joi.number().greater(0),


Joi.number().integer()?

alanshaw · 2018-05-08T12:29:51Z

test/core/ping.spec.js

+        ipfsdA.api.pingPullStream(ipfsdBId, { count }),
+        pullCatch(err => {
+          expect(err).to.not.exist()
+        }),


Can this assertion be done in the drain callback?

alanshaw · 2018-05-08T12:30:36Z

test/core/ping.spec.js

+        pullCatch(err => {
+          expect(err).to.not.exist()
+        }),
+        drain(({ Success, Time, Text }) => {


Should these be being converted to lower case in JS land?

This would either require the HTTP API to be changed (and consequently be different from the Go implementation) or the js-ipfs-api module to lowercase the responses (the later looks like a nicer option tbh). I'm not sure how desirable this would be, taking interoperability with go-ipfs and such into account. @diasdavid any thoughts?

I believe the current precedent is for js-ipfs core and js-ipfs-api to be lowerCamelCase but js-ipfs http to be compatible with Go i.e. UpperCamelCase.

It means that the js-ipfs http layer needs to translate from lowerCamelCase to UpperCamelCase (example) and js-ipfs-api needs to translate responses from UpperCamelCase to lowerCamelCase (example).

@JGAntunes did you see this ^^?

Yep yep, sorry about it @alanshaw! Started looking at it yesterday evening, will finish it as soon I have some time 👍

@JGAntunes if you're busy I'm happy to finish this off on your behalf? You want to push what you've done?

@alanshaw sure thing, feel free to pick it up if you have some time. I hope I'm not blocking anyone or anything with this. I'm away from my laptop, but tbh I hadn't done much, besides looking at similar stuff like you pointed out, copying some pieces of code and doing an initial skeleton of a transform stream that processed the ping response (all of this in js-ipfs-api). I'm counting on having some time to deal with this later this evening, but please do pick it up if you have a chance, just let me know 👍

victorb · 2018-05-09T08:30:05Z

test/core/ping.spec.js

+      parallel([
+        ipfsdA.api.swarm.connect.bind(ipfsdA.api, bMultiaddr),
+        ipfsdB.api.swarm.connect.bind(ipfsdB.api, cMultiaddr)
+      ], (err) => setTimeout(() => done(err), 500)) // FIXME timeout needed for connections to succeed  


Instead of doing this, we should do a setInterval that times out after ~30 seconds or something like that. Inside the interval, we should check if both peers are connected with the other peer, and if they are, cancel the interval + timeout and move on. If the they are not connected after 30 seconds, fail the test. This makes the test rely less on artificial pauses in the test execution.

@JGAntunes ^^

Sure, I'll pick this up meanwhile, as soon as I have some time.

Should be fixed now @victorbjelkholm @diasdavid, let me know what you think - https://github.com/ipfs/js-ipfs/pull/1342/files#diff-0e68633477fac5c3c897f68c6b28f7a9R177

alanshaw · 2018-05-14T11:18:01Z

I'm 👀 at this now

alanshaw · 2018-05-14T13:19:10Z

Ok, so in this PR in core we have:
* ping(peer, opts, callback<err, pullStream>)

In js-ipfs-api we have already:
* ping(peer, opts, callback<err, Array<{success, text, time}>)
* pingReadableStream(peer, opts): Readable
* pingPullStream(peer, opts): PullStream

I'm currently changing core.ping in this PR to work like the API and adding the missing core.pingReadableStream and core.pingPullStream.

@JGAntunes it would be grand if you could open a PR against interface-ipfs-core with tests for these and a spec doc.

JGAntunes · 2018-05-14T13:45:41Z

Alright! That makes sense, only now I've figured that the js-ipfs core components should match the same API provided by the js-ipfs-api 😅 sorry about it, would have added it if I knew.

Ok @alanshaw, I'll look into interface-ipfs-core in the evening and also submit the fix for the connection setTimeout @victorbjelkholm mentioned.

alanshaw · 2018-05-14T14:56:56Z

Thanks @JGAntunes, this branch should be good to create tests against now - it has the proposed core API :D

Good luck! 🚀 🚀 🚀

alanshaw · 2018-05-15T08:12:40Z

@JGAntunes did you get anywhere last night with this? If not I can spend some time today porting these over to interface-ipfs-core. It's the last thing to do on the 0.29 release checklist.

JGAntunes · 2018-05-15T10:24:48Z

Hey @alanshaw, sorry, no. Please do if you have some time 👍 I'll see if I can fix the setTimeout test meanwhile

daviddias · 2018-05-16T06:43:49Z

@JGAntunes mind rebasing master onto this branch to get CI green?

JGAntunes · 2018-05-16T10:57:58Z

Happy to rebase @diasdavid but CI wont go green until ipfs-inactive/js-ipfs-http-client#762 is merged and the interface-ipfs-core is updated. I'll see if I can look into it in the next couple of days.

alanshaw · 2018-05-16T11:13:32Z

@JGAntunes all good, I'm just finishing off the interface-ipfs-core tests and will merge ipfs-inactive/js-ipfs-http-client#762, rebase this and then merge this.

JGAntunes · 2018-05-16T11:15:00Z

Thanks @alanshaw! 🙌

* feat: implement `ipfs ping` flags #928 * feat: first shot at ping implementaion * fix: ETOOMANYSTREAMS 😢 * chore: cleanup on the ping component * chore: ping component linting * chore: bump js-ipfs-api and fix http ping validation * chore: add test to ping cli command * chore: add ping cli test * chore: refactor ping component and some cleanup * chore: add tests to the ping http API * fix: no need to check for peerRouting method in ping * chore: add tests for ping core functionality

daviddias · 2018-05-20T19:32:55Z

@alanshaw I've updated deps but CI is still pretty angry. Can you check what's up?

alanshaw · 2018-05-21T09:30:59Z

Yep on it

ghost assigned daviddias May 8, 2018

ghost added the status/in-progress In progress label May 8, 2018

daviddias mentioned this pull request May 8, 2018

⚡️ v0.29.0 RELEASE 🚀 #1320

Closed

30 tasks

daviddias force-pushed the feat/ping branch from c3665cc to ac4d8a9 Compare May 8, 2018 07:21

ghost assigned JGAntunes May 8, 2018

alanshaw suggested changes May 8, 2018

View reviewed changes

victorb suggested changes May 9, 2018

View reviewed changes

JGAntunes mentioned this pull request May 14, 2018

fix(ping): convert the ping messages to lowercase ipfs-inactive/js-ipfs-http-client#762

Closed

ghost assigned alanshaw May 14, 2018

JGAntunes and others added 2 commits May 16, 2018 14:20

chore: update deps

94c3db6

alanshaw force-pushed the feat/ping branch from 377e329 to cad0288 Compare May 16, 2018 13:21

daviddias and others added 4 commits May 21, 2018 10:46

fix: simplify circle

b052b7d

chore: move ping http validation back to joi alternatives

e6a9878

chore: remove useless pull-catch dep

e8508a4

chore: no need to require the pull module twice

6336256

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Ping #1342

feat: Ping #1342

daviddias commented May 8, 2018 •

edited by alanshaw

Loading

daviddias commented May 8, 2018

JGAntunes commented May 8, 2018

alanshaw May 8, 2018

JGAntunes May 8, 2018

alanshaw May 8, 2018

alanshaw May 8, 2018 •

edited

Loading

alanshaw May 8, 2018

alanshaw May 8, 2018

alanshaw May 8, 2018

alanshaw May 8, 2018

alanshaw May 8, 2018

JGAntunes May 8, 2018

alanshaw May 9, 2018 •

edited

Loading

alanshaw May 11, 2018

JGAntunes May 11, 2018

alanshaw May 11, 2018

JGAntunes May 11, 2018

victorb May 9, 2018

daviddias May 10, 2018

JGAntunes May 10, 2018

JGAntunes May 15, 2018

alanshaw commented May 14, 2018

alanshaw commented May 14, 2018 •

edited

Loading

JGAntunes commented May 14, 2018 •

edited

Loading

alanshaw commented May 14, 2018

alanshaw commented May 15, 2018

JGAntunes commented May 15, 2018

daviddias commented May 16, 2018

JGAntunes commented May 16, 2018

alanshaw commented May 16, 2018

JGAntunes commented May 16, 2018

daviddias commented May 20, 2018

alanshaw commented May 21, 2018

feat: Ping #1342

feat: Ping #1342

Conversation

daviddias commented May 8, 2018 • edited by alanshaw Loading

daviddias commented May 8, 2018

JGAntunes commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw commented May 14, 2018

alanshaw commented May 14, 2018 • edited Loading

JGAntunes commented May 14, 2018 • edited Loading

alanshaw commented May 14, 2018

alanshaw commented May 15, 2018

JGAntunes commented May 15, 2018

daviddias commented May 16, 2018

JGAntunes commented May 16, 2018

alanshaw commented May 16, 2018

JGAntunes commented May 16, 2018

daviddias commented May 20, 2018

alanshaw commented May 21, 2018

daviddias commented May 8, 2018 •

edited by alanshaw

Loading

alanshaw May 8, 2018 •

edited

Loading

alanshaw May 9, 2018 •

edited

Loading

alanshaw commented May 14, 2018 •

edited

Loading

JGAntunes commented May 14, 2018 •

edited

Loading