Skip to content

Commit

Permalink
[iOS] Dispose of log readers and port forwarders if launch fails (flu…
Browse files Browse the repository at this point in the history
…tter#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 flutter#127141
  • Loading branch information
vashworth authored and Casey Hillers committed May 24, 2023
1 parent 7a26013 commit 2e60ba0
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 2 deletions.
4 changes: 4 additions & 0 deletions packages/flutter_tools/lib/src/ios/devices.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
Expand Down
15 changes: 14 additions & 1 deletion packages/flutter_tools/lib/src/ios/ios_deploy.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
57 changes: 57 additions & 0 deletions packages/flutter_tools/test/general.shard/ios/ios_deploy_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}) {
Expand Down Expand Up @@ -94,7 +95,7 @@ FakeCommand attachDebuggerCommand({
'PATH': '/usr/bin:null',
'DYLD_LIBRARY_PATH': '/path/to/libraries',
},
stdout: '(lldb) run\nsuccess',
stdout: stdout,
stdin: stdin,
);
}
Expand Down Expand Up @@ -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>[
Expand Down

0 comments on commit 2e60ba0

Please sign in to comment.