Let flutter attach find the service port by looking through old logs again (#53153)

dev/devicelab/bin/tasks/flutter_attach_test_android.dart

@@ -57,10 +57,14 @@ Future<void> testReload(Process process, { Future<void> Function() onListening }
   }
 
   await eventOrExit(listening.future);
-  await eventOrExit(ready.future);
+  await eventOrExit(ready.future).timeout(const Duration(seconds: 5), onTimeout: () {
+    // If it can't attach in 5 seconds, it's not capable of finding the
+    // observatory URL in the logs.
+    throw TaskResult.failure('Failed to attach to running Flutter process');
+  });
 
   if (exitCode != null)
-    throw 'Failed to attach to test app; command unexpected exited, with exit code $exitCode.';
+    throw TaskResult.failure('Failed to attach to test app; command unexpected exited, with exit code $exitCode.');
 
   process.stdin.write('r');
   process.stdin.flush();
@@ -75,10 +79,10 @@ Future<void> testReload(Process process, { Future<void> Function() onListening }
   await process.exitCode;
 
   if (stderr.isNotEmpty)
-    throw 'flutter attach had output on standard error.';
+    throw TaskResult.failure('flutter attach had output on standard error.');
 
   if (exitCode != 0)
-    throw 'exit code was not 0';
+    throw TaskResult.failure('exit code was not 0');
 }
 
 void main() {
@@ -120,7 +124,7 @@ void main() {
         // After the delay, force-stopping it shouldn't do anything, but doesn't hurt.
         await device.shellExec('am', <String>['force-stop', kAppId]);
 
-        final String currentTime = (await device.shellEval('date', <String>['"+%F %R:%S.000"'])).trim();
+        String currentTime = (await device.shellEval('date', <String>['"+%F %R:%S.000"'])).trim();
         print('Start time on device: $currentTime');
         section('Relaunching application');
         await device.shellExec('am', <String>['start', '-n', kActivityId]);
@@ -140,6 +144,26 @@ void main() {
         );
         await testReload(attachProcess);
 
+        // Give the device the time to really shut down the app.
+        await Future<void>.delayed(const Duration(milliseconds: 200));
+        // After the delay, force-stopping it shouldn't do anything, but doesn't hurt.
+        await device.shellExec('am', <String>['force-stop', kAppId]);
+
+        section('Attaching after relaunching application');
+        await device.shellExec('am', <String>['start', '-n', kActivityId]);
+
+        // Let the application launch. Sync to the next time an observatory is ready.
+        currentTime = (await device.shellEval('date', <String>['"+%F %R:%S.000"'])).trim();
+        await device.adb(<String>['logcat', '-e', 'Observatory listening on http:', '-m', '1', '-T', currentTime]);
+
+        // Attach again now that the VM is already running.
+        attachProcess = await startProcess(
+          path.join(flutterDirectory.path, 'bin', 'flutter'),
+          <String>['--suppress-analytics', 'attach', '-d', device.deviceId],
+          isBot: false, // we just want to test the output, not have any debugging info
+        );
+        // Verify that it can discover the observatory port from past logs.
+        await testReload(attachProcess);
       } finally {
         section('Uninstalling');
         await device.adb(<String>['uninstall', kAppId]);

packages/flutter_tools/doc/attach.md

@@ -9,19 +9,17 @@ without `flutter run` and provides a HotRunner (enabling hot reload/restart).
 
 There are four ways for the attach command to discover a running app:
 
-1. If the app is already running and the observatory port is known, it can be
-explicitly provided to attach via the command-line, e.g. `$ flutter attach
---debug-port 12345`
-1. If the app is already running and the platform is iOS, attach can use mDNS
-to lookup the observatory port via the application ID, with just `$ flutter
-attach`
 1. If the platform is Fuchsia the module name must be provided, e.g. `$
 flutter attach --module=mod_name`. This can be called either before or after
 the application is started, attach will poll the device if it cannot
 immediately discover the port
-1. On other platforms (i.e. Android), if the app is not yet running attach
-will listen and wait for the app to be (manually) started with the default
-command: `$ flutter attach`
+1. On Android and iOS, just running `flutter attach` suffices. Flutter tools
+will search for an already running Flutter app or module if available.
+Otherwise, the tool will wait for the next Flutter app or module to launch
+before attaching.
+1. If the app or module is already running and the specific observatory port is
+known, it can be explicitly provided to attach via the command-line, e.g.
+`$ flutter attach --debug-port 12345`
 
 ## Source
 

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

@@ -213,6 +213,7 @@ class AndroidDevice extends Device {
   Future<String> get apiVersion => _getProperty('ro.build.version.sdk');
 
   AdbLogReader _logReader;
+  AdbLogReader _pastLogReader;
   _AndroidDevicePortForwarder _portForwarder;
 
   List<String> adbCommandForDevice(List<String> args) {
@@ -673,9 +674,23 @@ class AndroidDevice extends Device {
   }
 
   @override
-  FutureOr<DeviceLogReader> getLogReader({ AndroidApk app }) async {
-    // The Android log reader isn't app-specific.
-    return _logReader ??= await AdbLogReader.createLogReader(this, globals.processManager);
+  FutureOr<DeviceLogReader> getLogReader({
+    AndroidApk app,
+    bool includePastLogs = false,
+  }) async {
+    // The Android log reader isn't app-specific. The `app` parameter isn't used.
+    if (includePastLogs) {
+      return _pastLogReader ??= await AdbLogReader.createLogReader(
+        this,
+        globals.processManager,
+        includePastLogs: true,
+      );
+    } else {
+      return _logReader ??= await AdbLogReader.createLogReader(
+        this,
+        globals.processManager,
+      );
+    }
   }
 
   @override
@@ -724,6 +739,7 @@ class AndroidDevice extends Device {
   @override
   Future<void> dispose() async {
     _logReader?._stop();
+    _pastLogReader?._stop();
     await _portForwarder?.dispose();
   }
 }
@@ -901,6 +917,9 @@ class AdbLogReader extends DeviceLogReader {
   static Future<AdbLogReader> createLogReader(
     AndroidDevice device,
     ProcessManager processManager,
+    {
+      bool includePastLogs = false,
+    }
   ) async {
     // logcat -T is not supported on Android releases before Lollipop.
     const int kLollipopVersionCode = 21;
@@ -915,7 +934,12 @@ class AdbLogReader extends DeviceLogReader {
       'logcat',
       '-v',
       'time',
-      if (apiVersion != null && apiVersion >= kLollipopVersionCode) ...<String>[
+      // If we include logs from the past, filter for 'flutter' logs only.
+      if (includePastLogs) ...<String>[
+        '-s',
+        'flutter',
+      ] else if (apiVersion != null && apiVersion >= kLollipopVersionCode) ...<String>[
+        // Otherwise, filter for logs appearing past the present.
         // Empty `-T` means the timestamp of the logcat command invocation.
         '-T',
         device.lastLogcatTimestamp ?? '',

packages/flutter_tools/lib/src/commands/attach.dart

@@ -6,6 +6,7 @@ import 'dart:async';
 
 import 'package:meta/meta.dart';
 
+import '../android/android_device.dart';
 import '../artifacts.dart';
 import '../base/common.dart';
 import '../base/context.dart';
@@ -245,7 +246,9 @@ class AttachCommand extends FlutterCommand {
       if (observatoryUri == null) {
         final ProtocolDiscovery observatoryDiscovery =
           ProtocolDiscovery.observatory(
-            await device.getLogReader(),
+            // If it's an Android device, attaching relies on past log searching
+            // to find the service protocol.
+            await device.getLogReader(includePastLogs: device is AndroidDevice),
             portForwarder: device.portForwarder,
             ipv6: ipv6,
             devicePort: deviceVmservicePort,

packages/flutter_tools/lib/src/desktop_device.dart

@@ -63,7 +63,11 @@ abstract class DesktopDevice extends Device {
   Future<String> get sdkNameAndVersion async => globals.os.name;
 
   @override
-  DeviceLogReader getLogReader({ ApplicationPackage app }) {
+  DeviceLogReader getLogReader({
+    ApplicationPackage app,
+    bool includePastLogs = false,
+  }) {
+    assert(!includePastLogs, 'Past log reading not supported on desktop.');
     return _deviceLogReader;
   }
 

packages/flutter_tools/lib/src/device.dart

@@ -415,9 +415,17 @@ abstract class Device {
   Future<String> get sdkNameAndVersion;
 
   /// Get a log reader for this device.
-  /// If [app] is specified, this will return a log reader specific to that
+  ///
+  /// If `app` is specified, this will return a log reader specific to that
   /// application. Otherwise, a global log reader will be returned.
-  FutureOr<DeviceLogReader> getLogReader({ covariant ApplicationPackage app });
+  ///
+  /// If `includePastLogs` is true and the device type supports it, the log
+  /// reader will also include log messages from before the invocation time.
+  /// Defaults to false.
+  FutureOr<DeviceLogReader> getLogReader({
+    covariant ApplicationPackage app,
+    bool includePastLogs = false,
+  });
 
   /// Get the port forwarder for this device.
   DevicePortForwarder get portForwarder;

packages/flutter_tools/lib/src/fuchsia/fuchsia_device.dart

@@ -508,8 +508,13 @@ class FuchsiaDevice extends Device {
   }
 
   @override
-  DeviceLogReader getLogReader({ApplicationPackage app}) =>
-      _logReader ??= _FuchsiaLogReader(this, app);
+  DeviceLogReader getLogReader({
+    ApplicationPackage app,
+    bool includePastLogs = false,
+  }) {
+    assert(!includePastLogs, 'Past log reading not supported on Fuchsia.');
+    return _logReader ??= _FuchsiaLogReader(this, app);
+  }
   _FuchsiaLogReader _logReader;
 
   @override

packages/flutter_tools/lib/src/ios/devices.dart

@@ -372,7 +372,11 @@ class IOSDevice extends Device {
   Future<String> get sdkNameAndVersion async => 'iOS $_sdkVersion';
 
   @override
-  DeviceLogReader getLogReader({ IOSApp app }) {
+  DeviceLogReader getLogReader({
+    IOSApp app,
+    bool includePastLogs = false,
+  }) {
+    assert(!includePastLogs, 'Past log reading not supported on iOS devices.');
     _logReaders ??= <IOSApp, DeviceLogReader>{};
     return _logReaders.putIfAbsent(app, () => IOSDeviceLogReader.create(
       device: this,

packages/flutter_tools/lib/src/ios/simulators.dart

@@ -519,8 +519,12 @@ class IOSSimulator extends Device {
   }
 
   @override
-  DeviceLogReader getLogReader({ covariant IOSApp app }) {
+  DeviceLogReader getLogReader({
+    covariant IOSApp app,
+    bool includePastLogs = false,
+  }) {
     assert(app is IOSApp);
+    assert(!includePastLogs, 'Past log reading not supported on iOS simulators.');
     _logReaders ??= <ApplicationPackage, _IOSSimulatorLogReader>{};
     return _logReaders.putIfAbsent(app, () => _IOSSimulatorLogReader(this, app));
   }

packages/flutter_tools/lib/src/tester/flutter_tester.dart

@@ -79,7 +79,12 @@ class FlutterTesterDevice extends Device {
       _FlutterTesterDeviceLogReader();
 
   @override
-  DeviceLogReader getLogReader({ ApplicationPackage app }) => _logReader;
+  DeviceLogReader getLogReader({
+    ApplicationPackage app,
+    bool includePastLogs = false,
+  }) {
+    return _logReader;
+  }
 
   @override
   Future<bool> installApp(ApplicationPackage app) async => true;

packages/flutter_tools/lib/src/web/web_device.dart

@@ -60,7 +60,10 @@ class ChromeDevice extends Device {
   DeviceLogReader _logReader;
 
   @override
-  DeviceLogReader getLogReader({ApplicationPackage app}) {
+  DeviceLogReader getLogReader({
+    ApplicationPackage app,
+    bool includePastLogs = false,
+  }) {
     return _logReader ??= NoOpDeviceLogReader(app?.name);
   }
 
@@ -221,7 +224,10 @@ class WebServerDevice extends Device {
   DeviceLogReader _logReader;
 
   @override
-  DeviceLogReader getLogReader({ApplicationPackage app}) {
+  DeviceLogReader getLogReader({
+    ApplicationPackage app,
+    bool includePastLogs = false,
+  }) {
     return _logReader ??= NoOpDeviceLogReader(app?.name);
   }
 

packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart

@@ -97,7 +97,8 @@ void main() {
       });
 
       testUsingContext('finds observatory port and forwards', () async {
-        when(device.getLogReader()).thenAnswer((_) {
+        when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
+          .thenAnswer((_) {
             // Now that the reader is used, start writing messages to it.
             mockLogReader.addLine('Foo');
             mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
@@ -134,7 +135,9 @@ void main() {
           ..writeAsStringSync('{}');
 
         when(device.name).thenReturn('MockAndroidDevice');
-        when(device.getLogReader()).thenReturn(mockLogReader);
+        when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
+
+          .thenReturn(mockLogReader);
 
           final Process dartProcess = MockProcess();
           final StreamController<List<int>> compilerStdoutController = StreamController<List<int>>();
@@ -232,7 +235,8 @@ void main() {
       });
 
       testUsingContext('Fails with tool exit on bad Observatory uri', () async {
-        when(device.getLogReader()).thenAnswer((_) {
+        when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
+          .thenAnswer((_) {
             // Now that the reader is used, start writing messages to it.
             mockLogReader.addLine('Foo');
             mockLogReader.addLine('Observatory listening on http:/:/127.0.0.1:$devicePort');
@@ -249,7 +253,8 @@ void main() {
       });
 
       testUsingContext('accepts filesystem parameters', () async {
-        when(device.getLogReader()).thenAnswer((_) {
+        when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
+          .thenAnswer((_) {
             // Now that the reader is used, start writing messages to it.
             mockLogReader.addLine('Foo');
             mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
@@ -328,7 +333,8 @@ void main() {
       });
 
       testUsingContext('exits when ipv6 is specified and debug-port is not', () async {
-        when(device.getLogReader()).thenAnswer((_) {
+        when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
+          .thenAnswer((_) {
             // Now that the reader is used, start writing messages to it.
             mockLogReader.addLine('Foo');
             mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
@@ -350,7 +356,8 @@ void main() {
       },);
 
       testUsingContext('exits when observatory-port is specified and debug-port is not', () async {
-        when(device.getLogReader()).thenAnswer((_) {
+        when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
+          .thenAnswer((_) {
             // Now that the reader is used, start writing messages to it.
             mockLogReader.addLine('Foo');
             mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
@@ -402,7 +409,7 @@ void main() {
       when(mockHotRunner.isWaitingForObservatory).thenReturn(false);
 
       testDeviceManager.addDevice(device);
-      when(device.getLogReader())
+      when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
         .thenAnswer((_) {
           // Now that the reader is used, start writing messages to it.
           mockLogReader.addLine('Foo');
@@ -441,7 +448,8 @@ void main() {
       final MockHotRunner mockHotRunner = MockHotRunner();
       final MockHotRunnerFactory mockHotRunnerFactory = MockHotRunnerFactory();
       when(device.portForwarder).thenReturn(portForwarder);
-      when(device.getLogReader()).thenAnswer((_) => mockLogReader);
+      when(device.getLogReader(includePastLogs: anyNamed('includePastLogs')))
+        .thenAnswer((_) => mockLogReader);
       when(portForwarder.forward(devicePort, hostPort: anyNamed('hostPort')))
         .thenAnswer((_) async => hostPort);
       when(portForwarder.forwardedPorts)

packages/flutter_tools/test/commands.shard/hermetic/run_test.dart

@@ -633,7 +633,10 @@ class FakeDevice extends Fake implements Device {
   Future<String> get sdkNameAndVersion => Future<String>.value('');
 
   @override
-  DeviceLogReader getLogReader({ ApplicationPackage app }) {
+  DeviceLogReader getLogReader({
+    ApplicationPackage app,
+    bool includePastLogs = false,
+  }) {
     return FakeDeviceLogReader();
   }
 

packages/flutter_tools/test/general.shard/android/adb_log_reader_test.dart

@@ -79,6 +79,30 @@ void main() {
     expect(processManager.hasRemainingExpectations, false);
   });
 
+  testWithoutContext('AdbLogReader calls adb logcat with expected flags when requesting past logs', () async {
+    final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+      const FakeCommand(
+        command: <String>[
+          'adb',
+          '-s',
+          '1234',
+          'logcat',
+          '-v',
+          'time',
+          '-s',
+          'flutter',
+        ],
+      )
+    ]);
+    await AdbLogReader.createLogReader(
+      createMockDevice(null),
+      processManager,
+      includePastLogs: true,
+    );
+
+    expect(processManager.hasRemainingExpectations, false);
+  });
+
   testWithoutContext('AdbLogReader handles process early exit', () async {
     final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
       FakeCommand(

packages/flutter_tools/test/general.shard/android/android_device_test.dart

@@ -468,17 +468,48 @@ flutter:
     });
   });
 
-  group('lastLogcatTimestamp', () {
+  group('logcat', () {
     final ProcessManager mockProcessManager = MockProcessManager();
     final AndroidDevice device = AndroidDevice('1234');
 
-    testUsingContext('returns null if shell command failed', () async {
+    testUsingContext('lastLogcatTimestamp returns null if shell command failed', () async {
       when(mockProcessManager.runSync(argThat(contains('logcat'))))
           .thenReturn(ProcessResult(0, 1, '', ''));
       expect(device.lastLogcatTimestamp, isNull);
     }, overrides: <Type, Generator>{
       ProcessManager: () => mockProcessManager,
     });
+
+    testUsingContext('AdbLogReaders for past+future and future logs are not the same', () async {
+      when(mockProcessManager.run(
+        argThat(contains('getprop')),
+        stderrEncoding: anyNamed('stderrEncoding'),
+        stdoutEncoding: anyNamed('stdoutEncoding'),
+      )).thenAnswer((_) {
+        final StringBuffer buf = StringBuffer()
+          ..writeln('[ro.build.version.sdk]: [23]');
+        final ProcessResult result = ProcessResult(1, exitCode, buf.toString(), '');
+        return Future<ProcessResult>.value(result);
+      });
+      when(mockProcessManager.run(
+        argThat(contains('shell')),
+        stderrEncoding: anyNamed('stderrEncoding'),
+        stdoutEncoding: anyNamed('stdoutEncoding'),
+      )).thenAnswer((_) {
+        final StringBuffer buf = StringBuffer()
+          ..writeln('11-27 15:39:04.506');
+        final ProcessResult result = ProcessResult(1, exitCode, buf.toString(), '');
+        return Future<ProcessResult>.value(result);
+      });
+      final DeviceLogReader pastLogReader = await device.getLogReader(includePastLogs: true);
+      final DeviceLogReader defaultLogReader = await device.getLogReader();
+      expect(pastLogReader, isNot(equals(defaultLogReader)));
+      // Getting again is cached.
+      expect(pastLogReader, equals(await device.getLogReader(includePastLogs: true)));
+      expect(defaultLogReader, equals(await device.getLogReader()));
+    }, overrides: <Type, Generator>{
+      ProcessManager: () => mockProcessManager,
+    });
   });
 
   test('Can parse adb shell dumpsys info', () {