From 9af0e3ac7d811de11570e02f48d0461d97bdb964 Mon Sep 17 00:00:00 2001 From: Jacob Heun Date: Tue, 16 Apr 2019 12:43:26 +0200 Subject: [PATCH] fix: dont use peerinfo distinct (#334) * fix: dont use peerinfo distinct * refactor: remove unneeded code * refactor: clean up * refactor: fix feedback --- src/transport.js | 16 +++- src/utils.js | 11 +++ test/transport-manager.spec.js | 146 +++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 4 deletions(-) diff --git a/src/transport.js b/src/transport.js index cd15af2..ce0d298 100644 --- a/src/transport.js +++ b/src/transport.js @@ -9,6 +9,7 @@ const log = debug('libp2p:switch:transport') const LimitDialer = require('./limit-dialer') const { DIAL_TIMEOUT } = require('./constants') +const { uniqueBy } = require('./utils') // number of concurrent outbound dials to make per peer, same as go-libp2p-swtch const defaultPerPeerRateLimit = 8 @@ -124,10 +125,17 @@ class TransportManager { handler = this.switch._connectionHandler(key, handler) const transport = this.switch.transports[key] - const multiaddrs = TransportManager.dialables( - transport, - this.switch._peerInfo.multiaddrs.distinct() - ) + let originalAddrs = this.switch._peerInfo.multiaddrs.toArray() + + // Until TCP can handle distinct addresses on listen, https://github.com/libp2p/interface-transport/issues/41, + // make sure we aren't trying to listen on duplicate ports. This also applies to websockets. + originalAddrs = uniqueBy(originalAddrs, (addr) => { + // Any non 0 port should register as unique + const port = Number(addr.toOptions().port) + return isNaN(port) || port === 0 ? addr.toString() : port + }) + + const multiaddrs = TransportManager.dialables(transport, originalAddrs) if (!transport.listeners) { transport.listeners = [] diff --git a/src/utils.js b/src/utils.js index 2de2541..55e0494 100644 --- a/src/utils.js +++ b/src/utils.js @@ -47,3 +47,14 @@ module.exports.identifyDialer = (connection, cryptoPeerInfo) => { }) }) } + +/** + * Get unique values from `arr` using `getValue` to determine + * what is used for uniqueness + * @param {Array} arr The array to get unique values for + * @param {function(value)} getValue The function to determine what is compared + * @returns {Array} + */ +module.exports.uniqueBy = (arr, getValue) => { + return [...new Map(arr.map((i) => [getValue(i), i])).values()] +} diff --git a/test/transport-manager.spec.js b/test/transport-manager.spec.js index 9a5e645..82bdd85 100644 --- a/test/transport-manager.spec.js +++ b/test/transport-manager.spec.js @@ -7,10 +7,15 @@ const expect = chai.expect chai.use(dirtyChai) const Multiaddr = require('multiaddr') const PeerInfo = require('peer-info') +const sinon = require('sinon') const TransportManager = require('../src/transport') describe('Transport Manager', () => { + afterEach(() => { + sinon.restore() + }) + describe('dialables', () => { let peerInfo const dialAllTransport = { filter: addrs => addrs } @@ -141,4 +146,145 @@ describe('Transport Manager', () => { expect(dialableAddrs[0].toString()).to.equal('/ip6/::1/tcp/4001') }) }) + + describe('listen', () => { + const listener = { + once: function () {}, + listen: function () {}, + removeListener: function () {}, + getAddrs: function () {} + } + + it('should allow for multiple addresses with port 0', (done) => { + const mockListener = sinon.stub(listener) + mockListener.listen.callsArg(1) + mockListener.getAddrs.callsArgWith(0, null, []) + const mockSwitch = { + _peerInfo: { + multiaddrs: { + toArray: () => [ + Multiaddr('/ip4/127.0.0.1/tcp/0'), + Multiaddr('/ip4/0.0.0.0/tcp/0') + ], + replace: () => {} + } + }, + _options: {}, + _connectionHandler: () => {}, + transports: { + TCP: { + filter: (addrs) => addrs, + createListener: () => { + return mockListener + } + } + } + } + const transportManager = new TransportManager(mockSwitch) + transportManager.listen('TCP', null, null, (err) => { + expect(err).to.not.exist() + expect(mockListener.listen.callCount).to.eql(2) + done() + }) + }) + + it('should filter out equal addresses', (done) => { + const mockListener = sinon.stub(listener) + mockListener.listen.callsArg(1) + mockListener.getAddrs.callsArgWith(0, null, []) + const mockSwitch = { + _peerInfo: { + multiaddrs: { + toArray: () => [ + Multiaddr('/ip4/127.0.0.1/tcp/0'), + Multiaddr('/ip4/127.0.0.1/tcp/0') + ], + replace: () => {} + } + }, + _options: {}, + _connectionHandler: () => {}, + transports: { + TCP: { + filter: (addrs) => addrs, + createListener: () => { + return mockListener + } + } + } + } + const transportManager = new TransportManager(mockSwitch) + transportManager.listen('TCP', null, null, (err) => { + expect(err).to.not.exist() + expect(mockListener.listen.callCount).to.eql(1) + done() + }) + }) + + it('should account for addresses with no port', (done) => { + const mockListener = sinon.stub(listener) + mockListener.listen.callsArg(1) + mockListener.getAddrs.callsArgWith(0, null, []) + const mockSwitch = { + _peerInfo: { + multiaddrs: { + toArray: () => [ + Multiaddr('/p2p-circuit'), + Multiaddr('/p2p-websocket-star') + ], + replace: () => {} + } + }, + _options: {}, + _connectionHandler: () => {}, + transports: { + TCP: { + filter: (addrs) => addrs, + createListener: () => { + return mockListener + } + } + } + } + const transportManager = new TransportManager(mockSwitch) + transportManager.listen('TCP', null, null, (err) => { + expect(err).to.not.exist() + expect(mockListener.listen.callCount).to.eql(2) + done() + }) + }) + + it('should filter out addresses with the same, non 0, port', (done) => { + const mockListener = sinon.stub(listener) + mockListener.listen.callsArg(1) + mockListener.getAddrs.callsArgWith(0, null, []) + const mockSwitch = { + _peerInfo: { + multiaddrs: { + toArray: () => [ + Multiaddr('/ip4/127.0.0.1/tcp/8000'), + Multiaddr('/dnsaddr/libp2p.io/tcp/8000') + ], + replace: () => {} + } + }, + _options: {}, + _connectionHandler: () => {}, + transports: { + TCP: { + filter: (addrs) => addrs, + createListener: () => { + return mockListener + } + } + } + } + const transportManager = new TransportManager(mockSwitch) + transportManager.listen('TCP', null, null, (err) => { + expect(err).to.not.exist() + expect(mockListener.listen.callCount).to.eql(1) + done() + }) + }) + }) })