Skip to content

Commit

Permalink
[tool] fallback to sigkill when closing Chromium (#135521)
Browse files Browse the repository at this point in the history 
This implements #132654 (comment), namely:

Make `Chromium.close` more robust:

* Send `SIGTERM` and wait up to 5 seconds, if the process exited, great! Return from the function.
* If the process has not exited, then send a `SIGKILL`, which is a much firmer way to exit a process. Same as before, wait up to 5 seconds, if the process exited, great! Return from the function.
* If it still hasn't exited then give up trying to exit Chromium, just print a warning to the console and return from the function.

Bonus: a few nullability fixes and extra `-v` logging.

Fixes #132654
  • Loading branch information
@yjbanov
yjbanov committed Sep 28, 2023
1 parent b5c8fd1 commit 371aadd
Show file tree
Hide file tree
Showing 7 changed files with 212 additions and 26 deletions.
18 changes: 15 additions & 3 deletions packages/flutter_tools/lib/src/base/io.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,15 +191,27 @@ class ProcessSignal {
///
/// Returns true if the signal was delivered, false otherwise.
///
/// On Windows, this can only be used with [ProcessSignal.sigterm], which
/// terminates the process.
/// On Windows, this can only be used with [sigterm], which terminates the
/// process.
///
/// This is implemented by sending the signal using [Process.killPid].
/// This is implemented by sending the signal using [io.Process.killPid] and
/// therefore cannot be faked in tests. To fake sending signals in tests, use
/// [kill] instead.
bool send(int pid) {
assert(!_platform.isWindows || this == ProcessSignal.sigterm);
return io.Process.killPid(pid, _delegate);
}

/// A more testable variant of [send].
///
/// Sends this signal to the given `process` by invoking [io.Process.kill].
///
/// In tests this method can be faked by passing a fake implementation of the
/// [io.Process] interface.
bool kill(io.Process process) {
return process.kill(_delegate);
}

@override
String toString() => _delegate.toString();
}
Expand Down
20 changes: 17 additions & 3 deletions packages/flutter_tools/lib/src/test/flutter_web_platform.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,14 @@ class FlutterWebPlatform extends PlatformPlugin {
if (_closed) {
throw StateError('Load called on a closed FlutterWebPlatform');
}

final String pathFromTest = _fileSystem.path.relative(path, from: _fileSystem.path.join(_root, 'test'));
final Uri suiteUrl = url.resolveUri(_fileSystem.path.toUri('${_fileSystem.path.withoutExtension(pathFromTest)}.html'));
final String relativePath = _fileSystem.path.relative(_fileSystem.path.normalize(path), from: _fileSystem.currentDirectory.path);
if (_logger.isVerbose) {
_logger.printTrace('Loading test suite $relativePath.');
}

final PoolResource lockResource = await _suiteLock.request();

final Runtime browser = platform.runtime;
Expand All @@ -455,17 +463,23 @@ class FlutterWebPlatform extends PlatformPlugin {
throw StateError('Load called on a closed FlutterWebPlatform');
}

final String pathFromTest = _fileSystem.path.relative(path, from: _fileSystem.path.join(_root, 'test'));
final Uri suiteUrl = url.resolveUri(_fileSystem.path.toUri('${_fileSystem.path.withoutExtension(pathFromTest)}.html'));
final String relativePath = _fileSystem.path.relative(_fileSystem.path.normalize(path), from: _fileSystem.currentDirectory.path);
if (_logger.isVerbose) {
_logger.printTrace('Running test suite $relativePath.');
}

final RunnerSuite suite = await _browserManager!.load(relativePath, suiteUrl, suiteConfig, message, onDone: () async {
await _browserManager!.close();
_browserManager = null;
lockResource.release();
if (_logger.isVerbose) {
_logger.printTrace('Test suite $relativePath finished.');
}
});

if (_closed) {
throw StateError('Load called on a closed FlutterWebPlatform');
}

return suite;
}

Expand Down
52 changes: 43 additions & 9 deletions packages/flutter_tools/lib/src/web/chrome.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,10 @@ class ChromiumLauncher {
url,
];

final Process? process = await _spawnChromiumProcess(args, chromeExecutable);
final Process process = await _spawnChromiumProcess(args, chromeExecutable);

// When the process exits, copy the user settings back to the provided data-dir.
if (process != null && cacheDir != null) {
if (cacheDir != null) {
unawaited(process.exitCode.whenComplete(() {
_cacheUserSessionInformation(userDataDir, cacheDir);
// cleanup temp dir
Expand All @@ -245,10 +245,11 @@ class ChromiumLauncher {
url: url,
process: process,
chromiumLauncher: this,
logger: _logger,
), skipCheck);
}

Future<Process?> _spawnChromiumProcess(List<String> args, String chromeExecutable) async {
Future<Process> _spawnChromiumProcess(List<String> args, String chromeExecutable) async {
if (_operatingSystemUtils.hostPlatform == HostPlatform.darwin_arm64) {
final ProcessResult result = _processManager.runSync(<String>['file', chromeExecutable]);
// Check if ARM Chrome is installed.
Expand Down Expand Up @@ -469,25 +470,58 @@ class Chromium {
this.debugPort,
this.chromeConnection, {
this.url,
Process? process,
required Process process,
required ChromiumLauncher chromiumLauncher,
required Logger logger,
}) : _process = process,
_chromiumLauncher = chromiumLauncher;
_chromiumLauncher = chromiumLauncher,
_logger = logger;

final String? url;
final int debugPort;
final Process? _process;
final Process _process;
final ChromeConnection chromeConnection;
final ChromiumLauncher _chromiumLauncher;
final Logger _logger;

Future<int?> get onExit async => _process?.exitCode;
/// Resolves to browser's main process' exit code, when the browser exits.
Future<int> get onExit async => _process.exitCode;

/// The main Chromium process that represents this instance of Chromium.
///
/// Killing this process should result in the browser exiting.
@visibleForTesting
Process get process => _process;

/// Closes all connections to the browser and asks the browser to exit.
Future<void> close() async {
if (_logger.isVerbose) {
_logger.printTrace('Shutting down Chromium.');
}
if (_chromiumLauncher.hasChromeInstance) {
_chromiumLauncher.currentCompleter = Completer<Chromium>();
}
chromeConnection.close();
_process?.kill();
await _process?.exitCode;

// Try to exit Chromium nicely using SIGTERM, before exiting it rudely using
// SIGKILL. Wait no longer than 5 seconds for Chromium to exit before
// falling back to SIGKILL, and then to a warning message.
ProcessSignal.sigterm.kill(_process);
await _process.exitCode.timeout(const Duration(seconds: 5), onTimeout: () {
_logger.printWarning(
'Failed to exit Chromium (pid: ${_process.pid}) using SIGTERM. Will try '
'sending SIGKILL instead.'
);
ProcessSignal.sigkill.kill(_process);
return _process.exitCode.timeout(const Duration(seconds: 5), onTimeout: () async {
_logger.printWarning(
'Failed to exit Chromium (pid: ${_process.pid}) using SIGKILL. Giving '
'up. Will continue, assuming Chromium has exited successfully, but '
'it is possible that this left a dangling Chromium process running '
'on the system.'
);
return 0;
});
});
}
}
8 changes: 4 additions & 4 deletions packages/flutter_tools/test/general.shard/ios/ios_device_start_prebuilt_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ void main() {
completer.complete();
expect(secondLaunchResult.started, true);
expect(secondLaunchResult.hasVmService, true);
expect(await device.stopApp(iosApp), false);
expect(await device.stopApp(iosApp), true);
});

testWithoutContext('IOSDevice.startApp launches in debug mode via log reading on <iOS 13', () async {
Expand Down Expand Up @@ -291,7 +291,7 @@ void main() {

expect(launchResult.started, true);
expect(launchResult.hasVmService, true);
expect(await device.stopApp(iosApp), false);
expect(await device.stopApp(iosApp), true);
expect(logger.errorText, contains('The Dart VM Service was not discovered after 30 seconds. This is taking much longer than expected...'));
expect(utf8.decoder.convert(stdin.writes.first), contains('process interrupt'));
completer.complete();
Expand Down Expand Up @@ -336,7 +336,7 @@ void main() {

expect(launchResult.started, true);
expect(launchResult.hasVmService, true);
expect(await device.stopApp(iosApp), false);
expect(await device.stopApp(iosApp), true);
expect(logger.errorText, contains('The Dart VM Service was not discovered after 45 seconds. This is taking much longer than expected...'));
expect(logger.errorText, contains('Click "Allow" to the prompt asking if you would like to find and connect devices on your local network.'));
completer.complete();
Expand Down Expand Up @@ -388,7 +388,7 @@ void main() {
expect(processManager, hasNoRemainingExpectations);
expect(launchResult.started, true);
expect(launchResult.hasVmService, true);
expect(await device.stopApp(iosApp), false);
expect(await device.stopApp(iosApp), true);
});

testWithoutContext('IOSDevice.startApp does not retry when ios-deploy loses connection if not in CI', () async {
Expand Down
10 changes: 7 additions & 3 deletions packages/flutter_tools/test/general.shard/resident_web_runner_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';

import '../src/common.dart';
import '../src/context.dart';
import '../src/fake_process_manager.dart';
import '../src/fake_vm_services.dart';

const List<VmServiceExpectation> kAttachLogExpectations =
Expand Down Expand Up @@ -620,8 +621,9 @@ void main() {
]);
setupMocks();
final TestChromiumLauncher chromiumLauncher = TestChromiumLauncher();
final FakeProcess process = FakeProcess();
final Chromium chrome =
Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher);
Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
chromiumLauncher.setInstance(chrome);

flutterDevice.device = GoogleChromeDevice(
Expand Down Expand Up @@ -687,8 +689,9 @@ void main() {
]);
setupMocks();
final TestChromiumLauncher chromiumLauncher = TestChromiumLauncher();
final FakeProcess process = FakeProcess();
final Chromium chrome =
Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher);
Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
chromiumLauncher.setInstance(chrome);

flutterDevice.device = GoogleChromeDevice(
Expand Down Expand Up @@ -1025,8 +1028,9 @@ void main() {
setupMocks();
final FakeChromeConnection chromeConnection = FakeChromeConnection();
final TestChromiumLauncher chromiumLauncher = TestChromiumLauncher();
final FakeProcess process = FakeProcess();
final Chromium chrome =
Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher);
Chromium(1, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
chromiumLauncher.setInstance(chrome);

flutterDevice.device = GoogleChromeDevice(
Expand Down
10 changes: 9 additions & 1 deletion packages/flutter_tools/test/src/fake_process_manager.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,17 @@ class FakeProcess implements io.Process {
/// The raw byte content of stdout.
final List<int> _stdout;

/// The list of [kill] signals this process received so far.
@visibleForTesting
List<io.ProcessSignal> get signals => _signals;
final List<io.ProcessSignal> _signals = <io.ProcessSignal>[];

@override
bool kill([io.ProcessSignal signal = io.ProcessSignal.sigterm]) {
_signals.add(signal);

// Killing a fake process has no effect.
return false;
return true;
}
}

Expand Down Expand Up @@ -400,6 +407,7 @@ abstract class FakeProcessManager implements ProcessManager {
if (fakeProcess == null) {
return false;
}
fakeProcess.kill(signal);
if (fakeProcess._completer != null) {
fakeProcess._completer.complete();
}
Expand Down

0 comments on commit 371aadd

Please sign in to comment.