fix: avoid sync callback in async functions (libp2p#297)

* fix: avoid sync callback in async functions

* test: add error check

* refactor: clean up async usage

* chore: clean up

* refactor: remove async waterfall usage on identify

* chore: fix linting

Author: jacobheun

src/connection/manager.js

@@ -2,11 +2,11 @@
 
 const identify = require('libp2p-identify')
 const multistream = require('multistream-select')
-const waterfall = require('async/waterfall')
 const debug = require('debug')
 const log = debug('libp2p:switch:conn-manager')
 const once = require('once')
 const ConnectionFSM = require('../connection')
+const { msHandle, msSelect, identifyDialer } = require('../utils')
 
 const Circuit = require('libp2p-circuit')
 
@@ -136,34 +136,33 @@ class ConnectionManager {
           }
 
           // overload peerInfo to use Identify instead
-          conn.getPeerInfo = (callback) => {
+          conn.getPeerInfo = async (callback) => {
             const conn = muxedConn.newStream()
             const ms = new multistream.Dialer()
             callback = once(callback)
 
-            waterfall([
-              (cb) => ms.handle(conn, cb),
-              (cb) => ms.select(identify.multicodec, cb),
-              // run identify and verify the peer has the same info from crypto
-              (conn, cb) => identify.dialer(conn, cryptoPI, cb),
-              (peerInfo, observedAddrs, cb) => {
-                observedAddrs.forEach((oa) => {
-                  this.switch._peerInfo.multiaddrs.addSafe(oa)
-                })
-                cb(null, peerInfo)
-              }
-            ], (err, peerInfo) => {
-              if (err) {
-                return muxedConn.end(() => {
-                  callback(err, null)
-                })
-              }
-
-              if (peerInfo) {
-                conn.setPeerInfo(peerInfo)
-              }
-              callback(err, peerInfo)
-            })
+            let results
+            try {
+              await msHandle(ms, conn)
+              const msConn = await msSelect(ms, identify.multicodec)
+              results = await identifyDialer(msConn, cryptoPI)
+            } catch (err) {
+              return muxedConn.end(() => {
+                callback(err, null)
+              })
+            }
+
+            const { peerInfo, observedAddrs } = results
+
+            for (var i = 0; i < observedAddrs.length; i++) {
+              var addr = observedAddrs[i]
+              this.switch._peerInfo.multiaddrs.addSafe(addr)
+            }
+
+            if (peerInfo) {
+              conn.setPeerInfo(peerInfo)
+            }
+            callback(null, peerInfo)
           }
 
           conn.getPeerInfo((err, peerInfo) => {

src/utils.js

@@ -0,0 +1,49 @@
+'use strict'
+
+const Identify = require('libp2p-identify')
+
+/**
+ * For a given multistream, registers to handle the given connection
+ * @param {MultistreamDialer} multistream
+ * @param {Connection} connection
+ * @returns {Promise}
+ */
+module.exports.msHandle = (multistream, connection) => {
+  return new Promise((resolve, reject) => {
+    multistream.handle(connection, (err) => {
+      if (err) return reject(err)
+      resolve()
+    })
+  })
+}
+
+/**
+ * For a given multistream, selects the given protocol
+ * @param {MultistreamDialer} multistream
+ * @param {string} protocol
+ * @returns {Promise} Resolves the selected Connection
+ */
+module.exports.msSelect = (multistream, protocol) => {
+  return new Promise((resolve, reject) => {
+    multistream.select(protocol, (err, connection) => {
+      if (err) return reject(err)
+      resolve(connection)
+    })
+  })
+}
+
+/**
+ * Runs identify for the given connection and verifies it against the
+ * PeerInfo provided
+ * @param {Connection} connection
+ * @param {PeerInfo} cryptoPeerInfo The PeerInfo determined during crypto exchange
+ * @returns {Promise} Resolves {peerInfo, observedAddrs}
+ */
+module.exports.identifyDialer = (connection, cryptoPeerInfo) => {
+  return new Promise((resolve, reject) => {
+    Identify.dialer(connection, cryptoPeerInfo, (err, peerInfo, observedAddrs) => {
+      if (err) return reject(err)
+      resolve({ peerInfo, observedAddrs })
+    })
+  })
+}

test/dial-fsm.node.js

@@ -125,7 +125,9 @@ describe('dialFSM', () => {
       }
     })
 
-    const connFSM = switchA.dialFSM(switchB._peerInfo, '/closed/1.0.0', () => { })
+    const connFSM = switchA.dialFSM(switchB._peerInfo, '/closed/1.0.0', (err) => {
+      expect(err).to.not.exist()
+    })
     connFSM.once('close', () => {
       expect(switchA.connection.getAllById(switchB._peerInfo.id.toB58String())).to.have.length(0).mark()
     })