fix: start and stop error callback (#316)

* fix: ensure start and stop callbacks are called

Author: jacobheun

package.json

@@ -54,6 +54,7 @@
     "libp2p-websockets": "~0.12.0",
     "mafmt": "^6.0.2",
     "multiaddr": "^6.0.2",
+    "once": "^1.4.0",
     "peer-book": "~0.9.0",
     "peer-id": "~0.12.0",
     "peer-info": "~0.15.0"

src/index.js

@@ -17,6 +17,7 @@ const Ping = require('libp2p-ping')
 const WebSockets = require('libp2p-websockets')
 const ConnectionManager = require('libp2p-connection-manager')
 
+const { emitFirst } = require('./util')
 const peerRouting = require('./peer-routing')
 const contentRouting = require('./content-routing')
 const dht = require('./dht')
@@ -194,7 +195,7 @@ class Node extends EventEmitter {
    * @returns {void}
    */
   start (callback = () => {}) {
-    this.once('start', callback)
+    emitFirst(this, ['error', 'start'], callback)
     this.state('start')
   }
 
@@ -205,7 +206,7 @@ class Node extends EventEmitter {
    * @returns {void}
    */
   stop (callback = () => {}) {
-    this.once('stop', callback)
+    emitFirst(this, ['error', 'stop'], callback)
     this.state('stop')
   }
 
@@ -473,8 +474,9 @@ class Node extends EventEmitter {
         this._switch.stop(cb)
       },
       (cb) => {
-        // Ensures idempotent restarts
-        this._switch.transport.removeAll(cb)
+        // Ensures idempotent restarts, ignore any errors
+        // from removeAll, they're not useful at this point
+        this._switch.transport.removeAll(() => cb())
       }
     ], (err) => {
       if (err) {

src/util/index.js

@@ -0,0 +1,33 @@
+'use strict'
+const once = require('once')
+
+/**
+ * Registers `handler` to each event in `events`. The `handler`
+ * will only be called for the first event fired, at which point
+ * the `handler` will be removed as a listener.
+ *
+ * Ensures `handler` is only called once.
+ *
+ * @example
+ * // will call `callback` when `start` or `error` is emitted by `this`
+ * emitFirst(this, ['error', 'start'], callback)
+ *
+ * @private
+ * @param {EventEmitter} emitter The emitter to listen on
+ * @param {Array<string>} events The events to listen for
+ * @param {function(*)} handler The handler to call when an event is triggered
+ * @returns {void}
+ */
+function emitFirst (emitter, events, handler) {
+  handler = once(handler)
+  events.forEach((e) => {
+    emitter.once(e, (...args) => {
+      events.forEach((ev) => {
+        emitter.removeListener(ev, handler)
+      })
+      handler.apply(emitter, args)
+    })
+  })
+}
+
+module.exports.emitFirst = emitFirst

test/fsm.spec.js

@@ -26,6 +26,7 @@ describe('libp2p state machine (fsm)', () => {
     })
     afterEach(() => {
       node.removeAllListeners()
+      sinon.restore()
     })
     after((done) => {
       node.stop(done)
@@ -64,6 +65,23 @@ describe('libp2p state machine (fsm)', () => {
       node.start()
     })
 
+    it('should callback with an error when it occurs on stop', (done) => {
+      const error = new Error('some error starting')
+      node.once('start', () => {
+        node.once('error', (err) => {
+          expect(err).to.eql(error).mark()
+        })
+        node.stop((err) => {
+          expect(err).to.eql(error).mark()
+        })
+      })
+
+      expect(2).checks(done)
+
+      sinon.stub(node._switch, 'stop').callsArgWith(0, error)
+      node.start()
+    })
+
     it('should noop when starting a started node', (done) => {
       node.once('start', () => {
         node.state.on('STARTING', () => {
@@ -116,9 +134,11 @@ describe('libp2p state machine (fsm)', () => {
         throw new Error('should not start')
       })
 
-      expect(2).checks(done)
+      expect(3).checks(done)
 
-      node.start()
+      node.start((err) => {
+        expect(err).to.eql(error).mark()
+      })
     })
 
     it('should not dial when the node is stopped', (done) => {