[multicast_dns] Allow handling socket errors #8450
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
loic-sharma
commented
Jan 17, 2025
Comment on lines
+87
to
+90
Member
Author
There was a problem hiding this comment.
This mirrors the Stream.listen docs: https://api.flutter.dev/flutter/dart-async/Stream/listen.html
15c7c79 to
d54919a
Compare
d54919a to
3db88b5
Compare
Member
Author
|
On second thought, Jenn's suggestion of using a |
9 tasks
github-merge-queue bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Jan 23, 2025
### Background macOS Sequoia requires the user's permission to do multicast operations, which the Flutter tool does to connect to the Dart VM. If the app does not have permission to communicate with devices on the local network, the following happens: 1. Flutter tool starts a [multicast lookup](https://github.com/flutter/flutter/blob/bb2d34126cc8161dbe4a1bf23c925e48b732f670/packages/flutter_tools/lib/src/mdns_discovery.dart#L238-L241) 2. The mDNS client [sends data on the socket](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L219) 4. macOS blocks the operation. Dart's native socket implementation throws an `OSError` 5. Dart's `Socket.send` [catches the `OSError`](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1511-L1515), wraps it in a `SocketException`, and [schedules a microtask](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1513) that [reports the exception through the socket's stream](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/_internal/vm/bin/socket_patch.dart#L3011) ([`Socket` is a `Stream`](https://api.dart.dev/dart-io/Socket-class.html)) 6. The mDNS client [does not listen to the socket stream's errors](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L155), so [the error is sent to the current `Zone`'s uncaught error handler](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/async/stream_impl.dart#L553). ### Reproduction To reproduce this error on macOS... 1. Open System Settings > Privacy & Security > Local Network and toggle off Visual Studio Code 2. Run a Flutter app using a physical device ### Fix Ideally, we'd make `MDnsClient.lookup` throw an exception for this scenario. Unfortunately, the `MDnsClient` can have multiple lookup operations in parallel, and the `SocketException` doesn't give us enough information to match it back to a pending `MDnsClient` request. See flutter/packages#8450 as an attempt to solve this in the `MDnsClient` layer. Instead, this fix introduces a `Zone` in the tool to catch the socket's uncaught exception. Follow-up to #157638 See: #150131 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
loic-sharma
added a commit
to loic-sharma/flutter
that referenced
this pull request
Jan 23, 2025
) ### Background macOS Sequoia requires the user's permission to do multicast operations, which the Flutter tool does to connect to the Dart VM. If the app does not have permission to communicate with devices on the local network, the following happens: 1. Flutter tool starts a [multicast lookup](https://github.com/flutter/flutter/blob/bb2d34126cc8161dbe4a1bf23c925e48b732f670/packages/flutter_tools/lib/src/mdns_discovery.dart#L238-L241) 2. The mDNS client [sends data on the socket](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L219) 4. macOS blocks the operation. Dart's native socket implementation throws an `OSError` 5. Dart's `Socket.send` [catches the `OSError`](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1511-L1515), wraps it in a `SocketException`, and [schedules a microtask](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1513) that [reports the exception through the socket's stream](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/_internal/vm/bin/socket_patch.dart#L3011) ([`Socket` is a `Stream`](https://api.dart.dev/dart-io/Socket-class.html)) 6. The mDNS client [does not listen to the socket stream's errors](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L155), so [the error is sent to the current `Zone`'s uncaught error handler](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/async/stream_impl.dart#L553). ### Reproduction To reproduce this error on macOS... 1. Open System Settings > Privacy & Security > Local Network and toggle off Visual Studio Code 2. Run a Flutter app using a physical device ### Fix Ideally, we'd make `MDnsClient.lookup` throw an exception for this scenario. Unfortunately, the `MDnsClient` can have multiple lookup operations in parallel, and the `SocketException` doesn't give us enough information to match it back to a pending `MDnsClient` request. See flutter/packages#8450 as an attempt to solve this in the `MDnsClient` layer. Instead, this fix introduces a `Zone` in the tool to catch the socket's uncaught exception. Follow-up to flutter#157638 See: flutter#150131 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Wasmund1
pushed a commit
to Wasmund1/flutter
that referenced
this pull request
Jan 24, 2025
) ### Background macOS Sequoia requires the user's permission to do multicast operations, which the Flutter tool does to connect to the Dart VM. If the app does not have permission to communicate with devices on the local network, the following happens: 1. Flutter tool starts a [multicast lookup](https://github.com/flutter/flutter/blob/bb2d34126cc8161dbe4a1bf23c925e48b732f670/packages/flutter_tools/lib/src/mdns_discovery.dart#L238-L241) 2. The mDNS client [sends data on the socket](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L219) 4. macOS blocks the operation. Dart's native socket implementation throws an `OSError` 5. Dart's `Socket.send` [catches the `OSError`](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1511-L1515), wraps it in a `SocketException`, and [schedules a microtask](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1513) that [reports the exception through the socket's stream](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/_internal/vm/bin/socket_patch.dart#L3011) ([`Socket` is a `Stream`](https://api.dart.dev/dart-io/Socket-class.html)) 6. The mDNS client [does not listen to the socket stream's errors](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L155), so [the error is sent to the current `Zone`'s uncaught error handler](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/async/stream_impl.dart#L553). ### Reproduction To reproduce this error on macOS... 1. Open System Settings > Privacy & Security > Local Network and toggle off Visual Studio Code 2. Run a Flutter app using a physical device ### Fix Ideally, we'd make `MDnsClient.lookup` throw an exception for this scenario. Unfortunately, the `MDnsClient` can have multiple lookup operations in parallel, and the `SocketException` doesn't give us enough information to match it back to a pending `MDnsClient` request. See flutter/packages#8450 as an attempt to solve this in the `MDnsClient` layer. Instead, this fix introduces a `Zone` in the tool to catch the socket's uncaught exception. Follow-up to flutter#157638 See: flutter#150131 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
macOS Sequoia requires user consent to do multicast operations, which the Flutter tool does to connect to the Dart VM. We'd like to make the Flutter tool provide a more helpful error message when mDNS fails due to lack of permission. See: flutter/flutter#150131
If the app does not have permission to communicate with devices on the local network, the following happens:
OSErrorSocket.sendcatches theOSError, wraps it in aSocketException, and schedules a microtask that reports the exception through the socket's stream (Socketis aStream)Zone's uncaught error handler.Solution
This patch adds a
onSocketErrorcallback. This allows the caller to catch errors reported by the underlyingSocketand provide a better error message to the user.I'm not very happy with this API. Unfortunately, the
SocketExceptiondoesn't give us enough information to match it back to a pendingMDnsClientrequest. Please let me know if you have suggestions on a better API to handle this error!This will help us improve the error message for: flutter/flutter#150131
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.