Unverified Commit b9e8b0a8 authored by Victoria Ashworth's avatar Victoria Ashworth Committed by GitHub

[iOS] Dispose of log readers and port forwarders if launch fails (#127140)

When tests run in our CI using `flutter drive`, if there is a failure it will loop and try again.

https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/drive/drive_service.dart#L177-L186

However, it's using the same `device` instance for each iteration. So what was happening was when it would fail to launch, it would tell its listeners that it was cancelled.
https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/ios_deploy.dart#L486-L489 

Then when the next iteration started, the `vmServiceDiscovery` would immediately return with null because the `deviceLogReader` would be cached from the previous iteration and would already be cancelled. Therefore, bypassing and cancelling the timer.

https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/devices.dart#L585-L591

https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/devices.dart#L627

In addition, it seems like sometimes the stop would fail and therefore the the drain would never get the signal that it was done and therefore would hang forever. There was no indication that the stop had failed though because the logs were going to the stream that had no listeners since `deviceLogReader` was already cancelled.
https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/ios_deploy.dart#L563-L576

Fixes https://github.com/flutter/flutter/issues/127141
parent 077d644b
......@@ -523,6 +523,7 @@ class IOSDevice extends Device {
_logger.printError('Try launching Xcode and selecting "Product > Run" to fix the problem:');
_logger.printError(' open ios/Runner.xcworkspace');
_logger.printError('');
await dispose();
return LaunchResult.failed();
}
......@@ -557,6 +558,7 @@ class IOSDevice extends Device {
final Uri? serviceURL = await vmServiceDiscovery?.uri;
if (serviceURL == null) {
await iosDeployDebugger?.stopAndDumpBacktrace();
await dispose();
return LaunchResult.failed();
}
......@@ -587,12 +589,14 @@ class IOSDevice extends Device {
timer.cancel();
if (localUri == null) {
await iosDeployDebugger?.stopAndDumpBacktrace();
await dispose();
return LaunchResult.failed();
}
return LaunchResult.succeeded(vmServiceUri: localUri);
} on ProcessException catch (e) {
await iosDeployDebugger?.stopAndDumpBacktrace();
_logger.printError(e.message);
await dispose();
return LaunchResult.failed();
} finally {
startAppStatus.stop();
......
......@@ -314,6 +314,7 @@ class IOSDeployDebugger {
// Send signal to stop (pause) the app. Used before a backtrace dump.
static const String _signalStop = 'process signal SIGSTOP';
static const String _signalStopError = 'Failed to send signal 17';
static const String _processResume = 'process continue';
static const String _processInterrupt = 'process interrupt';
......@@ -401,11 +402,23 @@ class IOSDeployDebugger {
}
return;
}
if (line == _signalStop) {
// (lldb) process signal SIGSTOP
// or
// process signal SIGSTOP
if (line.contains(_signalStop)) {
// The app is about to be stopped. Only show in verbose mode.
_logger.printTrace(line);
return;
}
// error: Failed to send signal 17: failed to send signal 17
if (line.contains(_signalStopError)) {
// The stop signal failed, force exit.
exit();
return;
}
if (line == _backTraceAll) {
// The app is stopped and the backtrace for all threads will be printed.
_logger.printTrace(line);
......
......@@ -11,6 +11,7 @@ import 'package:flutter_tools/src/artifacts.dart';
import 'package:flutter_tools/src/base/file_system.dart';
import 'package:flutter_tools/src/base/logger.dart';
import 'package:flutter_tools/src/base/platform.dart';
import 'package:flutter_tools/src/base/process.dart';
import 'package:flutter_tools/src/cache.dart';
import 'package:flutter_tools/src/device.dart';
import 'package:flutter_tools/src/ios/ios_deploy.dart';
......@@ -147,6 +148,28 @@ void main () {
expect(logger.traceText, contains('* thread #1'));
});
testWithoutContext('debugger attached and stop failed', () async {
final StreamController<List<int>> stdin = StreamController<List<int>>();
final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
FakeCommand(
command: const <String>['ios-deploy'],
stdout: '(lldb) run\r\nsuccess\r\nsuccess\r\nprocess signal SIGSTOP\r\n\r\nerror: Failed to send signal 17: failed to send signal 17',
stdin: IOSink(stdin.sink),
),
]);
final IOSDeployDebuggerWaitForExit iosDeployDebugger = IOSDeployDebuggerWaitForExit.test(
processManager: processManager,
logger: logger,
);
expect(iosDeployDebugger.logLines, emitsInOrder(<String>[
'success',
]));
expect(await iosDeployDebugger.launchAndAttach(), isTrue);
await iosDeployDebugger.exitCompleter.future;
});
testWithoutContext('handle processing logging after process exit', () async {
final StreamController<List<int>> stdin = StreamController<List<int>>();
// Make sure we don't hit a race where logging processed after the process exits
......@@ -591,3 +614,37 @@ IOSDeploy setUpIOSDeploy(ProcessManager processManager, {
cache: cache,
);
}
class IOSDeployDebuggerWaitForExit extends IOSDeployDebugger {
IOSDeployDebuggerWaitForExit({
required super.logger,
required super.processUtils,
required super.launchCommand,
required super.iosDeployEnv
});
/// Create a [IOSDeployDebugger] for testing.
///
/// Sets the command to "ios-deploy" and environment to an empty map.
factory IOSDeployDebuggerWaitForExit.test({
required ProcessManager processManager,
Logger? logger,
}) {
final Logger debugLogger = logger ?? BufferLogger.test();
return IOSDeployDebuggerWaitForExit(
logger: debugLogger,
processUtils: ProcessUtils(logger: debugLogger, processManager: processManager),
launchCommand: <String>['ios-deploy'],
iosDeployEnv: <String, String>{},
);
}
final Completer<void> exitCompleter = Completer<void>();
@override
bool exit() {
final bool status = super.exit();
exitCompleter.complete();
return status;
}
}
......@@ -67,6 +67,7 @@ const FakeCommand kLaunchDebugCommand = FakeCommand(command: <String>[
// The command used to actually launch the app and attach the debugger with args in debug.
FakeCommand attachDebuggerCommand({
IOSink? stdin,
String stdout = '(lldb) run\nsuccess',
Completer<void>? completer,
bool isWirelessDevice = false,
}) {
......@@ -94,7 +95,7 @@ FakeCommand attachDebuggerCommand({
'PATH': '/usr/bin:null',
'DYLD_LIBRARY_PATH': '/path/to/libraries',
},
stdout: '(lldb) run\nsuccess',
stdout: stdout,
stdin: stdin,
);
}
......@@ -156,6 +157,54 @@ void main() {
expect(await device.stopApp(iosApp), false);
});
testWithoutContext('IOSDevice.startApp twice in a row where ios-deploy fails the first time', () async {
final BufferLogger logger = BufferLogger.test();
final FileSystem fileSystem = MemoryFileSystem.test();
final Completer<void> completer = Completer<void>();
final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
attachDebuggerCommand(
stdout: 'PROCESS_EXITED',
),
attachDebuggerCommand(
stdout: '(lldb) run\nsuccess\nThe Dart VM service is listening on http://127.0.0.1:456',
completer: completer,
),
]);
final IOSDevice device = setUpIOSDevice(
processManager: processManager,
fileSystem: fileSystem,
logger: logger,
);
final IOSApp iosApp = PrebuiltIOSApp(
projectBundleId: 'app',
bundleName: 'Runner',
uncompressedBundle: fileSystem.currentDirectory,
applicationPackage: fileSystem.currentDirectory,
);
device.portForwarder = const NoOpDevicePortForwarder();
final LaunchResult launchResult = await device.startApp(iosApp,
prebuiltApplication: true,
debuggingOptions: DebuggingOptions.enabled(BuildInfo.debug),
platformArgs: <String, dynamic>{},
);
expect(launchResult.started, false);
expect(launchResult.hasVmService, false);
final LaunchResult secondLaunchResult = await device.startApp(iosApp,
prebuiltApplication: true,
debuggingOptions: DebuggingOptions.enabled(BuildInfo.debug),
platformArgs: <String, dynamic>{},
discoveryTimeout: Duration.zero,
);
completer.complete();
expect(secondLaunchResult.started, true);
expect(secondLaunchResult.hasVmService, true);
expect(await device.stopApp(iosApp), false);
});
testWithoutContext('IOSDevice.startApp launches in debug mode via log reading on <iOS 13', () async {
final FileSystem fileSystem = MemoryFileSystem.test();
final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment