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

feat: implement ipfs ping flags #928 #1202

Closed
wants to merge 1 commit into from

Conversation

melvin0008
Copy link

Attempt to solve #928

  • Add Tests
  • Look for a better way to use the ping object to reduce HTTP calls
  • Add fake delay between ping
  • If none of the peers are initialized it gives Circuit is not enabled error.
  • Better error handling

@diasdavid Let me know your thoughts on the same

@melvin0008 melvin0008 force-pushed the melvin0008_feat_ping branch from 293bcf9 to 0dba866 Compare February 2, 2018 09:21

for (let i = 0; i < count; i++) {
argv.ipfs.ping(peerId, pingCb)
}
Copy link
Member

Choose a reason for hiding this comment

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

A ping with count 10 is a ping with 10 packets, not 10 ping calls. You need to count the number of pings that come in p.

Copy link
Author

@melvin0008 melvin0008 Feb 18, 2018

Choose a reason for hiding this comment

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

I agree that 10 ping calls is wrong. I started refactoring the code and was able to use the ping object and stream the times to the api.

But I have run into a problem. In js-ipfs-api the response assumes there will be one ping request.

So even if I stream multiple ping responses only the first one is returned.

My understanding of the flow when someone types jsipfs ping is

  1. cli/commands/ping.js -> this calls the ipfs-api ping
  2. node_modules/ipfs-api/src/ping.js -> this calls the http api ping
  3. http/api/resources/ping.js -> this calls the component ping
  4. core/components/ping.js which would then return the ping object
  5. http/api/resources/ping.js would then stream the times to the ipfs-api and end the stream when number of pings are complete.

The problem is js-ipfs-api is expecting only one response ping object. Is there a workaround I'm missing or should I create an issue inn js-ipfs-api ping and fix that. I just want to confirm whether you its an issue or not. :)

Also, irrespective of the api I was able to get the curl outputs for both the go-ipfs http api and js-ipfs http api to be the same . I have attached images for the same. But again I find one inconsistency . Since we are using new Date() in js-libp2p-ping we are not getting the same unit as go-ipfs. go-ipfs returns in nanosecond and js-ipfs returns the time in milliseconds. Let me know whether I should create a issue and fix that. I would be glad to do it. I just want to confirm whether its an issue or not.

Curl response to js-ipfs
screen shot 2018-02-17 at 6 37 38 pm

Curl response to go-ipfs
screen shot 2018-02-17 at 6 36 28 pm

Copy link
Member

Choose a reason for hiding this comment

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

Is there a workaround I'm missing or should I create an issue inn js-ipfs-api ping and fix that.

Sounds like it needs to be fixed in js-ipfs-api then. Just go for it too.

Copy link
Member

Choose a reason for hiding this comment

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

Seems that @ngotchac noticed a misbehavior a while ago too ipfs-inactive/js-ipfs-http-client#556

@daviddias
Copy link
Member

@melvin0008 still looking into finishing this PR?

@melvin0008
Copy link
Author

@diasdavid Yes will do it the coming weekend. Sorry for the wait

@daviddias daviddias added the status/in-progress In progress label Feb 19, 2018
@daviddias daviddias added exp/novice Someone with a little familiarity can pick up P2 Medium: Good to have, but can wait until someone steps up exp/expert Having worked on the specific codebase is important status/ready Ready to be worked and removed exp/novice Someone with a little familiarity can pick up status/in-progress In progress labels Mar 14, 2018
@vasco-santos vasco-santos added status/in-progress In progress and removed status/ready Ready to be worked labels Apr 5, 2018
@JGAntunes JGAntunes mentioned this pull request Apr 6, 2018
2 tasks
@daviddias
Copy link
Member

Let's coalesce the energies here on #1299

@daviddias daviddias closed this Apr 30, 2018
@ghost ghost removed the status/in-progress In progress label Apr 30, 2018
@daviddias daviddias mentioned this pull request May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/expert Having worked on the specific codebase is important P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants