-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Better support for wireless devices in IDEs #123716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better support for wireless devices in IDEs #123716
Conversation
| }).whenComplete(() async { | ||
| if (_deviceIdentifierByEvent?.hasListener ?? false) { | ||
|
|
||
| unawaited(Future.wait(<Future<void>>[ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
flutter/packages/flutter_tools/lib/src/macos/xcdevice.dart
Lines 250 to 253 in 55f5d05
| 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
flutter/packages/flutter_tools/lib/src/base/process.dart
Lines 488 to 501 in 2144a1b
| // 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?
There was a problem hiding this comment.
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/stderrto be fully processed and then cancels the subscriptions.
Has "fully processed" means the process has already exited though?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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();
}));There was a problem hiding this comment.
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:
flutter/packages/flutter_tools/lib/src/base/process.dart
Lines 488 to 493 in 2144a1b
| // 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
_processUtils.streamwaits 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 🙂
…nts, rename variable
jmagman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Unfortunately you're hitting #124128 |
Oof, I'll try re-running it |
Better support for wireless devices in IDEs
Updates polling for iOS devices to use a combination of the following commands to determine if a device is connected:
xcdevice listxcdevice observe --wifixcdevice observe --usbA device is considered connected if it does not have any
observeevents and was found as connected inxcdevice listor it hadobserveevents that indicate it's connected via one or more interfaces.Fixes #118113.
Part 5 in breakdown of #121262.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.