From 6045f0afc01ed3c060c630f83a4e6e740b8d7ac4 Mon Sep 17 00:00:00 2001 From: achingbrain Date: Fri, 26 Mar 2021 14:51:30 +0000 Subject: [PATCH 1/3] fix: the API of es6-promisify is not the same as promisify-es6 Fixes a regression introduced by #896 `promisify-es6` was swapped for `es6-promisify` but the APIs of the two modules are no the same so the NAT hole punching code broke. Also changes port numbers to 0 in the tests as other processes can be listening on those ports which causes spurious test failures. --- package.json | 1 + src/nat-manager.js | 8 ++++---- test/nat-manager/nat-manager.node.js | 8 ++++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/package.json b/package.json index 29f0caf748..806e8b9e18 100644 --- a/package.json +++ b/package.json @@ -109,6 +109,7 @@ }, "devDependencies": { "@nodeutils/defaults-deep": "^1.1.0", + "@types/es6-promisify": "^6.0.0", "abortable-iterator": "^3.0.0", "aegir": "^29.2.0", "chai-bytes": "^0.1.2", diff --git a/src/nat-manager.js b/src/nat-manager.js index 121139ab7e..be9687e19b 100644 --- a/src/nat-manager.js +++ b/src/nat-manager.js @@ -2,7 +2,7 @@ const NatAPI = require('@motrix/nat-api') const debug = require('debug') -const promisify = require('es6-promisify') +const { promisify } = require('es6-promisify') const Multiaddr = require('multiaddr') const log = Object.assign(debug('libp2p:nat'), { error: debug('libp2p:nat:err') @@ -132,9 +132,9 @@ class NatManager { } const client = new NatAPI(this._options) - const map = promisify(client.map, { context: client }) - const destroy = promisify(client.destroy, { context: client }) - const externalIp = promisify(client.externalIp, { context: client }) + const map = promisify(client.map.bind(client)) + const destroy = promisify(client.destroy.bind(client)) + const externalIp = promisify(client.externalIp.bind(client)) this._client = { // these are all network operations so add a retry diff --git a/test/nat-manager/nat-manager.node.js b/test/nat-manager/nat-manager.node.js index 9bf922e505..efe9a2de4d 100644 --- a/test/nat-manager/nat-manager.node.js +++ b/test/nat-manager/nat-manager.node.js @@ -156,7 +156,7 @@ describe('Nat Manager (TCP)', () => { natManager, addressManager } = await createNatManager([ - '/ip6/::/tcp/5001' + '/ip6/::/tcp/0' ]) let observed = addressManager.getObservedAddrs().map(ma => ma.toString()) @@ -173,7 +173,7 @@ describe('Nat Manager (TCP)', () => { natManager, addressManager } = await createNatManager([ - '/ip6/::1/tcp/5001' + '/ip6/::1/tcp/0' ]) let observed = addressManager.getObservedAddrs().map(ma => ma.toString()) @@ -207,7 +207,7 @@ describe('Nat Manager (TCP)', () => { natManager, addressManager } = await createNatManager([ - '/ip4/127.0.0.1/tcp/5900' + '/ip4/127.0.0.1/tcp/0' ]) let observed = addressManager.getObservedAddrs().map(ma => ma.toString()) @@ -224,7 +224,7 @@ describe('Nat Manager (TCP)', () => { natManager, addressManager } = await createNatManager([ - '/ip4/0.0.0.0/tcp/5900/sctp/49832' + '/ip4/0.0.0.0/tcp/0/sctp/0' ]) let observed = addressManager.getObservedAddrs().map(ma => ma.toString()) From b751e19ab60861ff537d7d696638be6e5eec3caf Mon Sep 17 00:00:00 2001 From: achingbrain Date: Fri, 26 Mar 2021 15:04:24 +0000 Subject: [PATCH 2/3] chore: fix types --- src/nat-manager.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/nat-manager.js b/src/nat-manager.js index be9687e19b..887f3d81fb 100644 --- a/src/nat-manager.js +++ b/src/nat-manager.js @@ -132,14 +132,32 @@ class NatManager { } const client = new NatAPI(this._options) + + /** @type {(...any) => any} */ const map = promisify(client.map.bind(client)) + /** @type {(...any) => any} */ const destroy = promisify(client.destroy.bind(client)) + /** @type {(...any) => any} */ const externalIp = promisify(client.externalIp.bind(client)) + // these are all network operations so add a retry this._client = { - // these are all network operations so add a retry + /** + * @param {...any} args + * @returns {Promise} + */ map: (...args) => retry(() => map(...args), { onFailedAttempt: log.error, unref: true }), + + /** + * @param {...any} args + * @returns {Promise} + */ destroy: (...args) => retry(() => destroy(...args), { onFailedAttempt: log.error, unref: true }), + + /** + * @param {...any} args + * @returns {Promise} + */ externalIp: (...args) => retry(() => externalIp(...args), { onFailedAttempt: log.error, unref: true }) } From 641a9cbc432217ea51d55f62a0b6d5646a233bd6 Mon Sep 17 00:00:00 2001 From: achingbrain Date: Fri, 26 Mar 2021 15:55:10 +0000 Subject: [PATCH 3/3] chore: add test for client creation --- test/nat-manager/nat-manager.node.js | 44 ++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/nat-manager/nat-manager.node.js b/test/nat-manager/nat-manager.node.js index efe9a2de4d..557f266a1c 100644 --- a/test/nat-manager/nat-manager.node.js +++ b/test/nat-manager/nat-manager.node.js @@ -3,6 +3,7 @@ const { expect } = require('aegir/utils/chai') const sinon = require('sinon') +const { networkInterfaces } = require('os') const AddressManager = require('../../src/address-manager') const TransportManager = require('../../src/transport-manager') const Transport = require('libp2p-tcp') @@ -241,4 +242,47 @@ describe('Nat Manager (TCP)', () => { new NatManager({ ttl: 5 }) // eslint-disable-line no-new }).to.throw().with.property('code', ERR_INVALID_PARAMETERS) }) + + it('shuts the nat api down when stopping', async function () { + function findRoutableAddress () { + const interfaces = networkInterfaces() + + for (const name of Object.keys(interfaces)) { + for (const iface of interfaces[name]) { + // Skip over non-IPv4 and internal (i.e. 127.0.0.1) addresses + if (iface.family === 'IPv4' && !iface.internal) { + return iface.address + } + } + } + } + + const addr = findRoutableAddress() + + if (!addr) { + // skip test if no non-loopback address is found + this.skip() + } + + const { + natManager + } = await createNatManager([ + `/ip4/${addr}/tcp/0` + ]) + + // use the actual nat manager client not the stub + delete natManager._client + + await natManager._start() + + const client = natManager._client + expect(client).to.be.ok() + + // ensure the client was stopped + const spy = sinon.spy(client, 'destroy') + + await natManager.stop() + + expect(spy.called).to.be.true() + }) })