Skip to content

Commit

Permalink
fallback to sigkill when closing Chromium
Browse files Browse the repository at this point in the history
  • Loading branch information
@yjbanov
yjbanov committed Sep 28, 2023
1 parent 433bca5 commit d6ae793
Show file tree
Hide file tree
Showing 6 changed files with 208 additions and 22 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;
});
});
}
}
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
120 changes: 117 additions & 3 deletions packages/flutter_tools/test/web.shard/chrome_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:io' as io;

import 'package:fake_async/fake_async.dart';
import 'package:file/memory.dart';
import 'package:file_testing/file_testing.dart';
import 'package:flutter_tools/src/base/file_system.dart';
Expand All @@ -16,7 +18,7 @@ import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';

import '../src/common.dart';
import '../src/fake_process_manager.dart';
import '../src/fakes.dart';
import '../src/fakes.dart' hide FakeProcess;

const List<String> kChromeArgs = <String>[
'--disable-background-timer-throttling',
Expand Down Expand Up @@ -165,6 +167,116 @@ void main() {
);
});

testWithoutContext('exits normally using SIGTERM', () async {
final BufferLogger logger = BufferLogger.test();
final FakeAsync fakeAsync = FakeAsync();

fakeAsync.run((_) {
() async {
final FakeChromeConnection chromeConnection = FakeChromeConnection(maxRetries: 4);
final ChromiumLauncher chromiumLauncher = ChromiumLauncher(
fileSystem: fileSystem,
platform: platform,
processManager: processManager,
operatingSystemUtils: operatingSystemUtils,
browserFinder: findChromeExecutable,
logger: logger,
);

final FakeProcess process = FakeProcess(
duration: const Duration(seconds: 3),
);

final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);

final Future<void> closeFuture = chrome.close();
fakeAsync.elapse(const Duration(seconds: 4));
await closeFuture;

expect(process.signals, <io.ProcessSignal>[io.ProcessSignal.sigterm]);
}();
});

fakeAsync.flushTimers();
expect(logger.warningText, isEmpty);
});

testWithoutContext('falls back to SIGKILL if SIGTERM did not work', () async {
final BufferLogger logger = BufferLogger.test();
final FakeAsync fakeAsync = FakeAsync();

fakeAsync.run((_) {
() async {
final FakeChromeConnection chromeConnection = FakeChromeConnection(maxRetries: 4);
final ChromiumLauncher chromiumLauncher = ChromiumLauncher(
fileSystem: fileSystem,
platform: platform,
processManager: processManager,
operatingSystemUtils: operatingSystemUtils,
browserFinder: findChromeExecutable,
logger: logger,
);

final FakeProcess process = FakeProcess(
duration: const Duration(seconds: 6),
);

final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);

final Future<void> closeFuture = chrome.close();
fakeAsync.elapse(const Duration(seconds: 7));
await closeFuture;

expect(process.signals, <io.ProcessSignal>[io.ProcessSignal.sigterm, io.ProcessSignal.sigkill]);
}();
});

fakeAsync.flushTimers();
expect(
logger.warningText,
'Failed to exit Chromium (pid: 1234) using SIGTERM. Will try sending SIGKILL instead.\n',
);
});

testWithoutContext('falls back to a warning if SIGKILL did not work', () async {
final BufferLogger logger = BufferLogger.test();
final FakeAsync fakeAsync = FakeAsync();

fakeAsync.run((_) {
() async {
final FakeChromeConnection chromeConnection = FakeChromeConnection(maxRetries: 4);
final ChromiumLauncher chromiumLauncher = ChromiumLauncher(
fileSystem: fileSystem,
platform: platform,
processManager: processManager,
operatingSystemUtils: operatingSystemUtils,
browserFinder: findChromeExecutable,
logger: logger,
);

final FakeProcess process = FakeProcess(
duration: const Duration(seconds: 20),
);

final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);

final Future<void> closeFuture = chrome.close();
fakeAsync.elapse(const Duration(seconds: 30));
await closeFuture;
expect(process.signals, <io.ProcessSignal>[io.ProcessSignal.sigterm, io.ProcessSignal.sigkill]);
}();
});

fakeAsync.flushTimers();
expect(
logger.warningText,
'Failed to exit Chromium (pid: 1234) using SIGTERM. Will try sending SIGKILL instead.\n'
'Failed to exit Chromium (pid: 1234) 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.\n',
);
});

testWithoutContext('does not crash if saving profile information fails due to a file system exception.', () async {
final BufferLogger logger = BufferLogger.test();
chromeLauncher = ChromiumLauncher(
Expand Down Expand Up @@ -656,7 +768,8 @@ void main() {
browserFinder: findChromeExecutable,
logger: logger,
);
final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher);
final FakeProcess process = FakeProcess();
final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
expect(await chromiumLauncher.connect(chrome, false), equals(chrome));
expect(logger.errorText, isEmpty);
});
Expand All @@ -672,7 +785,8 @@ void main() {
browserFinder: findChromeExecutable,
logger: logger,
);
final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher);
final FakeProcess process = FakeProcess();
final Chromium chrome = Chromium(0, chromeConnection, chromiumLauncher: chromiumLauncher, process: process, logger: logger);
await expectToolExitLater(
chromiumLauncher.connect(chrome, false),
allOf(
Expand Down

0 comments on commit d6ae793

Please sign in to comment.