-
Notifications
You must be signed in to change notification settings - Fork 172
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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?
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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. |
Thanks for the reminder. I'll add that to my backlog. ;) |
Adds support for SO_REUSEPORT.
This allows for multiple mDNS clients/servers/services running on the same host