Adds a new helpful tool exit message for SocketExceptions thrown during mdns discovery by LouiseHsu · Pull Request #157638 · flutter/flutter

LouiseHsu · 2024-10-25T20:58:14Z

Addresses #150131, but doesn't fix it, as there seem to be cases where the steps included in the messages added in this PR don't work.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

packages/flutter_tools/lib/src/mdns_discovery.dart

loic-sharma · 2024-10-29T22:53:01Z

packages/flutter_tools/lib/src/mdns_discovery.dart

+            'You may be having a permissions issue with your IDE. '
+                'Please try going to '
+                'System Settings -> Privacy & Security -> Local Network -> '
+                '[Find your IDE] -> Toggle ON, then restarting your phone.',


Minor formatting nit:

Suggested change

'You may be having a permissions issue with your IDE. '

'Please try going to '

'System Settings -> Privacy & Security -> Local Network -> '

'[Find your IDE] -> Toggle ON, then restarting your phone.',

'You might be having a permissions issue with your IDE. '

'Please try going to '

'System Settings -> Privacy & Security -> Local Network -> '

'[Find your IDE] -> Toggle ON, then restarting your phone.',

loic-sharma · 2024-10-29T22:53:16Z

packages/flutter_tools/lib/src/mdns_discovery.dart

+                '[Find your IDE] -> Toggle ON, then restarting your phone.',
+          );
+        } else {
+          throwToolExit('');


Should we just rethrow; here instead? Or perhaps add a generic message like Unable to listen for mDNS connections?

Ah, i think rethrow is better. I will add that.

loic-sharma · 2024-10-29T22:55:29Z

packages/flutter_tools/test/general.shard/mdns_discovery_test.dart

  final Map<String, List<TxtResourceRecord>> txtResponse;
  final Map<String, List<IPAddressResourceRecord>> ipResponse;
  final bool osErrorOnStart;
+  final bool socketErrorOnStart;


Ultra pendantic nit: errors in Dart are unrecoverable exceptions that shouldn't be caught (see this doc). I'd consider renaming this to socketExceptionOnStart

loic-sharma · 2024-10-30T18:28:44Z

packages/flutter_tools/lib/src/mdns_discovery.dart

+          throwToolExit(
+              'You might be having a permissions issue with your IDE. '
+              'Please try going to '
+              'System Settings -> Privacy & Security -> Local Network -> '
+              '[Find your IDE] -> Toggle ON, then restart your phone.');


Style nit:

Suggested change

throwToolExit(

'You might be having a permissions issue with your IDE. '

'Please try going to '

'System Settings -> Privacy & Security -> Local Network -> '

'[Find your IDE] -> Toggle ON, then restart your phone.');

throwToolExit(

'You might be having a permissions issue with your IDE. '

'Please try going to '

'System Settings -> Privacy & Security -> Local Network -> '

'[Find your IDE] -> Toggle ON, then restart your phone.',

);

wtf. I think my copy pasting is not perserving the spaces correctly.

You might have autoformat on, you should disable it for the Flutter repo (but you might want to keep it for the packages repo)

loic-sharma · 2024-10-30T18:33:11Z

packages/flutter_tools/test/general.shard/mdns_discovery_test.dart

+          throwsToolExit(message: 'You might be having a permissions issue with your IDE. '
+            'Please try going to '
+            'System Settings -> Privacy & Security -> Local Network -> '
+            '[Find your IDE] -> Toggle ON, then restart your phone.')


Hm I'm not sure how you're supposed to format a multi line string argument value... Maybe this?

Suggested change

throwsToolExit(message: 'You might be having a permissions issue with your IDE. '

'Please try going to '

'System Settings -> Privacy & Security -> Local Network -> '

'[Find your IDE] -> Toggle ON, then restart your phone.')

throwsToolExit(

message:

'You might be having a permissions issue with your IDE. '

'Please try going to '

'System Settings -> Privacy & Security -> Local Network -> '

'[Find your IDE] -> Toggle ON, then restart your phone.',

),

i believe u

loic-sharma

Looks good!

…own during mdns discovery (flutter/flutter#157638)

Manual roll requested by [email protected] flutter/flutter@fe71cad...0fe6153 2024-10-31 [email protected] Roll Flutter Engine from c40b0b602822 to f2154ef3e31c (8 revisions) (flutter/flutter#157926) 2024-10-31 [email protected] Hides added routes before top-most route finishes pushing (flutter/flutter#156104) 2024-10-31 [email protected] Fix cursor on hover expand/collapse icon (#155207) (flutter/flutter#155209) 2024-10-31 [email protected] Add test for `media_query_data.system_gesture_insets.0.dart` (flutter/flutter#157854) 2024-10-31 [email protected] Add and plumb `useImplicitPubspecResolution` across `flutter_tools`. (flutter/flutter#157879) 2024-10-31 [email protected] Roll Flutter Engine from 090c33aeae83 to c40b0b602822 (1 revision) (flutter/flutter#157896) 2024-10-31 [email protected] Roll Flutter Engine from 9295eeb6d3ce to 090c33aeae83 (4 revisions) (flutter/flutter#157893) 2024-10-30 [email protected] Roll Flutter Engine from 2bd854e23b61 to 9295eeb6d3ce (5 revisions) (flutter/flutter#157882) 2024-10-30 [email protected] Adds a new helpful tool exit message for SocketExceptions thrown during mdns discovery (flutter/flutter#157638) 2024-10-30 [email protected] Fix `GlowingOverscrollIndicator` examples (flutter/flutter#155203) 2024-10-30 [email protected] Roll Flutter Engine from 906a7ad88052 to 2bd854e23b61 (1 revision) (flutter/flutter#157878) 2024-10-30 [email protected] Upgrade templates to AGP 8.7/Gradle 8.10.2 (flutter/flutter#157872) 2024-10-30 [email protected] Roll Flutter Engine from 57ed5d338e7e to 906a7ad88052 (13 revisions) (flutter/flutter#157875) 2024-10-30 [email protected] Update Material 3 `LinearProgressIndicator` for new visual style (flutter/flutter#154817) 2024-10-30 [email protected] Roll Flutter Engine from 999797a2f690 to 57ed5d338e7e (5 revisions) (flutter/flutter#157833) 2024-10-30 [email protected] Add hidden `--no-implicit-pubspec-resolution` flag for one stable release. (flutter/flutter#157635) 2024-10-30 [email protected] Roll Packages from 028027e to 7cc1caa (5 revisions) (flutter/flutter#157864) 2024-10-30 [email protected] Mention partial PRs in the contributing docs (flutter/flutter#157863) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ng mdns discovery (flutter#157638) Addresses flutter#150131, but doesn't fix it, as there seem to be cases where the steps included in the messages added in this PR don't work.

### 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].  [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

) ### 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].  [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

…own during mdns discovery (flutter/flutter#157638)

LouiseHsu added 2 commits October 24, 2024 21:35

boop socket exceptions

d4b735b

typo!

d4cf9ad

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 25, 2024

Merge branch 'master' into socket-exception-warning

b92c9a3

LouiseHsu changed the title ~~Socket exception warning~~ Adds a new helpful tool exit message for SocketExceptions thrown during mdns discovery Oct 25, 2024

lint

c2e130f

LouiseHsu requested a review from jmagman October 25, 2024 23:25

LouiseHsu marked this pull request as ready for review October 25, 2024 23:25

loic-sharma reviewed Oct 29, 2024

View reviewed changes

packages/flutter_tools/lib/src/mdns_discovery.dart Show resolved Hide resolved

loic-sharma reviewed Oct 29, 2024

View reviewed changes

packages/flutter_tools/lib/src/mdns_discovery.dart Outdated Show resolved Hide resolved

loic-sharma reviewed Oct 29, 2024

View reviewed changes

packages/flutter_tools/lib/src/mdns_discovery.dart Outdated Show resolved Hide resolved

loic-sharma reviewed Oct 29, 2024

View reviewed changes

packages/flutter_tools/lib/src/mdns_discovery.dart Outdated Show resolved Hide resolved

LouiseHsu added 3 commits October 29, 2024 12:53

surface error and trace msgs

6043c5c

mac only.

8f53f40

typo

89defbc

LouiseHsu requested a review from loic-sharma October 29, 2024 22:09

loic-sharma reviewed Oct 29, 2024

View reviewed changes

LouiseHsu added 4 commits October 29, 2024 17:26

pr comments

b241124

globals import lol

d96e374

globals?

93ec95c

test justification

83f0053

LouiseHsu requested a review from loic-sharma October 30, 2024 17:13

loic-sharma reviewed Oct 30, 2024

View reviewed changes

.

d7d959b

loic-sharma reviewed Oct 30, 2024

View reviewed changes

loic-sharma approved these changes Oct 30, 2024

View reviewed changes

bonk

a646c39

LouiseHsu added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024

auto-submit bot merged commit ec04707 into flutter:master Oct 30, 2024

engine-flutter-autoroll mentioned this pull request Oct 31, 2024

Roll Flutter from fe71cadf1959 to 16622e67a051 (15 revisions) flutter/packages#7971

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

98ae293

…own during mdns discovery (flutter/flutter#157638)

engine-flutter-autoroll mentioned this pull request Oct 31, 2024

Roll Flutter from fe71cadf1959 to b01558c1cd70 (16 revisions) flutter/packages#7972

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

8d5f027

…own during mdns discovery (flutter/flutter#157638)

engine-flutter-autoroll mentioned this pull request Oct 31, 2024

Roll Flutter from fe71cadf1959 to 0fe615343bd9 (18 revisions) flutter/packages#7973

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

80141eb

…own during mdns discovery (flutter/flutter#157638)

engine-flutter-autoroll mentioned this pull request Oct 31, 2024

Manual roll Flutter from fe71cadf1959 to 0fe615343bd9 (18 revisions) flutter/packages#7989

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

62b75f4

…own during mdns discovery (flutter/flutter#157638)

LouiseHsu added cp: beta cherry pick this pull request to beta release candidate branch cp: stable cherry pick this pull request to stable release candidate branch labels Nov 14, 2024

jmagman mentioned this pull request Jan 15, 2025

On macOS Sequoia - [tool_crash] SocketException: Send failed, OS Error: No route to host, errno = 65 #150131

Open

loic-sharma mentioned this pull request Jan 18, 2025

Show error on macOS if missing Local Network permissions #161846

Merged

9 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

40aef90

…own during mdns discovery (flutter/flutter#157638)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

773ded9

…own during mdns discovery (flutter/flutter#157638)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

49bf65a

…own during mdns discovery (flutter/flutter#157638)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

610b1be

…own during mdns discovery (flutter/flutter#157638)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

ec04707 Adds a new helpful tool exit message for SocketExceptions thr…

47d732c

…own during mdns discovery (flutter/flutter#157638)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a new helpful tool exit message for SocketExceptions thrown during mdns discovery#157638

Adds a new helpful tool exit message for SocketExceptions thrown during mdns discovery#157638
auto-submit[bot] merged 13 commits intoflutter:masterfrom
LouiseHsu:socket-exception-warning

LouiseHsu commented Oct 25, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loic-sharma Oct 29, 2024

Uh oh!

loic-sharma Oct 29, 2024 •

edited

Loading

Uh oh!

LouiseHsu Oct 29, 2024

Uh oh!

loic-sharma Oct 29, 2024

Uh oh!

loic-sharma Oct 30, 2024

Uh oh!

LouiseHsu Oct 30, 2024

Uh oh!

loic-sharma Oct 30, 2024

Uh oh!

loic-sharma Oct 30, 2024

Uh oh!

LouiseHsu Oct 30, 2024

Uh oh!

loic-sharma left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

LouiseHsu commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LouiseHsu commented Oct 25, 2024 •

edited

Loading

loic-sharma Oct 29, 2024 •

edited

Loading