-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[multicast_dns] Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency - #79772 #6700
base: main
Are you sure you want to change the base?
Conversation
…r Simplicity and Efficiency
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.
Apologies for the delay reviewing this.
@vashworth kindly tested this out against the flutter
CLI and confirmed it didn't break wireless debugging for flutter run
or flutter attach
.
This change generally LGTM, though I would like a second opinion from @cbracken who might remember more about why the original code was written that way.
- This needs a pubspec version bump and CHANGELOG update. See https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version-and-changelog-updates
- Can you add a test to https://github.com/flutter/packages/blob/main/packages/multicast_dns/test/client_test.dart?
Taking a look! |
I did a bit of exploratory The existing code was derived from the (long-dead, now) Dartino code and was added in It's derived from older Dartino code from the Dart SDK. The most recent version of that can be viewed here: The Dartino mdns package was originally added in this commit: Looking at the actual logic in the context of the rest of the code now. |
Thanks for sending the patch. Thinking about this a bit, I agree that setting up a single socket on 0.0.0.0 should be sufficient for listening on all network interfaces. As you note, for sending, we'll rely on the operating system's routing to determine which network interface is used for sending. That all seems fine. The current package doesn't really allow for interface-specific tuning/filtering so I don't think we're losing anything here and the code and resource management is significantly simpler. @sgjesse wrote the original code and while he's moved on to other projects, may have thoughts. :) The existing tests should cover the majority of the behaviour here, but as @jmagman says, please add a test that verifies the change in behaviour. If you check the existing tests, you can see how to use the You could probably write a test where the
then verify that the counter is == 1 and that the only targetAddress passed in was |
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.
looks good modulo comments + as @jmagman says, please add a test (suggestion above).
…6 to null after closing sockets
@jmagman is it the following command enough for accomplishing point 1? sorry if it's sounds like a stupid question but I ran it locally and it changed dart run script/tool/bin/flutter_plugin_tools.dart update-release-info \
--version=bugfix \
--base-branch=fix-issue-79772 \
--changelog="Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency." |
You could edit the cc @stuartmorgan re: the |
The docs say to use The command run above used this PR's branch as the base branch, which means it will (by definition) find no changes. The repo tooling always interprets no changes as "assume everything changed" as a failsafe to prevent CI from silently running no tests if something goes wrong with diff computation. (Even if it didn't, that command still wouldn't have update the package, it would have just done nothing.) (The |
Thanks for the explanation @stuartmorgan / @jmagman , 👇 did the trick: ➜ multicast_dns git:(fix-issue-79772) dart run ../../script/tool/bin/flutter_plugin_tools.dart update-release-info --current-package \
--version=bugfix \
--changelog="Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency." |
finally all checks are green 😅 |
Taking another look! Thanks! |
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.
Just one comment, other than that looks good.
It may be worth adding an equivalent test for IPv6.
for (final RawDatagramSocket socket in _sockets) { | ||
socket.send(packet, _mDnsAddress!, selectedMDnsPort); | ||
if (_incomingIPv4 == null) { | ||
throw StateError('Unable to send packet on null socket.'); |
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.
Won't this cause an exception to be thrown if the connection is over IPv6?
For IPv6 in the old code, we'd just have no sockets to send over and not send anything, but now we'll throw an exception. I think you can just drop this check and everything should be fine, since you're using a null-safe dereference on the send
call below.
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.
I'm curious why we didn't use equivalent logic for the IPv6 approach in the old code -- i.e. we never sent on the IPv6 equivalent of 0.0.0.0, only on the sockets bound to the individual network interfaces. I'll do a little digging to see if I can understand why.
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.
Won't this cause an exception to be thrown if the connection is over IPv6?
correct!
I think you can just drop this check and everything should be fine
Removed. I did not go deeper on the IPV6
topic, I saw other MDNs packages (like Hashicorp multicast mdns package) that they handle also multicast
with IPV6
but I did not want to go deeper in this PR. Maybe it's worth to open another issue for implementing it (or have a look if it's doable or not), what do you think? 🤔
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.
I guess this thread can be closed, right?
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.
I'm curious if you've had a chance to test the previous code with IPv6. My worry is that the existing code was broken for IPv4 (and this fixes it), but this patch effectively removes IPv6 support altogether. That seems fine if it was previously broken, but if not, it's a regression for those users.
Other that phone users connecting over a cell connection, I suspect people with an IPv6-only connection are pretty few and far between so possibly not the end of the world, but still a regression.
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 balance, landing this is probably still a net win, but probably worth at least documenting the change so users are aware, and someone can land a fix if they need it.
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.
Thinking about it a little more I think there are two options here:
- We add back the
_sockets
List (maybe as_ipv6InterfaceSockets
) and the code that configures them as multicast -- but only when anInternetAddress.anyIPv6
is passed to the constructor, and then thesend
code at the bottom similarly would either send on_incomingIPv4
for IPv4, or the collection of IPv6 sockets for IPv6. - We update the constructor to specifically throw (maybe UnimplementedError) if
InternetAddress.anyIPv6
is passed in and document that IPv6 support has been removed, in the class/package docs.
Sorry to spot this at this point.
@cbracken could you take another look at this when you get a chance? |
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.
Overall this looks great for IPv4. We'll want to either document the removal of IPv6 support (if the previous code didn't exist) or add it back (if it did).
I encountered this package long time ago (see linked issue below) and there were cases where it wasn't working.
After 3 years (yeah it's more time than expected) I managed to find the time to dust off
wireshark
and have look again.Preamble
Considering the following setup
Where Raspberry pi runs a
mDNS
service using the followinggo
:main.go
Considering the following client (which I got from here):
client.dart
What happens
When running the client script (
dart run client.dart
so with the latest package ofmulticast_dns
package) as is, a list of sockets is created which are bind to port5353
andIP
s:0.0.0.0
;127.0.0.1
;192.168.2.16
;172.17.0.1
;a list of interfaces (see list below) are joined to the multicast socket which is bound to
0.0.0.0:5353
:lo
(with address127.0.0.1
);wlan0
(with address192.168.2.16
);docker0
(with address172.17.0.1
).and eventually when
lookup
function is being calledQM
queries are being sent fromALL
the sockets in the list; which means that for0.0.0.0
theIP
address chosen by the operating system will depend on various factors such as the routing table, the default network interface, and the specific configuration of the network interfaces on the machine. It could be sent from any of theIP
addresses associated with the machine's network interfaces, includingIP
addresses assigned to physical network adapters or virtual interfaces.Using
Wireshark
, I can see that 2QM
packets are being sent and I can see thatmDNS
service is responding to the client with proper packet but it seems that the socket opened at0.0.0.0:5353
is not reading them at all even though the socket is still open.First approach (not sure if it's RFC 6762 friendly)
I had the "feeling" that sending
QM
packets to0.0.0.0:5353
and other interfaces on the same port would generate some sort of unexpected behavior due to the nature of0.0.0.0
whichIP
selections depends on multiple factors.Therefore I tried initially to change the
incoming
socket (the one bound to0.0.0.0
) from:to
which essentially delegates to
OS
to choose a random port (instead of forcing5353
).In this case the client managed to process correctly all the packages for discovering the
mDNS
service, indeed inWireshark
I could see:and on the client I could see the message:
RFC 6762
friendly, I checked some packages which implementmDNS
clients and I saw some of them doing what I proposed. I would like to hear comments about it.Second approach (which it's what is presented in this PR)
After trying the first approach I realized that maybe there is no need to open sockets on more interfaces (and therefore send
QM
messages) it maybe be enough to send and listen only on a socket bound to0.0.0.0
since, again, listen on ANYIP
and send packets from a selectedIP
address chosen by theOS
.Also in this case the client managed to process correctly all the packages for discovering the
mDNS
service, indeed inWireshark
I could see:and on the client I could see the message:
Third approach (It did not work but mentioning for completeness)
The idea here is to don't send
QM
packets via0.0.0.0
but just listen on possible response/s since packets would be send via the followingIP
s and0.0.0.0
should represent ANYIP
.127.0.0.1
;192.168.2.16
;172.17.0.1
.Fourth approach (It did not work but mentioning for completeness)
Another solution that I tried but unfortunately it did not work, was to put
0.0.0.0
as last item in the socket list soQM
packets would be sent according to the following order:127.0.0.1
;192.168.2.16
;172.17.0.1
;0.0.0.0
.multicast_dns.start() function
The idea is indeed to let the first 3
IP
s to send theQM
packets which response should be hopefully captured by theincoming
socket before the socket on0.0.0.0
would send theQM
packet too.Wireshark filter
Related Issue
Disclaimers
flutter
/dart
, I pulled the code and I tried to debug it with help of unclegoogle
andprint()
;Wireshark
, inspect the networks and craft packets.Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.