Skip to content

Conversation

@vashworth
Copy link
Contributor

@vashworth vashworth commented Mar 29, 2023

Updates polling for iOS devices to use a combination of the following commands to determine if a device is connected:

  • xcdevice list
  • xcdevice observe --wifi
  • xcdevice observe --usb

A device is considered connected if it does not have any observe events and was found as connected in xcdevice list or it had observe events that indicate it's connected via one or more interfaces.

Fixes #118113.

Part 5 in breakdown of #121262.

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.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 29, 2023
@vashworth vashworth marked this pull request as ready for review March 29, 2023 19:04
@vashworth vashworth requested a review from jmagman March 29, 2023 19:04
@vashworth vashworth changed the title Wireless device discovery part 5 Better support for wireless devices in IDEs Mar 29, 2023
}).whenComplete(() async {
if (_deviceIdentifierByEvent?.hasListener ?? false) {

unawaited(Future.wait(<Future<void>>[
Copy link
Member

Choose a reason for hiding this comment

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

What if only one dies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When one dies, it also kills the other

final Future<void> usbProcessExited = _usbDeviceObserveProcess!.exitCode.then((int status) {
_logger.printTrace('xcdevice observe --usb exited with code $exitCode');
// Kill other process in case only one was killed.
_wifiDeviceObserveProcess?.kill();

// Run in interactive mode (via script) to convince
// xcdevice it has a terminal attached in order to redirect stdout.
_deviceObservationProcess = await _processUtils.start(
_usbDeviceObserveProcess = await _processUtils.start(
Copy link
Member

Choose a reason for hiding this comment

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

I think swapping to _processUtils.start with trace:true and setting up a mapFunction to process the XCDeviceEventNotification would give you the stdout/stderr behavior you want here, and would be cleaner.

It does most of what you need, but unfortunately returns a future to the exitCode and not the streaming process, so you won't be able to kill it as-written. You could add a streamProcess method to ProcessUtils that takes the same params, move all the stream logic into that, and return the process. Then make stream call streamProcess and return process.exitCode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried out _processUtils.stream (assuming that's what you meant), but it's a bit problematic because it waits for the stdout/stderr to be fully processed and then cancels the subscriptions.

// Wait for stdout to be fully processed
// because process.exitCode may complete first causing flaky tests.
await Future.wait<void>(<Future<void>>[
stdoutSubscription.asFuture<void>(),
stderrSubscription.asFuture<void>(),
]);
// The streams as futures have already completed, so waiting for the
// potentially async stream cancellation to complete likely has no benefit.
// Further, some Stream implementations commonly used in tests don't
// complete the Future returned here, which causes tests using
// mocks/FakeAsync to fail when these Futures are awaited.
unawaited(stdoutSubscription.cancel());
unawaited(stderrSubscription.cancel());

I can't think of any great workarounds that maintain the current usage of ProcessUtils.stream and also work for my use case. Perhaps I could make a customized version of ProcessUtils.stream in xcdevice.dart and use that function?

Copy link
Member

Choose a reason for hiding this comment

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

it's a bit problematic because it waits for the stdout/stderr to be fully processed and then cancels the subscriptions.

Has "fully processed" means the process has already exited though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell by testing it yet, it waits for it to exit

Copy link
Member

Choose a reason for hiding this comment

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

From what I can tell by testing it yet, it waits for it to exit

Isn't that what you want then? I don't think you want those subscriptions to continue after the process is done. Your code is doing the same thing in _streamXCDeviceEventCommand.

    unawaited(process.exitCode.whenComplete(() {
      stdoutSubscription.cancel();
      stderrSubscription.cancel();
    }));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right that we don't want those subscriptions to continue after the process is done, which is why I have the unawaited you quoted. The difference, though, is that _streamXCDeviceEventCommand does not wait for the process to exit.

Here's a breakdown of the problem with _processUtils.stream:

So _processUtils.stream waits for the process to exit before returning:

// Wait for stdout to be fully processed
// because process.exitCode may complete first causing flaky tests.
await Future.wait<void>(<Future<void>>[
stdoutSubscription.asFuture<void>(),
stderrSubscription.asFuture<void>(),
]);

So let's say I created _processUtils.streamProcess which returns the Process:

  Future<Process> streamProcess(
    args,
  }) async {
    final Process process = await start(
      args,
    );
    final StreamSubscription<String> stdoutSubscription = process.stdout
      [removed to shorten...]
    final StreamSubscription<String> stderrSubscription = process.stderr
      [removed to shorten...]

    // this is the problem-maker, it will get stuck waiting here
    await Future.wait<void>(<Future<void>>[
      stdoutSubscription.asFuture<void>(),
      stderrSubscription.asFuture<void>(),
    ]);

    unawaited(stdoutSubscription.cancel());
    unawaited(stderrSubscription.cancel());

    return process;
  }

Then, let's say I changed this to use _processUtils.streamProcess

_usbDeviceObserveProcess = await _processUtils.streamProcess( 
   args,
 ); 
  
  // This will not be reached until `_usbDeviceObserveProcess` is exited
 _wifiDeviceObserveProcess = await _processUtils.streamProcess( 
   args,
 ); 

In the above, _wifiDeviceObserveProcess will not be set until the _usbDeviceObserveProcess is exited, which isn't what we want. We want them to run simultaneously. Plus, hypothetically, these processes should run forever and never exit so it prevents it from listening for wifi connections because it's stuck there. Using unawaited to workaround it would also be problematic because _usbDeviceObserveProcess and _wifiDeviceObserveProcess are not actually set until they complete, which means they can't be killed

Copy link
Member

Choose a reason for hiding this comment

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

So _processUtils.stream waits for the process to exit before returning:

Ah, I get it. Thanks for explaining, I'm sure it's really obvious when you're stepping though it 🙂

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM

@jmagman
Copy link
Member

jmagman commented Apr 4, 2023

Unfortunately you're hitting #124128

 06:24 +45 ~3 -2: test/integration.shard/debug_adapter/flutter_adapter_test.dart: launch can run and terminate a Flutter app in noDebug mode [E]                                                        
   Expected: [
               'Launching lib/main.dart on Flutter test device in debug mode...',
               'topLevelFunction',
               'Application finished.',
               '',
               <a string starting with 'Exited'>
             ]
     Actual: [
               'material_color_utilities 0.2.0 (0.3.0 available)',
               'Got dependencies!',
               'Launching lib/main.dart on Flutter test device in debug mode...',
               'topLevelFunction',
               'Application finished.',
               '',
               'Exited.'
             ]
      Which: at location [0] is 'material_color_utilities 0.2.0 (0.3.0 available)' instead of 'Launching lib/main.dart on Flutter test device in debug mode...'
   
   package:matcher                                                       expect
   test/integration.shard/debug_adapter/test_support.dart 49:5           expectLines
   test/integration.shard/debug_adapter/flutter_adapter_test.dart 128:7  main.<fn>.<fn>

@vashworth
Copy link
Contributor Author

vashworth commented Apr 4, 2023

Unfortunately you're hitting #124128

Oof, I'll try re-running it

@vashworth vashworth added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2023
@auto-submit auto-submit bot merged commit 34d2c8d into flutter:master Apr 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2023
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
Better support for wireless devices in IDEs
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@vashworth vashworth deleted the wireless_device_discovery_part_5 branch September 27, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iOS network device not selected when running/debugging in VSCode

2 participants