Detect exact device ID matches quickly #62070
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
75929f7 to
63b054c
Compare
|
Very nice! I think you could construct a new test case that will now pass that would have failed. Consider two device discoverers, one returns instantly and one never returns. |
Done. |
| ]; | ||
|
|
||
| // Wait for an exact match, or for all discoverers to return results. | ||
| await Future.any<dynamic>(<Future<dynamic>>[ |
There was a problem hiding this comment.
The edge case Zach mentioned is a bit more obvious when streams aren't used. In this case, after Future.any completes the first future, the second future may still complete w/error. That would cause an unhandled exception to be thrown into the zone.
You could simulate this a similar 2 discoverer setup where one throws an error after another turn.
There was a problem hiding this comment.
Done, added test for this as well.
| return null; | ||
| }, onError: (dynamic error, StackTrace stackTrace) { | ||
| // Return matches from other discoverers even if one fails. | ||
| globals.printTrace('Ignored error discovering $deviceId: $error'); |
There was a problem hiding this comment.
I think this is almost the behavior we want, thought it's not quite the same as before. Previously if a discoverer crashed, we would surface that and go straight to crash logging. Especially if we don't match an exact device, knowing that some discovery has crashed is probably important, but I don't think that it should stop users from running on unrelated devices.
Unsure what the right solution is.
There was a problem hiding this comment.
Maybe if there is any error, and the target device isn't found, print an error directing the user to flutter devices or flutter doctor --verbose.
There was a problem hiding this comment.
I was happy with the old crashing behavior so we'd at least see it, but I'm still missing something related to why it matters if it's unhandled and thrown into the zone.
That would cause an unhandled exception to be thrown into the zone.
There was a problem hiding this comment.
I was actually wrong about this. Future.any will explicitly discard errors thrown by future's other than the first. I think what you have here is good
Description
When a command is run with
--deviceall relevant platform device discoverers gather all devices, which are then checked if the identifier or name exactly matches the specified device ID, then if they match the prefix of the specified device ID. Some device discoverers with hard-coded return very quickly (flutter-tester,chrome,macos,linux), and others that shell out to underlying tools (Android, iOS, Fuschia) take much longer.Instead of gathering all devices first, process them as they are discovered, and return quickly if an exact match is found.
Here are discovery times for exact matches on top of tree vs this PR (note these times include the overhead of calling a command I hacked the timing into, it's not how long this function took):
Before this change, all times were about the same, because it waited for all platform discoveries to be complete before matching. After this change, the time takes as long as the relevant discoverer can return.
Related Issues
Fixes #61466
Fixes #44898
Relevant for #15072 increasing discovery times when an exact networking iOS ID is specified without regressing other platforms.
Tests
getDeviceById exact matcheruses newLongPollingDeviceDiscoverythat never finishes polling. Exact matches are still returned quickly. This test runs forever on master.Checklist
///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change