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 multicast xmit windows #110

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

cpuchip
Copy link

@cpuchip cpuchip commented Jul 27, 2022

Adding to @davidflowerday's PR I've made this platform specific so windows/others uses SetMulticastInterface and linux/darwin uses ControlMessage, I've also updated server.go to include a fix for this problem as well.

I was having issues getting this library to even work on windows, only mDNS services I registered on localhost would show up in my browse/lookup requests, after digging in I found @davidflowerday's PR and a few other comments on other forks.

I've tests this fix on Windows and and linux (WSL) and both seem to be in good shape, I can resolve and register mDNS service both on and off box and see them. I do not have a mac ( Intel or Arm ) to test this on unfortunately.

from @davidflowerday's PR
I'm attempting to use go-chromecast on Windows which leverages this library. Unfortunately, mDNS wasn't working. I was able to fix it by replacing the code that used ControlMessage in WriteTo() with a call to SetMulticastInterface() instead. I think this may be related to issues #54 #69 and #75. I've tested this change on Windows and Linux and I will test on macOS shortly as well. I only changed client.go but it's possible a similar change is needed in server.go as well.

In the docs for the ipv4 package under Bugs there is this note:

On Windows, the ControlMessage for ReadFrom and WriteTo methods of PacketConn is not implemented.

Additionally, the ipv4 docs on Multicasting here show an example using SetMulticastInterface() as I've done in this PR which suggests to me that this is an OK way to handle this issue, but if you'd prefer something else I'd be happy to change it.

Thanks for making zeroconf and for considering this PR.

edaniels added a commit to edaniels/zeroconf that referenced this pull request Jan 19, 2023
@edaniels
Copy link

I think this basically works! I did get one failure but it looks flaky

--- FAIL: TestNoRegister (5.00s)
    service_test.go:81: Expected empty service entries but got {{test--xxxxxxxxxxxx _test--xxxx._tcp [] local. _test--xxxx._tcp.local. test--xxxxxxxxxxxx._test--xxxx._tcp.local. _services._dns-sd._udp.local.} ermbp.local. 8888 [txtv=0 lo=1 la=2] 3200 [10.1.8.177 100.84.156.104] [fe80::1 fe80::646e:ff:feaa:b671 fe80::646e:ff:feaa:b672 fe80::646e:ff:feaa:b670 fd74:4071:a3e3:cbcf:1c85:aae2:74be:9eea fe80::88bd:54ff:fec5:c37c fe80::88bd:54ff:fec5:c37c fe80::b881:defd:d07e:f0c7 fe80::675b:f3fb:7934:fcd4 fe80::ce81:b1c:bd2c:69e fe80::29c8:2e74:2c05:9fb fe80::a49:5af5:601:b0ae fd7a:115c:a1e0:ab12:4843:cd96:6254:9c68]}

Pushed your changes to yet another fork https://github.com/edaniels/zeroconf/tree/register_dynamic_fork. Maybe upvote #112!

edaniels added a commit to edaniels/zeroconf that referenced this pull request Jan 19, 2023
edaniels added a commit to edaniels/zeroconf that referenced this pull request Jan 19, 2023
Copy link
Owner

@grandcat grandcat left a comment

Choose a reason for hiding this comment

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

Thanks for making this Platform-specific change requested in the issue!
Looks okay to me, let's give it a try.

@grandcat grandcat merged commit e4f60f8 into grandcat:master Jan 19, 2023
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.

4 participants