Allow adb stdout to contain the port number without failing by DanTup · Pull Request #31491 · flutter/flutter

DanTup · 2019-04-23T16:16:03Z

While testing on CrOS I found some strange behaviour. When running Flutter with --observatory-port xxxx it fails like this:

$ flutter run --observatory-port 9006

// ...
Built build/app/outputs/apk/debug/app-debug.apk.
Error waiting for a debug connection: ProcessException: adb returned error:
9006

 Command: /home/danny/Apps/Android/platform-tools/adb -s 100.115.92.2:5555 forward tcp:9006 tcp:33803
Error launching application on Google Pixelbook.

The reason it fails, is that our code assumes that when we call adb forward that stdout is empty if we supplied a port but that it contains the port number if we used port=0. On ChromeOS, the port is written to stdout even when we specify it.

It seems strange that this behaves differently on CrOS, so I did some testing. And it got stranger. If I run adb forward from the terminal on macOS I see the same behaviour as CrOS - it prints the port out. However, it only does it the first time (not if it's already forwarded). I can pick any random ports and then run the forward command 10 times, and it's consistent - port printed the first time, but never after:

It seems like there may be some logic to this, but what I cannot explain is why when Flutter runs adb forward on macOS it never sees the stdout with the port number. I added logging and it never gets a port number:

[  +10 ms]       command: /Users/dantup/Library/Android/sdk/platform-tools/adb -s ZY2224Z9N9 forward tcp:8080 tcp:44201
                 exitCode: 0
                 stdout: 
                 stderr: 

[        ] stdout was empty!

Yet running the exact same command (with a new port number, since that one was now forwarded) I do see the port number:

$ /Users/dantup/Library/Android/sdk/platform-tools/adb -s ZY2224Z9N9 forward tcp:8085 tcp:43000
8085

So, this change tolerates this, by allowing stdout to be exactly the port number. It will no longer throw. This seems like a sensible change, even though I'm sad I can't figure out why macOS isn't crashing all over the place too.

FWIW, in my testing the ADB versions on both machines are:

Android Debug Bridge version 1.0.40
Version 28.0.2-5303910
Installed as /Users/dantup/Library/Android/sdk/platform-tools/adb

devoncarew · 2019-04-23T16:41:11Z

packages/flutter_tools/lib/src/android/android_device.dart

I'm curious why this else branch exists - at this point we've already gotten a valid hostPort. We just chose to fail if we then receive any stdout from adb?

We could instead just ignore stdout output.

I don't know for sure but I guess we're assuming if we get some unexpected output, it's could be a failure message (some tools write to stdout) and it's better to fail and review what's happened than assume that the port was mapping ok (which could result in harder to track down connection failures later in the process).

I don't see any mention of the stdout behavior in the adb docs. If this check is only based on observations, than updating it for new observations seem perfectly reasonable.

Yeah, I couldn't find any docs describing this (even casually in examples), so my guess is that it was based on how it seemed to work.

I still can't get my head around why Flutter doesn't get the stdout on macOS though, specifically in this case. I did try reading the ADB source code, but it's written in a language I don't speak.

The expected failure here is that you've specified an explicit port that's already used, so it can't be opened by adb.

jacob314 · 2019-04-23T17:05:24Z

@jonahwilliams do you have any thoughts? It seems like we are just being too strict about stdout output but maybe we are missing something.

jonahwilliams · 2019-04-23T17:17:22Z

It looks like these checks were added in 7750872 with the removal of the port scanner. cc @Hixie

It's not clear to me if the stdout behavior is based on observations for them, or on other adb documentation.

Hixie · 2019-04-24T19:57:25Z

packages/flutter_tools/lib/src/android/android_device.dart

I'm totally fine with this but we should phrase this as if it's by the whole team, not as if it's by one individual in the team. If someone really wants to track down who wrote it, they'll use git blame.

Hixie · 2019-04-24T19:57:39Z

Test?

DanTup · 2019-04-25T09:15:30Z

I've tweaked the comment and added tests that verify the behaviour for the various combinations of input hostPort and output stdout that we know about (including that any stdout that is not exactly the hostPort (if it was provided) still throws).

DanTup · 2019-04-25T09:17:25Z

@fkorotkov just got this on the Windows bot:

https://cirrus-ci.com/task/5213516968493056

I'll restart it - hopefully that doesn't stop you from being able to get logs if you need to investigate.

DanTup · 2019-04-26T18:09:36Z

I've rebased this as it had conflicts due to other new tests in the same file.

LMK if anything else is required - this is a blocker for running Flutter on ChromeOS and being able to use DevTools from the built-in browser.

jonahwilliams

LGTM

googlebot added the cla: yes label Apr 23, 2019

devoncarew reviewed Apr 23, 2019

View reviewed changes

jacob314 requested a review from jonahwilliams April 23, 2019 17:04

dnfield added tool Affects the "flutter" command-line tool. See also t: labels. platform-android Android applications specifically labels Apr 23, 2019

Hixie reviewed Apr 24, 2019

View reviewed changes

DanTup and others added 4 commits April 26, 2019 18:56

Allow adb stdout to contain the port number without failing

03ce481

Tweak comment

d22e739

Tweak comment

c08eaab

Add tests that port forwarder correctly responds to known ADB output

f9be794

jonahwilliams approved these changes Apr 26, 2019

View reviewed changes

DanTup merged commit fdcc8aa into flutter:master Apr 26, 2019

DanTup deleted the cros-adb-fix branch April 26, 2019 20:03

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

Conversation

DanTup commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devoncarew Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

DanTup Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

DanTup Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

Hixie Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

jacob314 commented Apr 23, 2019

Uh oh!

jonahwilliams commented Apr 23, 2019

Uh oh!

Hixie Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

Hixie commented Apr 24, 2019

Uh oh!

DanTup commented Apr 25, 2019

Uh oh!

DanTup commented Apr 25, 2019

Uh oh!

DanTup commented Apr 26, 2019

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DanTup commented Apr 23, 2019 •

edited

Loading