Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove unneeded peerbook puts #348

Merged
merged 6 commits into from
Apr 5, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .aegir.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const after = (done) => {
}

module.exports = {
bundlesize: { maxSize: '215kB' },
bundlesize: { maxSize: '217kB' },
hooks: {
pre: before,
post: after
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"libp2p-connection-manager": "~0.0.2",
"libp2p-floodsub": "~0.15.8",
"libp2p-ping": "~0.8.5",
"libp2p-switch": "~0.42.1",
"libp2p-switch": "~0.42.7",
"libp2p-websockets": "~0.12.2",
"mafmt": "^6.0.7",
"multiaddr": "^6.0.6",
Expand Down
5 changes: 0 additions & 5 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ class Node extends EventEmitter {
// reuse this muxed connection
this._switch.on('peer-mux-established', (peerInfo) => {
this.emit('peer:connect', peerInfo)
this.peerBook.put(peerInfo)
})

this._switch.on('peer-mux-closed', (peerInfo) => {
Expand Down Expand Up @@ -246,7 +245,6 @@ class Node extends EventEmitter {

this._switch.dial(peerInfo, protocol, (err, conn) => {
Copy link
Member

Choose a reason for hiding this comment

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

We can just pass the callback now. Lines below have to be removed

Suggested change
this._switch.dial(peerInfo, protocol, (err, conn) => {
this._switch.dial(peerInfo, protocol, callback)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, good call! I've updated those.

if (err) { return callback(err) }
this.peerBook.put(peerInfo)
callback(null, conn)
})
})
Expand Down Expand Up @@ -275,9 +273,6 @@ class Node extends EventEmitter {
if (err) { return callback(err) }

this._switch.dialFSM(peerInfo, protocol, (err, connFSM) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this._switch.dialFSM(peerInfo, protocol, (err, connFSM) => {
this._switch.dialFSM(peerInfo, protocol, callback)

if (!err) {
this.peerBook.put(peerInfo)
}
callback(err, connFSM)
})
})
Expand Down
10 changes: 4 additions & 6 deletions test/circuit-relay.node.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,9 @@ describe('circuit relay', () => {
nodeWS2 = node
cb()
}),
// set up node with TCP and listening on relay1
// set up node with TCP
(cb) => setupNode([
'/ip4/0.0.0.0/tcp/0',
`/p2p/${relayNode1.peerInfo.id.toB58String()}/p2p-circuit`
'/ip4/0.0.0.0/tcp/0'
], {
config: {
relay: {
Expand All @@ -128,10 +127,9 @@ describe('circuit relay', () => {
nodeTCP1 = node
cb()
}),
// set up node with TCP and listening on relay2 over TCP transport
// set up node with TCP
(cb) => setupNode([
'/ip4/0.0.0.0/tcp/0',
`/ip4/0.0.0.0/tcp/0/p2p/${relayNode2.peerInfo.id.toB58String()}/p2p-circuit`
'/ip4/0.0.0.0/tcp/0'
], {
config: {
relay: {
Expand Down
121 changes: 93 additions & 28 deletions test/peer-discovery.node.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,13 @@ const signalling = require('libp2p-webrtc-star/src/sig-server')
const parallel = require('async/parallel')
const crypto = require('crypto')

const PeerId = require('peer-id')
const PeerInfo = require('peer-info')

const createNode = require('./utils/create-node')
const echo = require('./utils/echo')

describe('peer discovery', () => {
let nodeA
let nodeB
let nodeC
let port = 24642
let ss

Expand Down Expand Up @@ -49,6 +47,15 @@ describe('peer discovery', () => {
nodeB = node
node.handle('/echo/1.0.0', echo)
node.start(cb)
}),
(cb) => createNode([
'/ip4/0.0.0.0/tcp/0',
`/ip4/127.0.0.1/tcp/${port}/ws/p2p-webrtc-star`
], options, (err, node) => {
expect(err).to.not.exist()
nodeC = node
node.handle('/echo/1.0.0', echo)
node.start(cb)
})
], done)
})
Expand All @@ -57,6 +64,7 @@ describe('peer discovery', () => {
parallel([
(cb) => nodeA.stop(cb),
(cb) => nodeB.stop(cb),
(cb) => nodeC.stop(cb),
(cb) => ss.stop(cb)
], done)
})
Expand Down Expand Up @@ -250,20 +258,31 @@ describe('peer discovery', () => {
peerDiscovery: {
mdns: {
enabled: true,
interval: 1e3, // discover quickly
// use a random tag to prevent CI collision
serviceTag: crypto.randomBytes(10).toString('hex')
}
}
}
})

it('find a peer', function (done) {
this.timeout(15 * 1000)
it('find peers', function (done) {
let expectedPeers = new Set([
nodeB.peerInfo.id.toB58String(),
nodeC.peerInfo.id.toB58String()
])

nodeA.once('peer:discovery', (peerInfo) => {
expect(nodeB.peerInfo.id.toB58String())
.to.eql(peerInfo.id.toB58String())
function finish () {
nodeA.removeAllListeners('peer:discovery')
expect(expectedPeers.size).to.eql(0)
done()
}

nodeA.on('peer:discovery', (peerInfo) => {
expectedPeers.delete(peerInfo.id.toB58String())
if (expectedPeers.size === 0) {
finish()
}
})
})
})
Expand All @@ -283,12 +302,24 @@ describe('peer discovery', () => {
}
})

it('find a peer', function (done) {
this.timeout(15 * 1000)
nodeA.once('peer:discovery', (peerInfo) => {
expect(nodeB.peerInfo.id.toB58String())
.to.eql(peerInfo.id.toB58String())
it('find peers', function (done) {
this.timeout(20e3)
let expectedPeers = new Set([
nodeB.peerInfo.id.toB58String(),
nodeC.peerInfo.id.toB58String()
])

function finish () {
nodeA.removeAllListeners('peer:discovery')
expect(expectedPeers.size).to.eql(0)
done()
}

nodeA.on('peer:discovery', (peerInfo) => {
expectedPeers.delete(peerInfo.id.toB58String())
if (expectedPeers.size === 0) {
finish()
}
})
})
})
Expand All @@ -302,6 +333,7 @@ describe('peer discovery', () => {
peerDiscovery: {
mdns: {
enabled: true,
interval: 1e3, // discovery quickly
// use a random tag to prevent CI collision
serviceTag: crypto.randomBytes(10).toString('hex')
},
Expand All @@ -312,12 +344,23 @@ describe('peer discovery', () => {
}
})

it('find a peer', function (done) {
this.timeout(15 * 1000)
nodeA.once('peer:discovery', (peerInfo) => {
expect(nodeB.peerInfo.id.toB58String())
.to.eql(peerInfo.id.toB58String())
it('find peers', function (done) {
let expectedPeers = new Set([
nodeB.peerInfo.id.toB58String(),
nodeC.peerInfo.id.toB58String()
])

function finish () {
nodeA.removeAllListeners('peer:discovery')
expect(expectedPeers.size).to.eql(0)
done()
}

nodeA.on('peer:discovery', (peerInfo) => {
expectedPeers.delete(peerInfo.id.toB58String())
if (expectedPeers.size === 0) {
finish()
}
})
})
})
Expand All @@ -332,24 +375,46 @@ describe('peer discovery', () => {
webRTCStar: {
enabled: false
}
},
dht: {
enabled: true,
kBucketSize: 20,
randomWalk: {
enabled: true,
queriesPerPeriod: 1,
interval: 1000, // start the query sooner
timeout: 3000
}
}
}
})

it('find a peer', function (done) {
this.timeout(15 * 1000)
it('find peers through the dht', function (done) {
let expectedPeers = new Set([
nodeB.peerInfo.id.toB58String(),
nodeC.peerInfo.id.toB58String()
])

nodeA.once('peer:discovery', (peerInfo) => {
expect(nodeB.peerInfo.id.toB58String())
.to.eql(peerInfo.id.toB58String())
function finish () {
nodeA.removeAllListeners('peer:discovery')
expect(expectedPeers.size).to.eql(0)
done()
}

nodeA.on('peer:discovery', (peerInfo) => {
expectedPeers.delete(peerInfo.id.toB58String())
if (expectedPeers.size === 0) {
finish()
}
})

// connect two dhts
const publicPeerId = new PeerId(nodeB._dht.peerInfo.id.id, null, nodeB._dht.peerInfo.id.pubKey)
const target = new PeerInfo(publicPeerId)
target.multiaddrs = nodeB._dht.peerInfo.multiaddrs
nodeA._dht.switch.dial(target, (err) => {
// Topology:
// A -> B
// C -> B
nodeA.dial(nodeB.peerInfo, (err) => {
expect(err).to.not.exist()
})
nodeC.dial(nodeB.peerInfo, (err) => {
expect(err).to.not.exist()
})
})
Expand Down