-
Notifications
You must be signed in to change notification settings - Fork 109
Conversation
Depends on ipfs/js-ipfs#960 |
Two things I hit during writing these tests:
|
That is strange. Can you replicate the same behaviour using just TCP sockets in a simple Node.js script?
This should not be the case, but it certainly happens in the 'dialing dance' inside libp2p-swarm. This work -- https://github.com/libp2p/js-libp2p-swarm/issues/24 -- would solve this. Can you make sure to add a test for this case (a failing one) commented out and pointing to that issue so that we know what's going on? |
(cb) => { | ||
// Waiting to make sure nodes are connected | ||
setTimeout(cb, 1000) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary for nodeA.swarm.peers or for nodeB.swarm.peers?
src/swarm.js
Outdated
// Need to explicitly declare the ports as 0 doesn't work with | ||
// multiple swarm addresses apparently | ||
// Not sure how to deal with the case if the ports are busy, this | ||
// is introducing flakyness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really weird to me. See #150 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case to reproduce the issue: libp2p/js-libp2p-switch#227
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a TODO here and create an issue to track this so that we can merge this PR as it is now.
Made some small test program but no problem there. Made a test-case for libp2p-swarm though that is currently failing, you can see it here: libp2p/js-libp2p-switch#227 Guessing there is some de-duplication we're doing that removes the second one since it's exactly the same as the first address being added. |
@victorbjelkholm can you confirm that:
|
@diasdavid yes to both of them |
Not mergable yet