Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: ping #1299

Merged
merged 12 commits into from
May 8, 2018
Merged

feat: ping #1299

merged 12 commits into from
May 8, 2018

Conversation

JGAntunes
Copy link
Member

@JGAntunes JGAntunes commented Apr 6, 2018

Just opening this for visibility, working on finishing the ping endeavour started at #1202.

Also, some relevant notes. Seems like hapijs has compression enabled by default, which means responses will take into account the Accept-Encoding header and will use gzip to compress responses (it seems hapi has no way of disabling compression for a specific route only also). From what I've seen the go-ipfs http api doesn't gzip responses by default AFAIK. Anyway, there seems to be an issue when compression is enabled and we're streaming responses - hapijs/hapi#3599 - as it seems the response is actually buffered, compressed and only then it is sent to the client. This results in the actual "streaming" capabilities being lost (the response all at once and only when it is complete). Not sure if this is a relevant issue or not, but though it was worth mentioning since when using the http API from a browser, the Accept-Encoding headers will be set and the resulting response will not be streamed.

Closes #928.

@JGAntunes JGAntunes added the status/in-progress In progress label Apr 6, 2018
@ghost ghost assigned JGAntunes Apr 6, 2018
@daviddias
Copy link
Member

I believe that meanwhile @vasco-santos started tackling this one as well. @JGAntunes mind syncing up with him to avoid dup work and find a converged solution? Thank you both!

@JGAntunes
Copy link
Member Author

Sure, @vasco-santos I'm currently finishing the tests for this and also doing some cleanup. I needed something to test the connections between 2 IPFS nodes hence why I'm working on this. Let me know what's the status on this from your side, I'll gladly drop this if you have something ready already 👍 or we can also work together if you like.

@vasco-santos
Copy link
Member

vasco-santos commented Apr 9, 2018

Hello @JGAntunes

I started this on Friday, but I didn't finish it yet. I was implementing using the pull stream (as you did too), but I didn't notice that you had created libp2p/js-libp2p-ping#69, which would be useful for me 😄
After finishing that, I would have to add the tests, as well. Therefore, I think you can keep with this PR.

I cloned your repo, and linked your PR to js-libp2p-ping and tested your implementation and it seems good to me, good job!

Just several notes that I was encompassing in my solution:

module.exports = {
command: 'ping <peerId>',

describe: 'Measure the latency of a connection',
Copy link
Member

@vasco-santos vasco-santos Apr 9, 2018

Choose a reason for hiding this comment

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

There are several command definitions using describe and others using description.

According to yargs docs, I think that the correct usage in this case is description. In spite of, describe also working, it is aimed at another use cases, as documented here.

What do you think @JGAntunes and @diasdavid ?

Copy link
Member

Choose a reason for hiding this comment

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

fine either way, pick only one though, consistency is key.

@JGAntunes
Copy link
Member Author

Hey @diasdavid, @vasco-santos . I was looking to finish this asap but I have some questions regarding tests. Noticed the interface-ipfs-core is responsible for running tests for the core components, seems though that these aren't being run for any of the miscellaneous operations I think - https://github.com/ipfs/interface-ipfs-core/blob/master/js/src/miscellaneous.js - https://github.com/ipfs/js-ipfs/tree/master/test/core/interface, is this by design or is it just something that's lacking?

There seems to be an issue when running a ping command with a node with whom we don't have an established connection yet - https://github.com/libp2p/js-libp2p-ping/issues/70 - hence the need for this on tests - https://github.com/ipfs/js-ipfs/pull/1299/files#diff-9478661c5bacfe4dd5f87a3b88574ca9R69

As for the http-api tests I've noticed you have both using the inject interface from hapi and js-ipfs as a client. What's the angle on this? Should both be used ideally? If so where will the major differences rely?

@vasco-santos
Copy link
Member

@JGAntunes

Regarding running a ping command with a node with no previous established connection, the GO implementation works correctly.
However, when I was implementing this, I only achieved a successful ping after connecting to the peer, as well. I don't know if this is the intended behavior or not.

For your other questions, it is better waiting for a @diasdavid answer.

@daviddias daviddias mentioned this pull request Apr 23, 2018
30 tasks
@daviddias
Copy link
Member

However, when I was implementing this, I only achieved a successful ping after connecting to the peer, as well. I don't know if this is the intended behavior or not.

That is because js-ipfs doesn't have the DHT by default, which gives peers Peer Routing. Learn more about it here:

@daviddias
Copy link
Member

@JGAntunes @vasco-santos merged and released libp2p/js-libp2p-ping#69, please update https://github.com/libp2p/js-libp2p and make sure that all tests pass, then we can update all the deps here :)

@JGAntunes
Copy link
Member Author

JGAntunes commented May 6, 2018

Sorry for delay on this everyone. I think the only thing missing now are the tests on the HTTP API using js-ipfs-api. Meanwhile, during ping I now try to use peerRouting.findPeer which will fail accordingly if the DHT experimental feature is not enabled:

{"Success":true,"Time":0,"Text":"Looking up peer QmSoLnSGccFuZQJzRadHn95W2CrSFmZuTdDWP8HXaHca9z"}
{"Success":false,"Time":0,"Text":"Error: DHT is not available"}

Will add the missing tests between today and tomorrow. Let me know if is anything else missing.

@JGAntunes JGAntunes changed the title [WIP] feat: ping feat: ping May 8, 2018
@JGAntunes
Copy link
Member Author

Added tests for both DHT enabled and disabled. However couldn't find a workaround for this - https://github.com/ipfs/js-ipfs/pull/1299/files#diff-0e68633477fac5c3c897f68c6b28f7a9R187

This is also dependent on #1340

Let me know what you think @diasdavid @vasco-santos

@daviddias daviddias changed the base branch from master to feat/ping May 8, 2018 06:58
@daviddias
Copy link
Member

Merging this PR into a branch on the main repo so that I can update and test things.

@daviddias daviddias merged commit 7bc90cc into ipfs:feat/ping May 8, 2018
@ghost ghost removed the status/in-progress In progress label May 8, 2018
@daviddias daviddias mentioned this pull request May 8, 2018
daviddias pushed a commit that referenced this pull request May 8, 2018
* 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
alanshaw pushed a commit that referenced this pull request May 16, 2018
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants