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

add support for SO_REUSEPORT #89

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

Conversation

lanrat
Copy link
Contributor

@lanrat lanrat commented Apr 27, 2021

Adds support for SO_REUSEPORT.

This allows for multiple mDNS clients/servers/services running on the same host

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.

Generally looks good to me.
Would be nice to have a test for this, e.g. two clients listening + one server answering. What do you think?

@lanrat
Copy link
Contributor Author

lanrat commented Apr 27, 2021

Making a test is a good idea. But it would be dependent on the OS/Kernel that is running the test.

It would only work on platforms that support SO_REUSEPORT.

@@ -36,7 +37,7 @@ var (
)

func joinUdp6Multicast(interfaces []net.Interface) (*ipv6.PacketConn, error) {
udpConn, err := net.ListenUDP("udp6", mdnsWildcardAddrIPv6)

Choose a reason for hiding this comment

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

Why we need SO_REUSEPORT? For multicast address, SO_REUSEPORT is the same as SO_REUSEADDR which was already set by default when you call net.ListenUDP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the Debian Linux system I am running this on, I have a few other mdns services. When starting a service using this library net.ListenUDP would fail due to the port already being in use.

After switching it to use SO_REUSEPORT to be inline with all the other mDNS services I was able to successfully run it with the other services at the same time without issue. [1,2]

It is my understanding that SO_REUSEPORT is not needed if all the services are part of the same program (PID), but if they are not; SO_REUSEPORT is required to tell the kernel it is ok for different processes to share the same address/port. Which is required for mDNS.

[1] https://github.com/udp-redux/udp-broadcast-relay-redux/blob/master/main.c#L382
[2] https://github.com/lathiat/avahi/blob/d1e71b320d96d0f213ecb0885c8313039a09f693/avahi-core/socket.c#L178

Choose a reason for hiding this comment

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

Thanks @lanrat for the details. After some digging and exercises, I agree that set both SO_REUSEADDR and SO_REUSEPORT is better than just setting SO_REUSEADDR.
I think the reason net.LIstenUDP failed in your env is some other mdns services running only enabled SO_REUSEPORT.

"It is my understanding that SO_REUSEPORT is not needed if all the services are part of the same program (PID), but if they are not; SO_REUSEPORT is required to tell the kernel it is ok for different processes to share the same address/port. Which is required for mDNS."
-> I don't agree with above comments. Based on my test, SO_REUSEADDR works quite well for multiple processes for mDNS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"It is my understanding that SO_REUSEPORT is not needed if all the services are part of the same program (PID), but if they are not; SO_REUSEPORT is required to tell the kernel it is ok for different processes to share the same address/port. Which is required for mDNS."
-> I don't agree with above comments. Based on my test, SO_REUSEADDR works quite well for multiple processes for mDNS.

This comment was based on the results of the limited testing I did, so I could very well be wrong here. I'll research this a bit more to fully understand what's going on.

Either way, adding SO_REUSEPORT does appear to fix the problem as I'm running 5+ mdns services on the same host.

@lanrat
Copy link
Contributor Author

lanrat commented Jun 9, 2021

@grandcat any updates on getting this or any of the other PRs merged?

@grandcat
Copy link
Owner

grandcat commented Jun 9, 2021

@grandcat any updates on getting this or any of the other PRs merged?

I believe the last state was to provide a test for this, even though it would only be tested on OSes which support it.
Right?

@lanrat
Copy link
Contributor Author

lanrat commented Jun 9, 2021

Thanks for the reminder. I'll add that to my backlog. ;)

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.

None yet

3 participants