fix: dont use peerinfo distinct (#334)

* fix: dont use peerinfo distinct * refactor: remove unneeded code * refactor: clean up * refactor: fix feedback
libp2p · Apr 16, 2019 · 9af0e3a · 9af0e3a
1 parent 951e0c9
commit 9af0e3a
Show file tree

Hide file tree

Showing 3 changed files with 169 additions and 4 deletions.
diff --git 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
@@ -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
@@ -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()
+      })
+    })
+  })
 })