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

util: waitUntilConnected #310

Closed
travisperson opened this issue Jun 25, 2018 · 1 comment · Fixed by #311
Closed

util: waitUntilConnected #310

travisperson opened this issue Jun 25, 2018 · 1 comment · Fixed by #311
Assignees
Labels

Comments

@travisperson
Copy link
Contributor

I've been going through the code and identifying patterns or ways to help clean up the code to make things easier to read and understand.

(Note: I've been working off of the in-progress PR #290)

There are a few tests that require multiple nodes to be connected (ping, pubsub, dht).

Running through this I found this setup code for ping, which uses a method waitUntilConnected, which doesn't actually connect any nodes, but instead just waits around till the nodes find each other (as far as I can tell).

https://github.com/ipfs/interface-ipfs-core/blob/f238e9f0ba8132dba51d3bac1eb491701f505ccd/js/src/ping.js#L38-L52

It appears that it heavily relies on peer discovery, which is dependent on how the node is configured, particularity for js-ipfs-core.

@alanshaw I believe you wrote this code. I believe it was added so that the tests itself did not have to attempt to find and connect to the peers, and instead could rely on the peers already being connected before conducting the pings.

Any reasons to use waitUntilConnected over simply calling ipfs.swarm.connect?

@alanshaw
Copy link
Contributor

In hindsight I think this was a mistake and I'm going to send a PR to just connect the nodes that need to be connected, if the test is not explicitly testing discovery.

To phrase it another way, we don't want to have to skip tests that are testing features that aren't discovery related.

I've come to appreciate the state of discovery a whole lot more. In windows AFAIK there's no MDNS so waiting could take an indeterminate amount of time while the DHT bootstraps and our peers get registered on it and there's also this weirdness where in 0.4.14+ our spawned nodes never acquire any peers.

alanshaw added a commit that referenced this issue Jun 26, 2018
fixes #310

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@ghost ghost added in progress and removed ready labels Jun 26, 2018
@ghost ghost removed the in progress label Jun 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants