Change iOS device discovery from polling to long-running observation #58137

jmagman · 2020-05-28T01:24:08Z

Description

For daemon iOS device polling, change periodic xcdevice list calls to a long-running xcdevice observe call.

Related Issues

Fixes #56826

Tests

xcdevice.observedDeviceEvents tests
start polling devices test
ItemListNotifier tests

Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.

jmagman · 2020-05-28T01:24:37Z

packages/flutter_tools/lib/src/macos/xcode.dart

+      // Run in interactive mode (via script) to convince
+      // xcdevice it has a terminal attached and pipe stdout.


Learned this trick from https://github.com/flutter/flutter/pull/12079/files#diff-5a9a07600dceb28cbe887a5a64a24b0aR502-R504.

jmagman · 2020-05-28T01:26:07Z

packages/flutter_tools/lib/src/device.dart

  Timer _timer;

  Future<List<Device>> pollingGetDevices({ Duration timeout });

-  void startPolling() {
+  Future<void> startPolling() async {


g3 PollingDeviceDiscovery subclasses don't override startPolling or stopPolling.

jmagman · 2020-05-28T23:59:21Z

packages/flutter_tools/lib/src/device.dart

+
+  @protected
+  @visibleForTesting
+  ItemListNotifier<Device> deviceNotifier;


Renamed _items -> deviceNotifier

packages/flutter_tools/lib/src/base/utils.dart

packages/flutter_tools/lib/src/ios/devices.dart

jonahwilliams · 2020-05-29T00:34:58Z

packages/flutter_tools/lib/src/ios/devices.dart

+      }, onDone: () {
+      // If the xcdevice process is killed, restart it.
+      _logger.printTrace('xcdevice observe stopped, restarting');
+      startPolling();


So I'm understanding this correctly: since we're relying on a single long running process now, it needs to restart in case it goes down unexpectedly, otherwise IDEs wouldn't be able to discover new iOS devices without restarting - right?

jonahwilliams · 2020-05-29T16:43:09Z

packages/flutter_tools/lib/src/ios/devices.dart

+        _logger.printTrace('xcdevice observe stopped, restarting');
+
+        // If the xcdevice process is killed, give it a few seconds and restart it.
+        Timer(const Duration(seconds: 1), () {


What if the device discovery exits immediately on start up (something is broken). Will this keep scheduling work in a loop? Likewise if this is killed as part of tear down.

PollingDeviceDiscovery polling currently schedules a timer every 4 seconds to call device-specific discovery code, so that's the existing behavior. It depends what we want the behavior to be. If I killall xcdevice (exits 137), I would expect it to get rescheduled and the IDE not to need to be restarted. However if it just fails in some other way, rerunning it probably won't help? @zanderso What do you think?

Restarting after stopPolling is bad though...

Its a bit of a tricky situation. How common is it for this process to go down during an IDE session? It might be less risky to log a message and move on

I never saw it organically go down in my testing, but I also never saw my hard drive fill up with xcdevice caches described in #56826.

Good opportunity for metrics?

yeah, if we're not sure lets measure

It looks like close() on the StreamController for this Stream isn't called, so how does the 'onDone' callback get invoked?

It is now. See comment at #58137 (comment).

zanderso · 2020-05-29T18:23:01Z

packages/flutter_tools/lib/src/ios/devices.dart

+        _logger.printTrace('xcdevice observe stopped, restarting');
+
+        // If the xcdevice process is killed, give it a few seconds and restart it.
+        Timer(const Duration(seconds: 1), () {


It looks like close() on the StreamController for this Stream isn't called, so how does the 'onDone' callback get invoked?

packages/flutter_tools/lib/src/macos/xcode.dart

jmagman · 2020-05-29T21:00:12Z

packages/flutter_tools/lib/src/macos/xcode.dart

+  void _setupDeviceIdentifierByEventStream() {
+    // _deviceIdentifierByEvent Should always be available for listeners
+    // in case polling needs to be stopped and restarted.
+    _deviceIdentifierByEvent = StreamController<Map<XCDeviceEvent, String>>.broadcast(
+      onListen: _startObservingTetheredIOSDevices,
+      onCancel: _stopObservingTetheredIOSDevices,
+    );
+  }


This was the piece I was missing when I wasn't closing _deviceIdentifierByEvent on the last commit. I want polling to be able to be started, stopped, and re-started. Before, I was avoiding closing it so it could be re-listened to, but I guess it should be closed when no one is listening.

Really I want to be able to pause the subscription and resume it, but the pause() documentation says:

* To avoid buffering events on a broadcast stream, it is better to * cancel this subscription, and start to listen again when events * are needed, if the intermediate events are not important.

jmagman · 2020-05-29T21:03:02Z

packages/flutter_tools/lib/src/macos/xcode.dart

+          'xcrun',
+          'xcdevice',
+          'observe',
+          '--both',


I also changed this to --both to reflect what I just did in https://github.com/flutter/flutter/pull/58257/files#diff-8d108d6a887a4f5b74f7c7d77003e795R97. Wireless devices aren't being sufaced yet, so if one attaches it will kick off an unnecessary fetch. Should be rare.

packages/flutter_tools/lib/src/macos/xcode.dart

zanderso

lgtm after addressing @jonahwilliams comments.

packages/flutter_tools/lib/src/ios/devices.dart

jonahwilliams

LGTM

tvolkert · 2020-06-17T20:18:42Z

packages/flutter_tools/lib/src/device.dart

-    _items ??= ItemListNotifier<Device>();
-    return _items.onRemoved;
+    deviceNotifier ??= ItemListNotifier<Device>();
+    return deviceNotifier.onRemoved;
  }

  void dispose() => stopPolling();


Shouldn't this be async now?

tvolkert · 2020-06-17T20:22:17Z

packages/flutter_tools/lib/src/ios/devices.dart

       super('iOS devices');

+  @override
+  void dispose() {
+    _observedDeviceEventsSubscription?.cancel();


Should this not call super.dispose()?

Also, if the super signature was async, this would just be await stopPolling()

jmagman added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. t: xcode "xcodebuild" on iOS and general Xcode project management labels May 28, 2020

jmagman self-assigned this May 28, 2020

fluttergithubbot added the work in progress; do not review label May 28, 2020

googlebot added the cla: yes label May 28, 2020

jmagman commented May 28, 2020

View reviewed changes

jmagman force-pushed the observe-attaches branch 2 times, most recently from 6740b4a to 941b59c Compare May 28, 2020 23:58

jmagman commented May 28, 2020

View reviewed changes

jmagman requested review from jonahwilliams and zanderso May 29, 2020 00:00

jmagman removed the work in progress; do not review label May 29, 2020

jmagman marked this pull request as ready for review May 29, 2020 00:00

jonahwilliams reviewed May 29, 2020

View reviewed changes

zanderso reviewed May 29, 2020

View reviewed changes

jmagman mentioned this pull request May 29, 2020

flutter run should support wireless debugging of iOS devices #15072

Closed

jmagman force-pushed the observe-attaches branch from a8a61fd to cc883cf Compare May 29, 2020 20:56

jmagman commented May 29, 2020

View reviewed changes

anirudhb mentioned this pull request May 29, 2020

flutter_tools: iOS wireless debugging support #58185

Closed

9 tasks

jmagman force-pushed the observe-attaches branch from 0a4f7e8 to 3c5f9a1 Compare May 29, 2020 23:27

jonahwilliams reviewed Jun 1, 2020

View reviewed changes

packages/flutter_tools/lib/src/macos/xcode.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Jun 1, 2020

View reviewed changes

packages/flutter_tools/lib/src/macos/xcode.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Jun 1, 2020

View reviewed changes

packages/flutter_tools/lib/src/macos/xcode.dart Show resolved Hide resolved

jonahwilliams reviewed Jun 1, 2020

View reviewed changes

packages/flutter_tools/lib/src/macos/xcode.dart Outdated Show resolved Hide resolved

zanderso approved these changes Jun 1, 2020

View reviewed changes

packages/flutter_tools/lib/src/ios/devices.dart Outdated Show resolved Hide resolved

Change iOS device discovery from polling to long-running observation

fdd1fbd

jmagman added 6 commits June 1, 2020 11:30

Fix failing tests, actually reschedule process

3872acc

Allow polling to be restarted

766d9c0

Fix command test

22a99ef

Remove flushMicrotasks

6983da4

Unused import

80f2cf1

Review edits

29bdefb

jmagman force-pushed the observe-attaches branch from ff18b4d to 29bdefb Compare June 1, 2020 18:31

jonahwilliams approved these changes Jun 1, 2020

View reviewed changes

jmagman added the waiting for tree to go green label Jun 1, 2020

fluttergithubbot merged commit f4d26a3 into flutter:master Jun 1, 2020

jmagman mentioned this pull request Jun 1, 2020

xcdevice is generating a dyld_shared_cache_arm64 recovered file every ~30 seconds while IDE open #56826

Closed

jmagman deleted the observe-attaches branch June 1, 2020 21:10

jmagman mentioned this pull request Jun 17, 2020

Change iOS device discovery from polling to long-running observation #59695

Merged

tvolkert reviewed Jun 17, 2020

View reviewed changes

jmagman mentioned this pull request Jun 17, 2020

Clean up PollingDeviceDiscovery dispose #59709

Merged

10 tasks

jmagman mentioned this pull request Jun 29, 2020

Fix daemon device discovery crash when Xcode isn't installed #60546

Merged

10 tasks

DanTup mentioned this pull request Jul 23, 2020

IDEs do not detect when newer iOS devices are attached and detached, only show up if attached when launched #62100

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021

		// Run in interactive mode (via script) to convince
		// xcdevice it has a terminal attached and pipe stdout.

Change iOS device discovery from polling to long-running observation #58137

Change iOS device discovery from polling to long-running observation #58137

Uh oh!

Conversation

jmagman commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams left a comment

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!

Uh oh!

jmagman commented May 28, 2020 •

edited

Loading