Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unpublish dns-sd service #747

Merged
merged 7 commits into from
Sep 30, 2024
Merged

fix: unpublish dns-sd service #747

merged 7 commits into from
Sep 30, 2024

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Sep 19, 2024

We were not unpublishing the zeroconf service when the app went into the background, but we were re-publishing the service when the app came into the foreground (the same with wifi coming off and on).

When zeroconf attempts to publish a service with the same name, it appends (${n}) to the published service name.

The result of this was that the app would leave multiple published dns-sd services e.g.

8130fb3b21ac48b6
8130fb3b21ac48b6 (1)
8130fb3b21ac48b6 (2)

Only the most recent would have the correct port - the older services would have the port from previous app opens.

Leaving these services published does not break anything per se, but the consequences are:

  1. Increased mdns network traffic, because other mdns devices share the service information.
  2. Slower connections between CoMapeo devices and possible performance overhead, because CoMapeo will attempt to connect to every service published.

This PR correctly unpublishes the service when the app goes into the background or the wifi turns off. Unfortunately the react-native-zeroconf library has not implemented an event for publishing or unpublishing failing, so we implement a timeout, but don't throw an error, because just trying again is a reasonable solution. We do report the error to Sentry though so that we can track these failures.

I have tested this by opening and closing the app while viewing this DNS-SD Browser for MacOS, but you could also test via avahi-browse -a -v --ignore-local --resolve.

I do not have two devices at the moment to test on real devices, this would benefit from that.

I don't think this is necessary for MVP, but could be worth releasing in a hotfix if there are discovery issues with large numbers of devices on the same network.

@gmaclennan gmaclennan self-assigned this Sep 19, 2024
@gmaclennan
Copy link
Member Author

This is possibly creating too much noise with Sentry because of reporting timeouts when stopping service adverstisement. Could investigate when that happens (probably when wifi is turned off) and be more selective in what we report to sentry.

Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, but a few nitpicks.

src/frontend/contexts/LocalDiscoveryContext.tsx Outdated Show resolved Hide resolved
src/frontend/contexts/LocalDiscoveryContext.tsx Outdated Show resolved Hide resolved
src/frontend/contexts/LocalDiscoveryContext.tsx Outdated Show resolved Hide resolved
EvanHahn added a commit that referenced this pull request Sep 30, 2024
I think this is a useful change on its own but should also make [an
upcoming change][0] easier.

[0]: #747
EvanHahn added a commit that referenced this pull request Sep 30, 2024
I think this is a useful change on its own but should also make [an
upcoming change][0] easier.

[0]: #747
@EvanHahn EvanHahn merged commit c55af0b into develop Sep 30, 2024
3 checks passed
@EvanHahn EvanHahn deleted the fix/zeroconf-unpublish branch September 30, 2024 17:08
@EvanHahn
Copy link
Contributor

This is possibly creating too much noise with Sentry because of reporting timeouts when stopping service adverstisement. Could investigate when that happens (probably when wifi is turned off) and be more selective in what we report to sentry.

I didn't address this comment because this PR was blocking a release, but I still think it's worth considering.

I'll bring it up with the team soon.

achou11 pushed a commit that referenced this pull request Oct 3, 2024
I think this is a useful change on its own but should also make [an
upcoming change][0] easier.

[0]: #747
achou11 pushed a commit that referenced this pull request Oct 3, 2024
ErikSin pushed a commit that referenced this pull request Oct 3, 2024
…#773)

I think this is a useful change on its own but should also make [an
upcoming change][0] easier.

[0]: #747

Co-authored-by: Evan Hahn <[email protected]>
ErikSin added a commit that referenced this pull request Oct 3, 2024
Co-authored-by: Gregor MacLennan <[email protected]>
Co-authored-by: Evan Hahn <[email protected]>
Co-authored-by: ErikSin <[email protected]>
ErikSin added a commit that referenced this pull request Oct 14, 2024
* chore: Github action for creating release candidate (#618)

* chore: update version

* chore: create flow and configure release types

* chore: edit title names

* chore: documentation

* chore: spelling

* chore: remove 'the'

* chore: 'there are features'

* chore: 'repo uses github actions'

* chore: 'develop'

* chore: add 'RC' and SHA to version

* chore: update name

* chore: typos

* chore: take out unnecessary file

* chore: update release candidate action to properly name pr (#639)

* fix: allow metrics env variables to be inlined (#643)

To quote [Expo's docs][0]:

> **Every environment variable must be statically referenced as a
> property of `process.env` using JavaScript's dot notation for it to be
> inlined.** For example, the expression `process.env.EXPO_PUBLIC_KEY`
> is valid and will be inlined.

Our metrics environment variables weren't doing this, so they couldn't
be inlined.

Closes [#640].

[0]: https://docs.expo.dev/guides/environment-variables/#how-to-read-from-environment-variables
[#640]: #640

* fix: use proper timestamp for location metadata (#645)

We should be using an ISO timestamp instead of a string number here.

Because `position` can be undefined and we were checking that earlier, I
moved things around a tiny bit to only check that once.

Closes [#644].

[#644]: #644

* feature: edit a track (#633)

* Adds edit icon to track. 
* Navigates to edit track screen if it is user's track. 
* Edit track screen added using the editor component
* Hook added for editing a track
* Doesn't allow preset to change if it is a track

* feat: show config details and upload new config (#608)

* chore: scaffolding for screens

* chore: translations

* chore: added import button

* chore: translations

* chore: create select file function

* chore: create select file and import config hook

* chore: use hook

* chore: use reusable file picker

* chore: update file extension to be 'comapeocat'

* Update `@mapeo/core` to 9.0.0-alpha.23 (#630)

* WIP: update `@mapeo/core` to 9.0.0-alpha.17

This isn't finished but does most of the upgrading needed.

* WIP: update to `@mapeo/[email protected]`

* chore: update to latest core, ipc, and schema

* chore: update to latest core, ipc, and schema in backend

* chore: update type

* chore: remove attachments from tracks

* Update `@mapeo/core` to latest (again)

---------

Co-authored-by: ErikSin <[email protected]>

* chore: EXPO_PUBLIC_FEATURE_TEST_DATA_UI set to true in RC profile (#652)

* chore: test data generator

* chore: take out npx executable

* Changes to using the new hook for matching a device to itself. (#653) (#667)

Co-authored-by: cimigree <[email protected]>

* Fixes ternary on editing an observation for changing preset (#668)

* fix: leave current project when joining another project (#646) (#670)

* fix: project name appears after successfully joining via invite (#672) (#690)

* chore: update copy to warn user about potential loss of data (#691) (#693)

* chore: copy

* chore: translations

* chore: copy

* chore: copy

* chore:translations

* chore: add period

* chore: styling

* feat: show date user added to project and screen instructing user on how to leave project (#674) (#692)

* chore: update device card and device name

* chore: translations

* chore: update device card

* chore: mapeo => CoMapeo

* chore: unnecessary array

* chore: update deviceCard

* chore: title sizing

* chore: bold

* chore: add question icon

* chore: remove unusedimport

* chore: remove unneccesary justify content

* fix: manage "obscured" config variables via babel env variable transform (#656) (#695)

Co-authored-by: Gregor MacLennan <[email protected]>

* fix: prevent text overflow in invitee role selection screen (#682) (#698)

* fix: update copy to replace Mapeo with CoMapeo (#684) (#699)

* fix: presets forever loading (#642) (#701)

Co-authored-by: cimigree <[email protected]>

* fix: handle category-related changes after importing a config more elegantly (#687) (#700)

* fix: do not set observation metadata when using manual gps coordinate (#705) (#706)

Co-authored-by: Evan Hahn <[email protected]>

* chore: tidy type for location callback (#710)

Cherry-pick of 306aa51.

* fix: simplify "accurate location" hook (#712)

Cherry picked from commit a8c7390.

* fix: create test data with proper metadata (#713)

Cherry picked from commit 81514f9.

* fix: always specify a device type (#711)

Cherry picked from commit cf09155.

* chore: add env file template (#697) (#714)

* fix: populate subject field when sharing observation via email (#680) (#715)

* fix: manual gps prompt when no location service or innacurate location (#702) (#719)

* fix: prompts user to manually add gps when no or inaccurate location

* chore: translations

* chore: small fixes

* chore: pr fixes

* chore: remove project config icon (#717) (#721)

* chore: New Crowdin updates (#694) (#722)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Mongolian)

* New translations en.json (Dutch)

* New translations en.json (Vietnamese)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Indonesian)

* New translations en.json (Khmer)

* New translations en.json (Tamil)

* New translations en.json (Thai)

* New translations en.json (Burmese)

* New translations en.json (Sinhala)

* New translations en.json (Swahili)

* New translations en.json (Nepali)

* New translations en.json (Ogiek)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Mongolian)

* New translations en.json (Dutch)

* New translations en.json (Vietnamese)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Indonesian)

* New translations en.json (Khmer)

* New translations en.json (Tamil)

* New translations en.json (Thai)

* New translations en.json (Burmese)

* New translations en.json (Sinhala)

* New translations en.json (Swahili)

* New translations en.json (Nepali)

* New translations en.json (Ogiek)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Portuguese, Brazilian)

---------

Co-authored-by: Digidem Bot <[email protected]>

* chore: update dependencies (#716) (#723)

* chore: update dependencies

This updates all of our (Co)Mapeo dependencies to their latest versions.
This should be the last time we do this before the 1.0 releases of those
modules.

* Default config's extension is `.comapeocat`

* chore: update mapeo-schema for ESM fix

* chore: update backend deps

* chore: update @electron/asar patch

* fix: temp fix

---------

Co-authored-by: Evan Hahn <[email protected]>
Co-authored-by: Gregor MacLennan <[email protected]>

* chore: update EAS config (#726) (#727)

* chore: update EAS config node version & VCS

* remove versionCode from app.json

* chore: change how versions are named (#725) (#728)

* chore: change how versions are named

* set node version used for builds

* chore: update EAS config node version & VCS

* remove versionCode from app.json

* nit: capitalize RC

* Changes isEnabled for metrics permission to be enabled by default. (#734)

* fix: update message content when sharing observation (#676) (#736)

* chore: New Crowdin updates (#733) (#737)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Mongolian)

* New translations en.json (Dutch)

* New translations en.json (Vietnamese)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Indonesian)

* New translations en.json (Khmer)

* New translations en.json (Tamil)

* New translations en.json (Thai)

* New translations en.json (Burmese)

* New translations en.json (Sinhala)

* New translations en.json (Swahili)

* New translations en.json (Nepali)

* New translations en.json (Ogiek)

Co-authored-by: Digidem Bot <[email protected]>

* chore: update CoMapeo dependencies to final release versions (#732) (#738)

This updates CoMapeo dependencies to their final release versions.
Notably, several names changed. For example, `@mapeo/core` is now
`@comapeo/core`.

There are no other changes other than renames and version bumps.

Co-authored-by: Evan Hahn <[email protected]>

* fix: mapeo/schema to comapeo/schema (#739) (#741)

* chore: remove test data generator (#735)

* fix: update to `@comapeo/[email protected]` (#746)

Cherry pick of 9b121f0.

* fix: fix issue with category icons not loading within reasonable time after importing a config (#748) (#750)

* chore: move query invalidation of config-related queries to useImportProjectConfig() (#749) (#751)

* chore: uninstall unused mock data package (#752) (#753)

This change should have no user impact.

This uninstalls `@mapeo/mock-data`, as it is unused.

Co-authored-by: Evan Hahn <[email protected]>

* chore: run CI workflow on pushes to develop and release-related branches (#740) (#754)

* fix: Only allow coordinator to update config (#756) (#759)

* chore: check if user is coordinator

* chore: create useGetOwnRole hook

* chore: useGetOwnRole hook in config screen

* chore: change role key

* chore: update `@types/react-native-zeroconf` to latest version (#763) (#773)

I think this is a useful change on its own but should also make [an
upcoming change][0] easier.

[0]: #747

Co-authored-by: Evan Hahn <[email protected]>

* fix: unpublish dns-sd service (#747) (#774)

Co-authored-by: Gregor MacLennan <[email protected]>
Co-authored-by: Evan Hahn <[email protected]>
Co-authored-by: ErikSin <[email protected]>

* chore: configure Sentry environment (#730) (#775)

Co-authored-by: Gregor MacLennan <[email protected]>

* chore: update copy (#767) (#776)

* chore: updated copy

* chore: take out unused variables

* New Crowdin updates (#743) (#777)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Mongolian)

* New translations en.json (Dutch)

* New translations en.json (Vietnamese)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Indonesian)

* New translations en.json (Khmer)

* New translations en.json (Tamil)

* New translations en.json (Thai)

* New translations en.json (Burmese)

* New translations en.json (Sinhala)

* New translations en.json (Swahili)

* New translations en.json (Nepali)

* New translations en.json (Ogiek)

* New translations en.json (Spanish)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Mongolian)

* New translations en.json (Dutch)

* New translations en.json (Vietnamese)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Indonesian)

* New translations en.json (Khmer)

* New translations en.json (Tamil)

* New translations en.json (Thai)

* New translations en.json (Burmese)

* New translations en.json (Sinhala)

* New translations en.json (Swahili)

* New translations en.json (Nepali)

* New translations en.json (Ogiek)

* New translations en.json (Spanish)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Spanish)

* New translations en.json (French)

* New translations en.json (Spanish)

* New translations en.json (German)

* New translations en.json (Japanese)

* New translations en.json (Mongolian)

* New translations en.json (Dutch)

* New translations en.json (Vietnamese)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Indonesian)

* New translations en.json (Khmer)

* New translations en.json (Tamil)

* New translations en.json (Thai)

* New translations en.json (Burmese)

* New translations en.json (Sinhala)

* New translations en.json (Swahili)

* New translations en.json (Nepali)

* New translations en.json (Ogiek)

---------

Co-authored-by: Digidem Bot <[email protected]>

* feat: set preset language to match app language (#783) (#784)

* feat: set preset language to match app language

* feat: translate fields

* test: load env variables so `npm test` works locally (#742) (#790)

We [set environment variables in CI][0] so that `npm test` works, but it
doesn't work locally because Jest isn't loading them.

This loads them when running Jest.

[0]: https://github.com/digidem/comapeo-mobile/blob/9b6cf8f71a0d960174c7e8cd5ce1b6bb8f1dd5a7/.github/workflows/ci.yml#L72-L75

Co-authored-by: Evan Hahn <[email protected]>
Co-authored-by: Gregor MacLennan <[email protected]>

* test: update Jest to latest version, 29.7.0 (#781) (#791)

This updates `jest` and `@types/jest` to their latest versions.

Co-authored-by: Evan Hahn <[email protected]>

* fix: Fix changing preset & store `presetRef` on observation (#785) (#792)

* fix: Fix changing preset & store preset ref on observation

* fix: revert removed ts-expect-error

(my vscode was using a newer version of typescript to check this, leading me to think this was not needed)

* chore: improve code legibility of usePreset

* chore: add unit test for updatePreset

* fix: missing presetRef when first creating observation without photos

* fix: editing an observation was not correctly removing tags

Co-authored-by: Gregor MacLennan <[email protected]>

* New Crowdin updates (#780) (#793)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Portuguese, Brazilian)

* New translations en.json (Spanish)

Co-authored-by: Digidem Bot <[email protected]>

* chore: take our background location permission

---------

Co-authored-by: ErikSin <[email protected]>
Co-authored-by: Evan Hahn <[email protected]>
Co-authored-by: cimigree <[email protected]>
Co-authored-by: Gregor MacLennan <[email protected]>
Co-authored-by: Andrew Chou <[email protected]>
Co-authored-by: Digidem Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants