From c347bf2f35e6462e1e4b1ca6e9a8a46b7981139b Mon Sep 17 00:00:00 2001 From: Alex Potsides Date: Wed, 25 Oct 2023 06:26:42 +0100 Subject: [PATCH] refactor!: remove isStarted method from Startable (#2145) We have an `isStarted` method on the `Startable` interface but we only really use it in tests. Our implementations tend to guard on being started twice so it just adds noise to every implementation. BREAKING CHANGE: the `isStarted` method has been removed from the `Startable` interface --- .../src/pubsub/api.ts | 2 - packages/interface/src/startable.ts | 2 - packages/libp2p/test/core/start.spec.ts | 47 ----- packages/libp2p/test/identify/service.spec.ts | 2 - .../test/transports/transport-manager.spec.ts | 3 - packages/peer-discovery-mdns/src/index.ts | 183 +----------------- packages/peer-discovery-mdns/src/mdns.ts | 158 +++++++++++++++ .../test/compliance.spec.ts | 14 +- .../test/multicast-dns.spec.ts | 3 +- 9 files changed, 169 insertions(+), 245 deletions(-) delete mode 100644 packages/libp2p/test/core/start.spec.ts create mode 100644 packages/peer-discovery-mdns/src/mdns.ts diff --git a/packages/interface-compliance-tests/src/pubsub/api.ts b/packages/interface-compliance-tests/src/pubsub/api.ts index a879c46473..4c81f4f595 100644 --- a/packages/interface-compliance-tests/src/pubsub/api.ts +++ b/packages/interface-compliance-tests/src/pubsub/api.ts @@ -48,7 +48,6 @@ export default (common: TestSetup): void => { await start(...Object.values(components)) - expect(pubsub.isStarted()).to.equal(true) expect(components.registrar.register).to.have.property('callCount', 1) }) @@ -62,7 +61,6 @@ export default (common: TestSetup): void => { await start(...Object.values(components)) await stop(...Object.values(components)) - expect(pubsub.isStarted()).to.equal(false) expect(components.registrar.unregister).to.have.property('callCount', 1) }) diff --git a/packages/interface/src/startable.ts b/packages/interface/src/startable.ts index 8393abc489..7923b1d557 100644 --- a/packages/interface/src/startable.ts +++ b/packages/interface/src/startable.ts @@ -2,8 +2,6 @@ * Implemented by components that have a lifecycle */ export interface Startable { - isStarted(): boolean - /** * If implemented, this method will be invoked before the start method. * diff --git a/packages/libp2p/test/core/start.spec.ts b/packages/libp2p/test/core/start.spec.ts deleted file mode 100644 index a8c5b44855..0000000000 --- a/packages/libp2p/test/core/start.spec.ts +++ /dev/null @@ -1,47 +0,0 @@ -/* eslint-env mocha */ - -import { webSockets } from '@libp2p/websockets' -import { expect } from 'aegir/chai' -import { createLibp2p, type Libp2p } from '../../src/index.js' -import { plaintext } from '../../src/insecure/index.js' - -describe('start', () => { - let libp2p: Libp2p - - afterEach(async () => { - if (libp2p != null) { - await libp2p.stop() - } - }) - - it('it should start by default', async () => { - libp2p = await createLibp2p({ - transports: [ - webSockets() - ], - connectionEncryption: [ - plaintext() - ] - }) - - expect(libp2p.isStarted()).to.be.true() - }) - - it('it should allow overriding', async () => { - libp2p = await createLibp2p({ - start: false, - transports: [ - webSockets() - ], - connectionEncryption: [ - plaintext() - ] - }) - - expect(libp2p.isStarted()).to.be.false() - - await libp2p.start() - - expect(libp2p.isStarted()).to.be.true() - }) -}) diff --git a/packages/libp2p/test/identify/service.spec.ts b/packages/libp2p/test/identify/service.spec.ts index 64e7303e2d..5181b8cdfc 100644 --- a/packages/libp2p/test/identify/service.spec.ts +++ b/packages/libp2p/test/identify/service.spec.ts @@ -153,7 +153,6 @@ describe('identify', () => { // Wait for identify to finish await identityServiceIdentifySpy.firstCall.returnValue - sinon.stub(libp2p, 'isStarted').returns(true) // Cause supported protocols to change await libp2p.handle('/echo/2.0.0', () => {}) @@ -251,7 +250,6 @@ describe('identify', () => { // Wait for identify to finish await identityServiceIdentifySpy.firstCall.returnValue - sinon.stub(libp2p, 'isStarted').returns(true) await libp2p.peerStore.merge(libp2p.peerId, { multiaddrs: [multiaddr('/ip4/180.0.0.1/tcp/15001/ws')] diff --git a/packages/libp2p/test/transports/transport-manager.spec.ts b/packages/libp2p/test/transports/transport-manager.spec.ts index c71a124a92..075364aea9 100644 --- a/packages/libp2p/test/transports/transport-manager.spec.ts +++ b/packages/libp2p/test/transports/transport-manager.spec.ts @@ -124,7 +124,6 @@ describe('libp2p.transportManager (dial only)', () => { start: false }) - expect(libp2p.isStarted()).to.be.false() await expect(libp2p.start()).to.eventually.be.rejected .with.property('code', ErrorCodes.ERR_NO_VALID_ADDRESSES) }) @@ -147,7 +146,6 @@ describe('libp2p.transportManager (dial only)', () => { start: false }) - expect(libp2p.isStarted()).to.be.false() await expect(libp2p.start()).to.eventually.be.undefined() }) @@ -169,7 +167,6 @@ describe('libp2p.transportManager (dial only)', () => { start: false }) - expect(libp2p.isStarted()).to.be.false() await expect(libp2p.start()).to.eventually.be.undefined() }) }) diff --git a/packages/peer-discovery-mdns/src/index.ts b/packages/peer-discovery-mdns/src/index.ts index ed37439b57..6f78e6f7e2 100644 --- a/packages/peer-discovery-mdns/src/index.ts +++ b/packages/peer-discovery-mdns/src/index.ts @@ -76,186 +76,9 @@ * ``` */ -import { CustomEvent, TypedEventEmitter } from '@libp2p/interface/events' -import { peerDiscovery } from '@libp2p/interface/peer-discovery' -import { logger } from '@libp2p/logger' -import multicastDNS from 'multicast-dns' -import * as query from './query.js' -import { stringGen } from './utils.js' -import type { PeerDiscovery, PeerDiscoveryEvents } from '@libp2p/interface/peer-discovery' -import type { PeerInfo } from '@libp2p/interface/peer-info' -import type { Startable } from '@libp2p/interface/src/startable.js' -import type { AddressManager } from '@libp2p/interface-internal/address-manager' - -const log = logger('libp2p:mdns') - -export interface MulticastDNSInit { - /** - * (true/false) announce our presence through mDNS, default `true` - */ - broadcast?: boolean - - /** - * query interval, default 10 \* 1000 (10 seconds) - */ - interval?: number - - /** - * name of the service announce , default '_p2p._udp.local\` - */ - serviceTag?: string - /** - * Peer name to announce (should not be peeer id), default random string - */ - peerName?: string - - /** - * UDP port to broadcast to - */ - port?: number - - /** - * UDP IP to broadcast to - */ - ip?: string -} - -export interface MulticastDNSComponents { - addressManager: AddressManager -} - -class MulticastDNS extends TypedEventEmitter implements PeerDiscovery, Startable { - public mdns?: multicastDNS.MulticastDNS - - private readonly broadcast: boolean - private readonly interval: number - private readonly serviceTag: string - private readonly peerName: string - private readonly port: number - private readonly ip: string - private _queryInterval: ReturnType | null - private readonly components: MulticastDNSComponents - - constructor (components: MulticastDNSComponents, init: MulticastDNSInit = {}) { - super() - - this.broadcast = init.broadcast !== false - this.interval = init.interval ?? (1e3 * 10) - this.serviceTag = init.serviceTag ?? '_p2p._udp.local' - this.ip = init.ip ?? '224.0.0.251' - this.peerName = init.peerName ?? stringGen(63) - // 63 is dns label limit - if (this.peerName.length >= 64) { - throw new Error('Peer name should be less than 64 chars long') - } - this.port = init.port ?? 5353 - this.components = components - this._queryInterval = null - this._onMdnsQuery = this._onMdnsQuery.bind(this) - this._onMdnsResponse = this._onMdnsResponse.bind(this) - this._onMdnsWarning = this._onMdnsWarning.bind(this) - this._onMdnsError = this._onMdnsError.bind(this) - } - - readonly [peerDiscovery] = this - - readonly [Symbol.toStringTag] = '@libp2p/mdns' - - isStarted (): boolean { - return Boolean(this.mdns) - } - - /** - * Start sending queries to the LAN. - * - * @returns {void} - */ - async start (): Promise { - if (this.mdns != null) { - return - } - - this.mdns = multicastDNS({ port: this.port, ip: this.ip }) - this.mdns.on('query', this._onMdnsQuery) - this.mdns.on('response', this._onMdnsResponse) - this.mdns.on('warning', this._onMdnsWarning) - this.mdns.on('error', this._onMdnsError) - - this._queryInterval = query.queryLAN(this.mdns, this.serviceTag, this.interval) - } - - _onMdnsQuery (event: multicastDNS.QueryPacket): void { - if (this.mdns == null) { - return - } - - log.trace('received incoming mDNS query') - query.gotQuery( - event, - this.mdns, - this.peerName, - this.components.addressManager.getAddresses(), - this.serviceTag, - this.broadcast) - } - - _onMdnsResponse (event: multicastDNS.ResponsePacket): void { - log.trace('received mDNS query response') - - try { - const foundPeer = query.gotResponse(event, this.peerName, this.serviceTag) - - if (foundPeer != null) { - log('discovered peer in mDNS query response %p', foundPeer.id) - - this.dispatchEvent(new CustomEvent('peer', { - detail: foundPeer - })) - } - } catch (err) { - log.error('Error processing peer response', err) - } - } - - _onMdnsWarning (err: Error): void { - log.error('mdns warning', err) - } - - _onMdnsError (err: Error): void { - log.error('mdns error', err) - } - - /** - * Stop sending queries to the LAN. - * - * @returns {Promise} - */ - async stop (): Promise { - if (this.mdns == null) { - return - } - - this.mdns.removeListener('query', this._onMdnsQuery) - this.mdns.removeListener('response', this._onMdnsResponse) - this.mdns.removeListener('warning', this._onMdnsWarning) - this.mdns.removeListener('error', this._onMdnsError) - - if (this._queryInterval != null) { - clearInterval(this._queryInterval) - this._queryInterval = null - } - - await new Promise((resolve) => { - if (this.mdns != null) { - this.mdns.destroy(resolve) - } else { - resolve() - } - }) - - this.mdns = undefined - } -} +import { MulticastDNS } from './mdns.js' +import type { MulticastDNSInit, MulticastDNSComponents } from './mdns.js' +import type { PeerDiscovery } from '@libp2p/interface/peer-discovery' export function mdns (init: MulticastDNSInit = {}): (components: MulticastDNSComponents) => PeerDiscovery { return (components: MulticastDNSComponents) => new MulticastDNS(components, init) diff --git a/packages/peer-discovery-mdns/src/mdns.ts b/packages/peer-discovery-mdns/src/mdns.ts new file mode 100644 index 0000000000..cdcd802a46 --- /dev/null +++ b/packages/peer-discovery-mdns/src/mdns.ts @@ -0,0 +1,158 @@ +import { CustomEvent, EventEmitter } from '@libp2p/interface/events' +import { peerDiscovery } from '@libp2p/interface/peer-discovery' +import { logger } from '@libp2p/logger' +import multicastDNS from 'multicast-dns' +import * as query from './query.js' +import { stringGen } from './utils.js' +import type { PeerDiscovery, PeerDiscoveryEvents } from '@libp2p/interface/peer-discovery' +import type { PeerInfo } from '@libp2p/interface/peer-info' +import type { Startable } from '@libp2p/interface/src/startable.js' +import type { AddressManager } from '@libp2p/interface-internal/address-manager' + +const log = logger('libp2p:mdns') + +export interface MulticastDNSInit { + broadcast?: boolean + interval?: number + serviceTag?: string + peerName?: string + port?: number + ip?: string +} + +export interface MulticastDNSComponents { + addressManager: AddressManager +} + +export class MulticastDNS extends EventEmitter implements PeerDiscovery, Startable { + public mdns?: multicastDNS.MulticastDNS + + private readonly broadcast: boolean + private readonly interval: number + private readonly serviceTag: string + private readonly peerName: string + private readonly port: number + private readonly ip: string + private _queryInterval: ReturnType | null + private readonly components: MulticastDNSComponents + + constructor (components: MulticastDNSComponents, init: MulticastDNSInit = {}) { + super() + + this.broadcast = init.broadcast !== false + this.interval = init.interval ?? (1e3 * 10) + this.serviceTag = init.serviceTag ?? '_p2p._udp.local' + this.ip = init.ip ?? '224.0.0.251' + this.peerName = init.peerName ?? stringGen(63) + // 63 is dns label limit + if (this.peerName.length >= 64) { + throw new Error('Peer name should be less than 64 chars long') + } + this.port = init.port ?? 5353 + this.components = components + this._queryInterval = null + this._onMdnsQuery = this._onMdnsQuery.bind(this) + this._onMdnsResponse = this._onMdnsResponse.bind(this) + this._onMdnsWarning = this._onMdnsWarning.bind(this) + this._onMdnsError = this._onMdnsError.bind(this) + } + + readonly [peerDiscovery] = this + + readonly [Symbol.toStringTag] = '@libp2p/mdns' + + isStarted (): boolean { + return Boolean(this.mdns) + } + + /** + * Start sending queries to the LAN. + * + * @returns {void} + */ + async start (): Promise { + if (this.mdns != null) { + return + } + + this.mdns = multicastDNS({ port: this.port, ip: this.ip }) + this.mdns.on('query', this._onMdnsQuery) + this.mdns.on('response', this._onMdnsResponse) + this.mdns.on('warning', this._onMdnsWarning) + this.mdns.on('error', this._onMdnsError) + + this._queryInterval = query.queryLAN(this.mdns, this.serviceTag, this.interval) + } + + _onMdnsQuery (event: multicastDNS.QueryPacket): void { + if (this.mdns == null) { + return + } + + log.trace('received incoming mDNS query') + query.gotQuery( + event, + this.mdns, + this.peerName, + this.components.addressManager.getAddresses(), + this.serviceTag, + this.broadcast) + } + + _onMdnsResponse (event: multicastDNS.ResponsePacket): void { + log.trace('received mDNS query response') + + try { + const foundPeer = query.gotResponse(event, this.peerName, this.serviceTag) + + if (foundPeer != null) { + log('discovered peer in mDNS query response %p', foundPeer.id) + + this.dispatchEvent(new CustomEvent('peer', { + detail: foundPeer + })) + } + } catch (err) { + log.error('Error processing peer response', err) + } + } + + _onMdnsWarning (err: Error): void { + log.error('mdns warning', err) + } + + _onMdnsError (err: Error): void { + log.error('mdns error', err) + } + + /** + * Stop sending queries to the LAN. + * + * @returns {Promise} + */ + async stop (): Promise { + if (this.mdns == null) { + return + } + + this.mdns.removeListener('query', this._onMdnsQuery) + this.mdns.removeListener('response', this._onMdnsResponse) + this.mdns.removeListener('warning', this._onMdnsWarning) + this.mdns.removeListener('error', this._onMdnsError) + + if (this._queryInterval != null) { + clearInterval(this._queryInterval) + this._queryInterval = null + } + + await new Promise((resolve) => { + if (this.mdns != null) { + this.mdns.destroy(resolve) + } else { + resolve() + } + }) + + this.mdns = undefined + } +} diff --git a/packages/peer-discovery-mdns/test/compliance.spec.ts b/packages/peer-discovery-mdns/test/compliance.spec.ts index b24d590d91..d5975b654c 100644 --- a/packages/peer-discovery-mdns/test/compliance.spec.ts +++ b/packages/peer-discovery-mdns/test/compliance.spec.ts @@ -1,16 +1,14 @@ /* eslint-env mocha */ import { CustomEvent } from '@libp2p/interface/events' -import { isStartable } from '@libp2p/interface/startable' import tests from '@libp2p/interface-compliance-tests/peer-discovery' import { createEd25519PeerId } from '@libp2p/peer-id-factory' import { multiaddr } from '@multiformats/multiaddr' import { stubInterface } from 'ts-sinon' -import { mdns } from '../src/index.js' -import type { PeerDiscovery } from '@libp2p/interface/peer-discovery' +import { MulticastDNS } from '../src/mdns.js' import type { AddressManager } from '@libp2p/interface-internal/address-manager' -let discovery: PeerDiscovery +let discovery: MulticastDNS describe('compliance tests', () => { let intervalId: ReturnType @@ -25,18 +23,18 @@ describe('compliance tests', () => { multiaddr(`/ip4/127.0.0.1/tcp/13921/p2p/${peerId1.toString()}`) ]) - discovery = mdns({ + discovery = new MulticastDNS({ + addressManager + }, { broadcast: false, port: 50001 - })({ - addressManager }) // Trigger discovery const maStr = '/ip4/127.0.0.1/tcp/15555/ws/p2p-webrtc-star' intervalId = setInterval(() => { - if (isStartable(discovery) && !discovery.isStarted()) { + if (!discovery.isStarted()) { return } diff --git a/packages/peer-discovery-mdns/test/multicast-dns.spec.ts b/packages/peer-discovery-mdns/test/multicast-dns.spec.ts index 9955002dd7..1ac9b75056 100644 --- a/packages/peer-discovery-mdns/test/multicast-dns.spec.ts +++ b/packages/peer-discovery-mdns/test/multicast-dns.spec.ts @@ -6,7 +6,8 @@ import { multiaddr } from '@multiformats/multiaddr' import { expect } from 'aegir/chai' import pWaitFor from 'p-wait-for' import { stubInterface } from 'ts-sinon' -import { mdns, type MulticastDNSComponents } from './../src/index.js' +import { mdns } from './../src/index.js' +import type { MulticastDNSComponents } from './../src/mdns.js' import type { PeerId } from '@libp2p/interface/peer-id' import type { PeerInfo } from '@libp2p/interface/peer-info' import type { AddressManager } from '@libp2p/interface-internal/address-manager'