[flutter_tools] iOS fallback discovery protocol by jonahwilliams · Pull Request #49735 · flutter/flutter

jonahwilliams · 2020-01-29T21:34:52Z

Description

A protocol for discovery of a vmservice on an attached iOS device with multiple fallbacks.

On versions of iOS 13 and greater, libimobiledevice can no longer listen to logs directly. The only way to discover an active observatory is through the mDNS protocol. However, there are a number of circumstances where this breaks down, such as when the device is connected to certain wifi networks or with certain hotspot connections enabled.

Another approach to discover a vmservice is to attempt to assign a specific port and then attempt to connect. This may fail if the port is not available. This port value should be either random, or otherwise generated with application specific input. This reduces the chance of accidentally connecting to another running flutter application.

Finally, if neither of the above approaches works, we can still attempt to parse logs.

To improve the overall resilience of the process, this class combines the three discovery strategies. First it assigns a port and attempts to connect. Then if this fails it falls back to mDNS, then finally attempting to scan logs.

Fixes #46724

WIP because I still need to test the port selection logic and wait for the flag/engine to roll. Also I'm not yet sure if the port forwarder will throw, or if I need to ping the observatory

jonahwilliams · 2020-01-29T21:37:28Z

+    try {
+      // TODO(jonahwilliams): determine if this succeeds even if there is nothing
+      // listening.
+      final int hostPort = await _portForwarder.forward(assumedDevicePort, hostPort: hostVmservicePort);


If this still succeeds when there is no vmservice, I'll update it to make an HTTP get to grab version info or something safe

Actually this definitely won't throw. Instead I'll need to poll w/ exponential backoff for a while. Need to run some experiments to determine the best numbers

jonahwilliams · 2020-01-29T21:37:56Z

Adding @jmagman @christopherfujino for initial thoughts

jonahwilliams · 2020-01-30T23:54:35Z

Waiting on an engine roll to start working on this again

…_discovery

jonahwilliams · 2020-02-03T16:16:15Z

engine roll has landed and this is working locally

jonahwilliams · 2020-02-03T16:47:05Z

I spoke too soon, the flag is not yet in flutter/flutter

zanderso · 2020-02-03T17:00:27Z

+    // Step 2.5: Generate a potential open port using the provided argument,
+    // or randomly with the package name as a seed.
+    final int assumedObservatoryPort = debuggingOptions?.deviceVmServicePort
+      ?? math.Random(packageId.hashCode).nextInt(1000) + 6000;


We should probably pick a random port in the standard ephemeral port range 49152 to 65535 (2^15+2^14 to 2^16−1)

https://en.wikipedia.org/wiki/Ephemeral_port

zanderso · 2020-02-03T17:04:14Z

+        final HttpClientResponse response = await request.close();
+        if (response.statusCode == HttpStatus.ok) {
+          final String responseBody = await response.transform(utf8.decoder).join('');
+          if (responseBody.contains('Dart')) {


As discussed offline, this is probably a bit fragile, and we should instead connect to the vmservice to check for an isolate with the right name or root library uri.

zanderso · 2020-02-03T17:10:28Z

+        // response page containing a specific string. This logic needs to
+        // be tightened up to ensure we don't accidentally try to connect
+        // to some arbitrary server.
+        final HttpClientRequest request = await _httpClient.getUrl(assumedUri);


The body of this loop could be pulled out to a helper function.

zanderso · 2020-02-03T17:11:29Z

+        attempts += 1;
+      }
+    } on Exception {
+      _logger.printTrace('Failed to connect directly, falling back to mDNS');


It might be helpful to print the exception object here, too.

zanderso · 2020-02-03T17:12:34Z

+    } on Exception {
+      _logger.printTrace('Failed to connect directly, falling back to mDNS');
+    }
+    UsageEvent('ios-mdns', 'precheck-failure').send();


Consider creating a new event type in events.dart. It might also be interesting to record the port number that we tried for here in case there's one that fails a lot.

I created ios-handshake as an event type. I don't think it would be worth it to record the port, since we're effectively choosing a random number, we'd need a tremendous amount of data to get any insight into the failures.

zanderso · 2020-02-03T17:13:58Z

+        return result;
+      }
+    } on Exception {
+      _logger.printTrace('Failed to connect with mDNS, falling back to log scanning');


…_discovery

…tter into ios_Fallback_discovery

jonahwilliams · 2020-02-05T20:36:25Z

This requires flutter-team-archive/engine@7e1d144 to roll into the framework to land - tested locally to verify that the fallback works as expected when multiple apps are run with the same specified port

jonahwilliams · 2020-02-05T20:38:13Z

That commit is included in #50177

zanderso

lgtm w/ nits and question

zanderso · 2020-02-05T21:09:24Z

+        }
+      } on Exception {
+        // No action, we might have failed to connect.
+        // The error message will be irrelevant.


You might record one of the exception objects to print with the failure trace message.

zanderso · 2020-02-05T21:11:19Z

+        hostVmservicePort: hostVmservicePort,
+      );
+      if (result != null) {
+        UsageEvent('ios-mdns', 'success').send();


It'll make it a little easier to view this data in analytics if first argument for all of the events is the same. So maybe make this:

UsageEvent('ios-handshake', 'mdns-success').send();

and similarly below.

Are you worried about changing the existing event names?

No. We're the only ones looking at those events in GA, and they'll kind of mean something different now anyway since the handshake procedure is different, so may as well make the naming uniform.

Done. I kept the first set of events as just success and failure, since I wasn't sure what else to prefix them with

zanderso · 2020-02-05T21:13:22Z

+      .thenAnswer((Invocation invocation) async => 1);
+  });
+
+  testUsingContext('Selects assumed port if VM service connection is successful', () async {


Sorry if I already asked this, but can these be testWithoutContext() ?

I think you mentioned it and I completely forgot

Ahh, no it was the Usage/event classes need to be refactored. There doesn't seem to be a way to reference any of the helpers without going through the context

Ah, okay. I'll take on the Usage refactor when I'm back in the office.

zanderso

still lgtm

…_discovery

mehmetf · 2020-02-07T14:58:03Z

Jonah, do you imagine this will require a change in google3? or is it expected to work as is?

@jamesderlin FYI.

jonahwilliams · 2020-02-07T17:13:19Z

I expect this to work as is

[flutter_tools] iOS fallback discovery protocol

f592386

fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Jan 29, 2020

googlebot added the cla: yes label Jan 29, 2020

fix typos

3b552cb

jonahwilliams commented Jan 29, 2020

View reviewed changes

jonahwilliams added 3 commits February 1, 2020 12:13

Merge branch 'master' of github.com:flutter/flutter into ios_Fallback…

d89ffe1

…_discovery

attempt to connect to observatory through forwarded port

a3d110a

but now it really works

437a4de

jonahwilliams marked this pull request as ready for review February 3, 2020 16:16

jonahwilliams requested a review from zanderso February 3, 2020 16:36

jonahwilliams removed the work in progress; do not review label Feb 3, 2020

zanderso reviewed Feb 3, 2020

View reviewed changes

jonahwilliams added 5 commits February 4, 2020 08:57

Merge branch 'master' of github.com:flutter/flutter into ios_Fallback…

18a2df7

…_discovery

connect to actual vmservice

37c3557

Merge branch 'master' of github.com:flutter/flutter into ios_Fallback…

fe7d1ce

…_discovery

Merge branch 'ios_Fallback_discovery' of github.com:jonahwilliams/flu…

4d15f79

…tter into ios_Fallback_discovery

make tests work

169830d

jonahwilliams requested a review from zanderso February 5, 2020 20:38

zanderso approved these changes Feb 5, 2020

View reviewed changes

jonahwilliams added 2 commits February 5, 2020 14:18

log exception event

e19c1bc

update event names

dbe83c6

zanderso approved these changes Feb 5, 2020

View reviewed changes

Merge branch 'master' into ios_Fallback_discovery

3ddb1d7

jonahwilliams added 2 commits February 6, 2020 12:42

Merge branch 'master' of github.com:flutter/flutter into ios_Fallback…

614067e

…_discovery

remove unused import

5c13fbb

jonahwilliams added the waiting for tree to go green label Feb 6, 2020

fluttergithubbot merged commit 69ecca5 into flutter:master Feb 6, 2020

jonahwilliams deleted the ios_Fallback_discovery branch February 10, 2020 17:49

jmagman mentioned this pull request Feb 11, 2020

Tool crash: type 'Sentinel' is not a subtype of type 'Isolate' in type cast #50610

Closed

This was referenced Feb 27, 2020

Error connecting to the service protocol: failed to connect to http://127.0.0.1:1029/89AGRYqn_pA=/ #48234

Closed

Flutter tool hangs waiting on observatory port when AndroidManifest.xml and MainActivity.kt package name get out of sync #33214

Closed

jmagman mentioned this pull request Sep 15, 2020

flutter drive CI tests fail targeting iOS 14, stuck on local network permission popup #65207

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Conversation

jonahwilliams commented Jan 29, 2020

Description

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!

jonahwilliams commented Jan 29, 2020

Uh oh!

jonahwilliams commented Jan 30, 2020

Uh oh!

jonahwilliams commented Feb 3, 2020

Uh oh!

jonahwilliams commented Feb 3, 2020

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!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Feb 5, 2020

Uh oh!

jonahwilliams commented Feb 5, 2020

Uh oh!

zanderso 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!

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!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

mehmetf commented Feb 7, 2020

Uh oh!

jonahwilliams commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development