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

Use start/stop interface on mDNS to correctly stop servers #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jessepollak
Copy link

This PR correctly starts and stops the DNS server that advertises Sonos devices. Prior to this PR, if a Nodetunes instance was stopped, the DNS would continue to be broadcasted.

@jabooth
Copy link

jabooth commented Dec 11, 2016

Hey @jessepollak!

Just looking at this PR, can you help me understand the problem that it fixes? I haven't seen any issues with the device advertisement persisting when it shouldn't, at least when just using airsonos.

The code certainly looks to tidy up the tear-down of the connection, I'm just struggling to picture a sequence of steps where it becomes important - right now when I kill airsonos devices disappear after a few seconds...am I missing something? Thanks!

@jessepollak
Copy link
Author

@jabooth the use case where this was necessary was that I had a long running process where I wanted to start and stop airsonos multiple times (I was working on a mac app to wrap this functionality). When I was programmatically stopping airsonos, the DNS advertisement was not stopping — this PR makes it stop.

The functionality you're seeing (where the devices dissappear) is likely happening because you are starting and stopping the whole airsonos process. When the process is killed, all orphaned advertisements are also killed.

@jabooth
Copy link

jabooth commented Dec 12, 2016

@jessepollak makes total sense, thanks for taking the time to explain 👍

iainbrighton added a commit to iainbrighton/nodetunes that referenced this pull request Jan 6, 2017
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