From 5810bd8970476f7afc3b4c01c5febcdbe5f3b70b Mon Sep 17 00:00:00 2001 From: Gregor MacLennan Date: Thu, 19 Sep 2024 16:42:25 +0100 Subject: [PATCH 1/5] fix: unpublish dns-sd service --- .../contexts/LocalDiscoveryContext.tsx | 117 +++++++++++++++--- 1 file changed, 101 insertions(+), 16 deletions(-) diff --git a/src/frontend/contexts/LocalDiscoveryContext.tsx b/src/frontend/contexts/LocalDiscoveryContext.tsx index 068e6c863..8d032f6f4 100644 --- a/src/frontend/contexts/LocalDiscoveryContext.tsx +++ b/src/frontend/contexts/LocalDiscoveryContext.tsx @@ -8,6 +8,7 @@ import NetInfo, { import StateMachine from 'start-stop-state-machine'; import Zeroconf, {type Service as ZeroconfService} from 'react-native-zeroconf'; import {type MapeoClientApi} from '@comapeo/ipc'; +import * as Sentry from '@sentry/react-native'; import noop from '../lib/noop'; type LocalDiscoveryController = ReturnType< @@ -33,6 +34,12 @@ const POLL_WIFI_STATE_INTERVAL_MS = 2000; const ZEROCONF_SERVICE_TYPE = 'comapeo'; const ZEROCONF_PROTOCOL = 'tcp'; const ZEROCONF_DOMAIN = 'local.'; +// react-native-zeroconf does not notify when a service fails to register or unregister +// https://github.com/balthazar/react-native-zeroconf/blob/master/android/src/main/java/com/balthazargronon/RCTZeroconf/nsd/NsdServiceImpl.java#L210 +// so we need a timeout, otherwise the service would never be considered +// "started" or "stopped", which would stop browsing for peers. +const ZEROCONF_PUBLISH_TIMEOUT_MS = 5000; +const ZEROCONF_UNPUBLISH_TIMEOUT_MS = 5000; const LocalDiscoveryContext = React.createContext< LocalDiscoveryController | undefined @@ -91,33 +98,48 @@ export function createLocalDiscoveryController(mapeoApi: MapeoClientApi) { }; let cancelNetInfoFetch: undefined | (() => void); const zeroconf = new Zeroconf(); + // In edge-cases, we may end up with multiple published names (NSD service + // will append ` (1)` to the name if there is a conflict), so we need to track + // them here so that we can unpublish them when we stop the service. + const publishedNames = new Set(); const sm = new StateMachine({ async start() { - const [{name, port}] = await Promise.all([ - mapeoApi.startLocalPeerDiscoveryServer(), - startZeroconf(zeroconf), - ]); - zeroconf.publishService( - ZEROCONF_SERVICE_TYPE, - ZEROCONF_PROTOCOL, - ZEROCONF_DOMAIN, - name, - port, + // start browsing straight away + const startZeroconfPromise = startZeroconf(zeroconf); + const {name, port} = await mapeoApi.startLocalPeerDiscoveryServer(); + // publishedName could be different from the name we requested, if there + // was a conflict on the network (the conflict could come from the same + // name still being registered on the network and not yet cleaned up) + const publishedName = await publishZeroconf(zeroconf, {name, port}).catch( + e => { + // Publishing could fail (timeout), but we don't want to throw the + // state machine start(), because that would leave the state machine + // in an "error" state and stop other things from working. By silently + // failing (with the report to Sentry), we are able to try again next + // time. + Sentry.captureException(e); + }, ); + if (publishedName) publishedNames.add(publishedName); + await startZeroconfPromise; }, async stop() { await Promise.all([ mapeoApi.stopLocalPeerDiscoveryServer(), stopZeroconf(zeroconf), + unpublishZeroconf(zeroconf, publishedNames).catch(e => { + // See above for why we silently fail here + Sentry.captureException(e); + }), ]); }, }); - sm.on('state', state => { - if (state.value === 'error') { - updateState({status: 'error', error: state.error}); + sm.on('state', smState => { + if (smState.value === 'error') { + updateState({status: 'error', error: smState.error}); } else { - updateState({status: state.value}); + updateState({status: smState.value}); } }); const listeners = new Set<() => void>(); @@ -163,9 +185,9 @@ export function createLocalDiscoveryController(mapeoApi: MapeoClientApi) { cancel = true; }; NetInfo.fetch('wifi') - .then(state => { + .then(netInfoState => { if (cancel) return; - onNetInfo(state); + onNetInfo(netInfoState); }) .catch(noop); } @@ -298,6 +320,40 @@ function startZeroconf(zeroconf: Zeroconf): Promise { }); } +function publishZeroconf( + zeroconf: Zeroconf, + {name, port}: {name: string; port: number}, +): Promise { + return new Promise((resolve, reject) => { + const timeoutId = setTimeout(() => { + cleanup(); + reject(new Error('Timed out publishing zeroconf service')); + }, ZEROCONF_PUBLISH_TIMEOUT_MS); + + const cleanup = () => { + clearTimeout(timeoutId); + zeroconf.off('published', onPublish); + }; + const onPublish = ({name: publishedName}: ZeroconfService) => { + cleanup(); + resolve(publishedName); + }; + + zeroconf.on( + // @ts-expect-error - the types are wrong, this is the correct event name + 'published', + onPublish, + ); + zeroconf.publishService( + ZEROCONF_SERVICE_TYPE, + ZEROCONF_PROTOCOL, + ZEROCONF_DOMAIN, + name, + port, + ); + }); +} + function stopZeroconf(zeroconf: Zeroconf): Promise { return new Promise((resolve, reject) => { const cleanup = () => { @@ -319,6 +375,35 @@ function stopZeroconf(zeroconf: Zeroconf): Promise { }); } +function unpublishZeroconf( + zeroconf: Zeroconf, + publishedNames: Set, +): Promise { + if (publishedNames.size === 0) return Promise.resolve(); + return new Promise((resolve, reject) => { + const timeoutId = setTimeout(() => { + cleanup(); + reject(new Error('Timed out unpublishing zeroconf service')); + }, ZEROCONF_UNPUBLISH_TIMEOUT_MS); + + const cleanup = () => { + clearTimeout(timeoutId); + zeroconf.off('remove', onRemove); + }; + const onRemove = (name: string) => { + publishedNames.delete(name); + if (publishedNames.size === 0) { + cleanup(); + resolve(); + } + }; + zeroconf.on('remove', onRemove); + for (const name of publishedNames) { + zeroconf.unpublishService(name); + } + }); +} + function zeroconfServiceToMapeoPeer({ addresses, port, From 01b0d0ed89f9adb8781e0dbd5eb44d19ebdebf91 Mon Sep 17 00:00:00 2001 From: Gregor MacLennan Date: Mon, 30 Sep 2024 16:03:28 +0100 Subject: [PATCH 2/5] Update src/frontend/contexts/LocalDiscoveryContext.tsx Co-authored-by: Evan Hahn --- src/frontend/contexts/LocalDiscoveryContext.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/frontend/contexts/LocalDiscoveryContext.tsx b/src/frontend/contexts/LocalDiscoveryContext.tsx index 8d032f6f4..a842a5fdb 100644 --- a/src/frontend/contexts/LocalDiscoveryContext.tsx +++ b/src/frontend/contexts/LocalDiscoveryContext.tsx @@ -340,7 +340,7 @@ function publishZeroconf( }; zeroconf.on( - // @ts-expect-error - the types are wrong, this is the correct event name + // @ts-expect-error We can remove this when is merged. 'published', onPublish, ); From 30223c32b8515d884519ae7ae3bdffc59dd01b72 Mon Sep 17 00:00:00 2001 From: Gregor MacLennan Date: Mon, 30 Sep 2024 16:03:36 +0100 Subject: [PATCH 3/5] Update src/frontend/contexts/LocalDiscoveryContext.tsx Co-authored-by: Evan Hahn --- .../contexts/LocalDiscoveryContext.tsx | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/frontend/contexts/LocalDiscoveryContext.tsx b/src/frontend/contexts/LocalDiscoveryContext.tsx index a842a5fdb..a50dc4c8e 100644 --- a/src/frontend/contexts/LocalDiscoveryContext.tsx +++ b/src/frontend/contexts/LocalDiscoveryContext.tsx @@ -111,17 +111,17 @@ export function createLocalDiscoveryController(mapeoApi: MapeoClientApi) { // publishedName could be different from the name we requested, if there // was a conflict on the network (the conflict could come from the same // name still being registered on the network and not yet cleaned up) - const publishedName = await publishZeroconf(zeroconf, {name, port}).catch( - e => { - // Publishing could fail (timeout), but we don't want to throw the - // state machine start(), because that would leave the state machine - // in an "error" state and stop other things from working. By silently - // failing (with the report to Sentry), we are able to try again next - // time. - Sentry.captureException(e); - }, - ); - if (publishedName) publishedNames.add(publishedName); + try { + const publishedName = await publishZeroconf(zeroconf, {name, port}); + publishedNames.add(publishedName); + } catch (e) { + // Publishing could fail (timeout), but we don't want to throw the + // state machine start(), because that would leave the state machine + // in an "error" state and stop other things from working. By silently + // failing (with the report to Sentry), we are able to try again next + // time. + Sentry.captureException(e); + } await startZeroconfPromise; }, async stop() { From 24ff95a03392cf3e40b9a32ec98c085a7cfea8c9 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 30 Sep 2024 16:59:01 +0000 Subject: [PATCH 4/5] Remove unnecessary ts-expect-error --- src/frontend/contexts/LocalDiscoveryContext.tsx | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/frontend/contexts/LocalDiscoveryContext.tsx b/src/frontend/contexts/LocalDiscoveryContext.tsx index a50dc4c8e..e044f6be4 100644 --- a/src/frontend/contexts/LocalDiscoveryContext.tsx +++ b/src/frontend/contexts/LocalDiscoveryContext.tsx @@ -339,11 +339,7 @@ function publishZeroconf( resolve(publishedName); }; - zeroconf.on( - // @ts-expect-error We can remove this when is merged. - 'published', - onPublish, - ); + zeroconf.on('published', onPublish); zeroconf.publishService( ZEROCONF_SERVICE_TYPE, ZEROCONF_PROTOCOL, From 5cb2e2fc808da2d28e4258967fd1d583f8f38f3a Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 30 Sep 2024 17:00:11 +0000 Subject: [PATCH 5/5] Rename arg for clarity --- src/frontend/contexts/LocalDiscoveryContext.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/frontend/contexts/LocalDiscoveryContext.tsx b/src/frontend/contexts/LocalDiscoveryContext.tsx index e044f6be4..cc2ccf015 100644 --- a/src/frontend/contexts/LocalDiscoveryContext.tsx +++ b/src/frontend/contexts/LocalDiscoveryContext.tsx @@ -373,9 +373,9 @@ function stopZeroconf(zeroconf: Zeroconf): Promise { function unpublishZeroconf( zeroconf: Zeroconf, - publishedNames: Set, + publishedNamesToBeMutated: Set, ): Promise { - if (publishedNames.size === 0) return Promise.resolve(); + if (publishedNamesToBeMutated.size === 0) return Promise.resolve(); return new Promise((resolve, reject) => { const timeoutId = setTimeout(() => { cleanup(); @@ -387,14 +387,14 @@ function unpublishZeroconf( zeroconf.off('remove', onRemove); }; const onRemove = (name: string) => { - publishedNames.delete(name); - if (publishedNames.size === 0) { + publishedNamesToBeMutated.delete(name); + if (publishedNamesToBeMutated.size === 0) { cleanup(); resolve(); } }; zeroconf.on('remove', onRemove); - for (const name of publishedNames) { + for (const name of publishedNamesToBeMutated) { zeroconf.unpublishService(name); } });