From b42f25e4249716c2674f1c98c3d49f6e93a1a79c Mon Sep 17 00:00:00 2001 From: Jacob Heun Date: Mon, 21 Jan 2019 17:08:49 +0100 Subject: [PATCH 1/2] fix: ensure start and stop callbacks are called --- package.json | 1 + src/index.js | 32 +++++++++++++++++++++++++++++++- test/fsm.spec.js | 24 ++++++++++++++++++++++-- 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 57411384f3..e374cf8125 100644 --- a/package.json +++ b/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" diff --git a/src/index.js b/src/index.js index 047a190302..b955b98171 100644 --- a/src/index.js +++ b/src/index.js @@ -6,6 +6,7 @@ const debug = require('debug') const log = debug('libp2p') log.error = debug('libp2p:error') const errCode = require('err-code') +const once = require('once') const each = require('async/each') const series = require('async/series') @@ -31,6 +32,35 @@ const notStarted = (action, state) => { ) } +/** + * 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` + * eitherOnce(this, ['error', 'start'], callback) + * + * @private + * @param {EventEmitter} emitter The emitter to listen on + * @param {Array} events The events to listen for + * @param {function(*)} handler The handler to call when an event is triggered + * @returns {void} + */ +function eitherOnce (emitter, events, handler) { + handler = once(handler) + events.forEach((e) => { + emitter.once(e, (...args) => { + events.forEach((ev) => { + emitter.removeListener(ev, handler) + }) + handler.apply(emitter, args) + }) + }) +} + /** * @fires Node#error Emitted when an error occurs * @fires Node#peer:connect Emitted when a peer is connected to this node @@ -194,7 +224,7 @@ class Node extends EventEmitter { * @returns {void} */ start (callback = () => {}) { - this.once('start', callback) + eitherOnce(this, ['error', 'start'], callback) this.state('start') } diff --git a/test/fsm.spec.js b/test/fsm.spec.js index 9279c1313d..8acb1dc6f2 100644 --- a/test/fsm.spec.js +++ b/test/fsm.spec.js @@ -20,6 +20,7 @@ describe('libp2p state machine (fsm)', () => { }) afterEach(() => { node.removeAllListeners() + sinon.restore() }) after((done) => { node.stop(done) @@ -58,6 +59,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.not.exist().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', () => { @@ -110,9 +128,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) => { From 426c222d00fdc7537557b8d251293016e27a3e6d Mon Sep 17 00:00:00 2001 From: Jacob Heun Date: Fri, 1 Feb 2019 15:20:34 +0100 Subject: [PATCH 2/2] fix: improve stop callback --- src/index.js | 40 ++++++---------------------------------- src/util/index.js | 33 +++++++++++++++++++++++++++++++++ test/fsm.spec.js | 2 +- 3 files changed, 40 insertions(+), 35 deletions(-) create mode 100644 src/util/index.js diff --git a/src/index.js b/src/index.js index b955b98171..60917a2bdb 100644 --- a/src/index.js +++ b/src/index.js @@ -6,7 +6,6 @@ const debug = require('debug') const log = debug('libp2p') log.error = debug('libp2p:error') const errCode = require('err-code') -const once = require('once') const each = require('async/each') const series = require('async/series') @@ -18,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') @@ -32,35 +32,6 @@ const notStarted = (action, state) => { ) } -/** - * 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` - * eitherOnce(this, ['error', 'start'], callback) - * - * @private - * @param {EventEmitter} emitter The emitter to listen on - * @param {Array} events The events to listen for - * @param {function(*)} handler The handler to call when an event is triggered - * @returns {void} - */ -function eitherOnce (emitter, events, handler) { - handler = once(handler) - events.forEach((e) => { - emitter.once(e, (...args) => { - events.forEach((ev) => { - emitter.removeListener(ev, handler) - }) - handler.apply(emitter, args) - }) - }) -} - /** * @fires Node#error Emitted when an error occurs * @fires Node#peer:connect Emitted when a peer is connected to this node @@ -224,7 +195,7 @@ class Node extends EventEmitter { * @returns {void} */ start (callback = () => {}) { - eitherOnce(this, ['error', 'start'], callback) + emitFirst(this, ['error', 'start'], callback) this.state('start') } @@ -235,7 +206,7 @@ class Node extends EventEmitter { * @returns {void} */ stop (callback = () => {}) { - this.once('stop', callback) + emitFirst(this, ['error', 'stop'], callback) this.state('stop') } @@ -503,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) { diff --git a/src/util/index.js b/src/util/index.js new file mode 100644 index 0000000000..bfee1875a7 --- /dev/null +++ b/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} 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 diff --git a/test/fsm.spec.js b/test/fsm.spec.js index 8acb1dc6f2..0d570a7b62 100644 --- a/test/fsm.spec.js +++ b/test/fsm.spec.js @@ -66,7 +66,7 @@ describe('libp2p state machine (fsm)', () => { expect(err).to.eql(error).mark() }) node.stop((err) => { - expect(err).to.not.exist().mark() + expect(err).to.eql(error).mark() }) })