fix: wrong handling of event listeners in MulticastDNS start/stop methods

[ghost]

First, the methods start and stop of class MulticastDNS specify different handler functions for adding an event listener in start and removing this listener in stop. In effect, the added listeners are never removed, as (event) => this._onMdnsQuery(event) and this._onMdnsQuery are two completely different functions that are not reference-equal!

Second, the second invocation of removeListener in the stop method (line 88) should target the response event, not the query event.

This is how the buggy code looks like (with console.log annotations on listener count). The listener count is still 1 after stop method has run.

async start () {
    if (this.mdns) return

    this.mdns = multicastDNS({ port: this.port })
    this.mdns.on('query', (event) => this._onMdnsQuery(event))
    console.log("*** MDNS START LISTENER COUNT query", this.mdns.listenerCount('query'));
    this.mdns.on('response', (event) => this._onMdnsResponse(event))

    this._queryInterval = query.queryLAN(this.mdns, this.serviceTag, this.interval)

    if (this._goMdns) {
      await this._goMdns.start()
    }
}

async stop () {
    if (!this.mdns) {
      return
    }

    this.mdns.removeListener('query', this._onMdnsQuery)
    console.log("*** MDNS STOP LISTENER COUNT query", this.mdns.listenerCount('query'));
    this.mdns.removeListener('query', this._onMdnsResponse)
    this._goMdns && this._goMdns.removeListener('peer', this._onPeer)

    clearInterval(this._queryInterval)
    this._queryInterval = null

    await Promise.all([
      this._goMdns && this._goMdns.stop(),
      new Promise((resolve) => this.mdns.destroy(resolve))
    ])

    this.mdns = undefined
  }
}

This is the correct way to fix both bugs:

constructor (options = {}) {
    super()
    ....
    this._onPeer = this._onPeer.bind(this)
    this._onMdnsQuery = this._onMdnsQuery.bind(this)
    this._onMdnsResponse = this._onMdnsResponse.bind(this)
    ....
}

async start () {
    if (this.mdns) return

    this.mdns = multicastDNS({ port: this.port })
    this.mdns.on('query', this._onMdnsQuery)
    this.mdns.on('response', this._onMdnsResponse)

    this._queryInterval = query.queryLAN(this.mdns, this.serviceTag, this.interval)

    if (this._goMdns) {
      await this._goMdns.start()
    }
}

async stop () {
    if (!this.mdns) {
      return
    }

    this.mdns.removeListener('query', this._onMdnsQuery)
    this.mdns.removeListener('response', this._onMdnsResponse)
    this._goMdns && this._goMdns.removeListener('peer', this._onPeer)

    clearInterval(this._queryInterval)
    this._queryInterval = null

    await Promise.all([
      this._goMdns && this._goMdns.stop(),
      new Promise((resolve) => this.mdns.destroy(resolve))
    ])

    this.mdns = undefined
  }
}

Thanks for providing a patch release! Hubertus

[jacobheun]

If you'd like to submit a PR that would be appreciated!

[ghost]

PR is here