Movatterモバイル変換

packages/flutter_tools/lib/src/mdns_discovery.dart

		}) async {
		final Completer<List<MDnsVmServiceDiscoveryResult>> completer =
		Completer<List<MDnsVmServiceDiscoveryResult>>();
		unawaited(

Copy link

MemberAuthor

loic-sharmaJan 18, 2025•
edited
Loading

Choose a reason for hiding this comment

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

If there's an error, theFuture returned byrunZonedGuarded never completes, hence theunawaited andCompleter. See:https://dart.dev/libraries/async/zones#example-errors-cant-leave-error-zones

This is inspired by the tool's test context:

flutter/packages/flutter_tools/test/src/context.dart

Lines 130 to 175 in9bf4e89

	// To catch all errors thrown by the test, even uncaught async errors, we use a zone.
	//
	// Zones introduce their own event loop, so we do not await futures created inside
	// the zone from outside the zone. Instead, we create a Completer outside the zone,
	// and have the test complete it when the test ends (in success or failure), and we
	// await that.
	finalCompleter<void> completer=Completer<void>();
	runZonedGuarded<Future<dynamic>>(
	()async {
	try {
	returnawait context.run<dynamic>(
	// Apply the overrides to the test context in the zone since their
	// instantiation may reference items already stored on the context.
	overrides: overrides,
	name:'test-specific overrides',
	body: ()async {
	if (initializeFlutterRoot) {
	// Provide a sane default for the flutterRoot directory. Individual
	// tests can override this either in the test or during setup.
	Cache.flutterRoot??=getFlutterRoot();
	}
	returnawaittestMethod();
	},
	);
	}finally {
	// We do not need a catch { ... } block because the error zone
	// will catch all errors and send them to the completer below.
	//
	// See https://github.com/flutter/flutter/pull/141821/files#r1462288131.
	if (!completer.isCompleted) {
	completer.complete();
	}
	}
	},
	(Object error,StackTrace stackTrace) {
	// When things fail, it's ok to print to the console!
	print(error);// ignore: avoid_print
	print(stackTrace);// ignore: avoid_print
	_printBufferedErrors(context);
	if (!completer.isCompleted) {
	completer.completeError(error, stackTrace);
	}
	throw error;//ignore: only_throw_errors
	},
	);
	return completer.future;

Show error on macOS if missing Local Network permissions

3b32c07

loic-sharma force-pushed thelocal_network_error branch from36ffb7c to3b32c07Compare

January 18, 2025 02:22

loic-sharma added3 commits

January 17, 2025 18:32

Add explaination

1e6c148

Polish

7c360df

Add comment to test

b986ead

loic-sharma commented

Jan 21, 2025

packages/flutter_tools/lib/src/mdns_discovery.dart

		bool useDeviceIPAsHost = false,
		required Duration timeout,
		bool quitOnFind = false,
		}) async {

Copy link

MemberAuthor

loic-sharmaJan 21, 2025

Choose a reason for hiding this comment

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

This diff is confusing. The only change to_pollingVmService's arguments is thatdeviceVmservicePort was renamed todeviceVmServicePort._pollingVmService and_doPollingVmService have the same arguments.

loic-sharma marked this pull request as ready for review

January 21, 2025 22:42

loic-sharma requested review fromLouiseHsu,andrewkolos,bkonyi andjonahwilliams

January 21, 2025 22:43

Copy link

MemberAuthor

loic-sharma commentedJan 21, 2025•
edited
Loading

~~@jonahwilliams Jenn told me you actually understand DartZones and all their implications. Would you be able to review this to double-check I'm not doing anything horrible?~~

EDIT:@matanlurey spot-checked theZone usage.

bkonyi approved these changes

Copy link

Contributor

bkonyi left a comment

Choose a reason for hiding this comment

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

Excellent! I'm looking forward to this no longer being our top crasher!

We should consider cherry picking this into stable if we can.

andrewkolos approved these changes

Copy link

Contributor

andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM

loic-sharma added the autosubmitMerge PR when tree becomes green via auto submit App label

auto-submitbot added this pull request to themerge queue

Merged via the queue intoflutter:master with commit136d8e5

166 checks passed

flutter-dashboardbot removed the autosubmitMerge PR when tree becomes green via auto submit App label

loic-sharma added the cp: stablecherry pick this pull request to stable release candidate branch label

Copy link

flutteractionsbot commentedJan 23, 2025

Failed to create CP due to merge conflicts.
You will need to create the PR manually. Seethe cherrypick wiki for more info.

loic-sharma added a commit to loic-sharma/flutter that referenced this pull request

Show error on macOS if missing Local Network permissions (flutter#161846

385397b

)### BackgroundmacOS Sequoia requires the user's permission to do multicast operations,which the Flutter tool does to connect to the Dart VM. If the app doesnot have permission to communicate with devices on the local network,the following happens:1. Flutter tool starts a [multicastlookup](https://github.com/flutter/flutter/blob/bb2d34126cc8161dbe4a1bf23c925e48b732f670/packages/flutter_tools/lib/src/mdns_discovery.dart#L238-L241)2. The mDNS client [sends data on thesocket](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L219)4. macOS blocks the operation. Dart's native socket implementationthrows 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 amicrotask](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1513)that [reports the exception through the socket'sstream](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'serrors](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 errorhandler](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/async/stream_impl.dart#L553).### ReproductionTo reproduce this error on macOS...1. Open System Settings > Privacy & Security > Local Network and toggleoff Visual Studio Code2. Run a Flutter app using a physical device### FixIdeally, we'd make `MDnsClient.lookup` throw an exception for thisscenario. Unfortunately, the `MDnsClient` can have multiple lookupoperations in parallel, and the `SocketException` doesn't give us enoughinformation to match it back to a pending `MDnsClient` request. Seeflutter/packages#8450 as an attempt to solvethis in the `MDnsClient` layer.Instead, this fix introduces a `Zone` in the tool to catch the socket'suncaught exception.Follow-up toflutter#157638See:flutter#150131## Pre-launch Checklist- [x] I read the [Contributor Guide] and followed the process outlinedthere for submitting PRs.- [x] I read the [Tree Hygiene] wiki page, which explains myresponsibilities.- [x] I read and followed the [Flutter Style Guide], including [Featureswe expect every widget to implement].- [x] I signed the [CLA].- [x] I listed at least one issue that this PR fixes in the descriptionabove.- [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 DrivenFixes] where supported.- [x] All existing and new tests are passing.If you need help, consider asking for advice on the #hackers-new channelon [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 mentioned this pull request

[beta] Show error on macOS if missing Local Network permissions#162119

Merged

engine-flutter-autoroll mentioned this pull request

Roll Flutter from c1561a41c530 to bf7daf22a51e (19 revisions)flutter/packages#8495

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request

136d8e5Show error on macOS if missing Local Network permissions (flu…

4ab76e7

…tter/flutter#161846)

engine-flutter-autoroll mentioned this pull request

Roll Flutter from c1561a41c530 to 02e11f95a18a (20 revisions)flutter/packages#8496

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request

136d8e5Show error on macOS if missing Local Network permissions (flu…

98be839

…tter/flutter#161846)

Wasmund1 pushed a commit to Wasmund1/flutter that referenced this pull request

Show error on macOS if missing Local Network permissions (flutter#161846

397616c

)### BackgroundmacOS Sequoia requires the user's permission to do multicast operations,which the Flutter tool does to connect to the Dart VM. If the app doesnot have permission to communicate with devices on the local network,the following happens:1. Flutter tool starts a [multicastlookup](https://github.com/flutter/flutter/blob/bb2d34126cc8161dbe4a1bf23c925e48b732f670/packages/flutter_tools/lib/src/mdns_discovery.dart#L238-L241)2. The mDNS client [sends data on thesocket](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L219)4. macOS blocks the operation. Dart's native socket implementationthrows 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 amicrotask](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1513)that [reports the exception through the socket'sstream](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'serrors](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 errorhandler](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/async/stream_impl.dart#L553).### ReproductionTo reproduce this error on macOS...1. Open System Settings > Privacy & Security > Local Network and toggleoff Visual Studio Code2. Run a Flutter app using a physical device### FixIdeally, we'd make `MDnsClient.lookup` throw an exception for thisscenario. Unfortunately, the `MDnsClient` can have multiple lookupoperations in parallel, and the `SocketException` doesn't give us enoughinformation to match it back to a pending `MDnsClient` request. Seeflutter/packages#8450 as an attempt to solvethis in the `MDnsClient` layer.Instead, this fix introduces a `Zone` in the tool to catch the socket'suncaught exception.Follow-up toflutter#157638See:flutter#150131## Pre-launch Checklist- [x] I read the [Contributor Guide] and followed the process outlinedthere for submitting PRs.- [x] I read the [Tree Hygiene] wiki page, which explains myresponsibilities.- [x] I read and followed the [Flutter Style Guide], including [Featureswe expect every widget to implement].- [x] I signed the [CLA].- [x] I listed at least one issue that this PR fixes in the descriptionabove.- [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 DrivenFixes] where supported.- [x] All existing and new tests are passing.If you need help, consider asking for advice on the #hackers-new channelon [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

engine-flutter-autoroll mentioned this pull request

Roll Flutter from c1561a41c530 to c1ffaa9d9deb (21 revisions)flutter/packages#8497

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request

136d8e5Show error on macOS if missing Local Network permissions (flu…

93936f2

…tter/flutter#161846)

engine-flutter-autoroll mentioned this pull request

Manual roll Flutter from c1561a41c530 to c1ffaa9d9deb (21 revisions)flutter/packages#8498

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request

136d8e5Show error on macOS if missing Local Network permissions (flu…

473556d

…tter/flutter#161846)

engine-flutter-autoroll mentioned this pull request

Roll Flutter from c1561a41c530 to c1ffaa9d9deb (21 revisions)flutter/packages#8500

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request

136d8e5Show error on macOS if missing Local Network permissions (flu…

8feca32

…tter/flutter#161846)

auto-submitbot pushed a commit to flutter/packages that referenced this pull request