refactor(discovery-client): add new mDNS browser wrapper (#6154)

discovery-client/src/__tests__/mdns-browser.test.js

@@ -0,0 +1,168 @@
+// @flow
+import EventEmitter from 'events'
+import Mdns from 'mdns-js'
+
+import { MOCK_BROWSER_SERVICE } from '../__fixtures__/mdns-browser-service'
+import { createMdnsBrowser } from '../mdns-browser'
+
+import type { MdnsServiceType, Browser } from 'mdns-js'
+
+jest.mock('mdns-js', () => ({
+  tcp: (name: string) => ({
+    name,
+    protocol: 'tcp',
+    subtypes: [],
+    description: '',
+  }),
+  createBrowser: jest.fn(),
+  ServiceType: function() {},
+}))
+
+const createBrowser: JestMockFn<[MdnsServiceType], Browser> = Mdns.createBrowser
+
+const baseBrowser: Browser = (Object.assign(
+  new EventEmitter(),
+  ({ discover: jest.fn(), stop: jest.fn() }: any)
+): any)
+
+describe('mdns browser', () => {
+  const onService = jest.fn()
+
+  beforeEach(() => {
+    createBrowser.mockReturnValue(baseBrowser)
+  })
+
+  afterEach(() => {
+    jest.resetAllMocks()
+    baseBrowser.removeAllListeners()
+  })
+
+  it('creates mdns browser that searches for http', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+
+    expect(createBrowser).toHaveBeenCalledWith(Mdns.tcp('http'))
+    expect(baseBrowser.discover).toHaveBeenCalled()
+  })
+
+  it('does not search for anything until start is called', () => {
+    createMdnsBrowser({ onService, ports: [31950] })
+
+    expect(createBrowser).toHaveBeenCalledTimes(0)
+    expect(baseBrowser.discover).toHaveBeenCalledTimes(0)
+  })
+
+  it('does not search for anything until base mdns browser is ready', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+
+    expect(createBrowser).toHaveBeenCalledWith(Mdns.tcp('http'))
+    expect(baseBrowser.discover).toHaveBeenCalledTimes(0)
+  })
+
+  it('can stop the browser', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+    browser.stop()
+
+    expect(baseBrowser.stop).toHaveBeenCalled()
+  })
+
+  it('can restart the browser', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+
+    expect(createBrowser).toHaveBeenCalledTimes(1)
+    expect(baseBrowser.discover).toHaveBeenCalledTimes(1)
+    expect(baseBrowser.stop).toHaveBeenCalledTimes(0)
+
+    browser.start()
+    baseBrowser.emit('ready')
+
+    expect(createBrowser).toHaveBeenCalledTimes(2)
+    expect(baseBrowser.discover).toHaveBeenCalledTimes(2)
+    expect(baseBrowser.stop).toHaveBeenCalledTimes(1)
+  })
+
+  it('calls onService when a service is emitted', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+    baseBrowser.emit('update', MOCK_BROWSER_SERVICE)
+
+    expect(onService).toHaveBeenCalledWith({
+      name: 'opentrons-dev',
+      ip: '192.168.1.42',
+      port: 31950,
+    })
+  })
+
+  it('ignores advertisements without names', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+    baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, fullname: undefined })
+
+    expect(onService).toHaveBeenCalledTimes(0)
+  })
+
+  it('ignores advertisements without ports', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+    baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, port: undefined })
+
+    expect(onService).toHaveBeenCalledTimes(0)
+  })
+
+  it('ignores advertisements without addresses', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+    baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, addresses: [] })
+
+    expect(onService).toHaveBeenCalledTimes(0)
+  })
+
+  it('prefers IPv4 addresses', () => {
+    const browser = createMdnsBrowser({ onService, ports: [31950] })
+    const addresses = ['fe80::caf4:6db4:4652:e975', '192.168.1.42']
+
+    browser.start()
+    baseBrowser.emit('ready')
+    baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, addresses })
+
+    expect(onService).toHaveBeenCalledWith({
+      name: 'opentrons-dev',
+      ip: '192.168.1.42',
+      port: 31950,
+    })
+  })
+
+  it('can filter based on ports', () => {
+    const browser = createMdnsBrowser({ onService, ports: [12345] })
+
+    browser.start()
+    baseBrowser.emit('ready')
+    baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE })
+    baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, port: 12345 })
+
+    expect(onService).toHaveBeenCalledTimes(1)
+    expect(onService).toHaveBeenCalledWith({
+      name: 'opentrons-dev',
+      ip: '192.168.1.42',
+      port: 12345,
+    })
+  })
+})

discovery-client/src/index.js

@@ -8,7 +8,7 @@ import toRegex from 'to-regex'
 import differenceBy from 'lodash/differenceBy'
 import xorBy from 'lodash/xorBy'
 
-import { createMdnsBrowser, getKnownIps } from './mdns-browser'
+import { createMdnsBrowserLegacy, getKnownIps } from './mdns-browser'
 import { poll, stop, type PollRequest } from './poller'
 import {
   createServiceList,
@@ -196,7 +196,7 @@ export class DiscoveryClient extends EventEmitter {
   _startBrowser(): void {
     this._stopBrowser()
 
-    const browser = createMdnsBrowser()
+    const browser = createMdnsBrowserLegacy()
       .once('ready', () => browser.discover())
       .on('update', service => this._handleUp(service))
       .on('error', error => this.emit('error', error))

discovery-client/src/mdns-browser.js

@@ -1,19 +1,89 @@
 // @flow
 // mdns browser wrapper
-import mdns, { ServiceType } from 'mdns-js'
-import type { Browser, NetworkConnection } from 'mdns-js'
+import net from 'net'
+import { createBrowser, tcp, ServiceType } from 'mdns-js'
 import keys from 'lodash/keys'
 import flatMap from 'lodash/flatMap'
 
+import type {
+  Browser as BaseBrowser,
+  BrowserService,
+  NetworkConnection,
+} from 'mdns-js'
+
+import type { MdnsBrowser, MdnsBrowserOptions, LogLevel } from './types'
+
 monkeyPatchThrowers()
 
-export function createMdnsBrowser(): Browser {
-  return mdns.createBrowser(mdns.tcp('http'))
+// TODO(mc, 2020-07-14): remove the function in favor of createMdnsBrowser
+export function createMdnsBrowserLegacy(): BaseBrowser {
+  return createBrowser(tcp('http'))
+}
+
+/**
+ * Create a mDNS browser wrapper can be started and stopped and calls
+ * `onService` when the underlying browser receives an advertisement
+ */
+export function createMdnsBrowser(options: MdnsBrowserOptions): MdnsBrowser {
+  const { onService, ports, logger } = options
+  const log = (level: LogLevel, msg: string, meta: {} = {}) => {
+    typeof logger?.[level] === 'function' && logger[level](msg, meta)
+  }
+
+  let browser: BaseBrowser | null = null
+
+  const start = () => {
+    stop()
+
+    log('debug', 'Creating _http._tcp mDNS browser', { ports })
+
+    const baseBrowser = createBrowser(tcp('http'))
+
+    baseBrowser
+      .once('ready', () => baseBrowser.discover())
+      .on('update', (service: BrowserService) => {
+        const { fullname, addresses, port } = service
+        const ip = addresses.find(address => net.isIPv4(address))
+
+        if (
+          fullname != null &&
+          ip != null &&
+          port != null &&
+          ports.includes(port)
+        ) {
+          const name = fullname.replace(/\._http\._tcp.local$/, '')
+          onService({ name, ip, port })
+        } else {
+          log('debug', 'Ignoring mDNS service', { service })
+        }
+      })
+      .on('error', (e: Error) => {
+        log('error', 'Unexpected mDNS browser error', { message: e.message })
+      })
+
+    browser = baseBrowser
+  }
+
+  const stop = () => {
+    if (browser) {
+      log('debug', 'Stopping mDNS browser')
+      browser.stop()
+      browser.removeAllListeners('ready')
+      browser.removeAllListeners('update')
+      browser.removeAllListeners('error')
+      browser = null
+    }
+  }
+
+  return { start, stop }
 }
 
-export function getKnownIps(maybeBrowser: ?Browser): Array<string> {
+// TODO(mc, 2020-07-14): remove the function because it's part of the mDNS
+// "rediscovery" logic that doesn't work well
+// https://github.com/Opentrons/opentrons/issues/5985
+export function getKnownIps(maybeBrowser: ?BaseBrowser): Array<string> {
   if (!maybeBrowser) return []
-  const browser: Browser = maybeBrowser
+  const browser: BaseBrowser = maybeBrowser
 
   return flatMap<NetworkConnection, string>(
     browser.networking.connections,
@@ -25,6 +95,12 @@ export function getKnownIps(maybeBrowser: ?Browser): Array<string> {
   )
 }
 
+/**
+ * The `ServiceType` class in mdns-js can throw in an uncatchable way when
+ * it receives certain types of advertisements that happen in real life. This
+ * function monkeypatches the ServiceType prototype to catch the throws
+ * https://github.com/mdns-js/node-mdns-js/issues/82
+ */
 function monkeyPatchThrowers() {
   // this method can throw (without emitting), so we need to patch this up
   const originalServiceTypeFromString = ServiceType.prototype.fromString

discovery-client/src/types.js

@@ -164,6 +164,28 @@ export type MdnsBrowserService = $ReadOnly<{|
 |}>
 
 /**
+ * Options used to construct an mDNS browser
+ */
+export type MdnsBrowserOptions = $ReadOnly<{|
+  /** list of allowed ports; if empty, no services will be emitted */
+  ports: $ReadOnlyArray<number>,
+  /** Function to call whenever a service is discovered on mDNS */
+  onService: (service: MdnsBrowserService) => mixed,
+  /** Optional logger */
+  logger?: Logger,
+|}>
+
+/**
+ * An mDNS browser that can be started and stopped as needed
+ */
+export type MdnsBrowser = $ReadOnly<{|
+  /** Start discovering services */
+  start: () => void,
+  /** Stop discovering services and tear down the underlying browser */
+  stop: () => void,
+|}>
+
+/*
  * Robot object that the DiscoveryClient returns that combines latest known
  * health data from the robot along with possible IP addressess
  */

flow-typed/npm/mdns-js_v1.0.x.js

@@ -1,10 +1,8 @@
 // flow-typed signature: 41a424ad7c0d0083d9498f60975cb2db
 // flow-typed version: <<STUB>>/mdns-js_v1.0.1/flow_v0.76.0
 
-import EventEmitter from 'events'
-
 declare module 'mdns-js' {
-  declare class mdns$Browser extends EventEmitter {
+  declare class mdns$Browser extends events$EventEmitter {
     discover(): void;
     stop(): void;
     networking: {