firka/flutter
firka/flutter
5
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

[flutter_tools] Run ShutdownHooks when handling signals (#134590)

Browse Source 
Fixes https://github.com/flutter/flutter/issues/134566.

Prior to this fix, `ShutdownHooks` were run in the private helper
function `_exit()` defined in the `package:flutter_tools/runner.dart`
library. Independent of this, the tool had signal handling logic that
traps SIGINT and SIGTERM. However, these handlers called `exit()` from
`dart:io`, and didn't run these hooks.

This PR moves the `_exit()` private helper to
`package:flutter_tools/src/base/process.dart` and renames it to
`exitWithHooks()`, so that it can be called by the signal handlers in
`package:flutter_tools/src/base/signals.dart`.
This commit is contained in:
Christopher Fujino 2023-09-13 13:05:29 -07:00 committed by GitHub
parent ab1b865e58
commit 3d7cd3594a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 129 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
packages/flutter_tools/lib/runner.dart
View File

@ -20,7 +20,6 @@ import 'src/context_runner.dart';
import 'src/doctor.dart';
import 'src/globals.dart' as globals;
import 'src/reporting/crash_reporting.dart';
import 'src/reporting/first_run.dart';
import 'src/reporting/reporting.dart';
import 'src/runner/flutter_command.dart';
import 'src/runner/flutter_command_runner.dart';
@ -115,7 +114,7 @@ Future<int> run(
// Triggering [runZoned]'s error callback does not necessarily mean that
// we stopped executing the body. See https://github.com/dart-lang/sdk/issues/42150.
if (firstError == null) {
return await _exit(0, shutdownHooks: shutdownHooks);
return await exitWithHooks(0, shutdownHooks: shutdownHooks);
}
// We already hit some error, so don't return success. The error path
@ -151,7 +150,7 @@ Future<int> _handleToolError(
globals.printError('${error.message}\n');
globals.printError("Run 'flutter -h' (or 'flutter <command> -h') for available flutter commands and options.");
// Argument error exit code.
return _exit(64, shutdownHooks: shutdownHooks);
return exitWithHooks(64, shutdownHooks: shutdownHooks);
} else if (error is ToolExit) {
if (error.message != null) {
globals.printError(error.message!);
@ -159,14 +158,14 @@ Future<int> _handleToolError(
if (verbose) {
globals.printError('\n$stackTrace\n');
}
return _exit(error.exitCode ?? 1, shutdownHooks: shutdownHooks);
return exitWithHooks(error.exitCode ?? 1, shutdownHooks: shutdownHooks);
} else if (error is ProcessExit) {
// We've caught an exit code.
if (error.immediate) {
exit(error.exitCode);
return error.exitCode;
} else {
return _exit(error.exitCode, shutdownHooks: shutdownHooks);
return exitWithHooks(error.exitCode, shutdownHooks: shutdownHooks);
}
} else {
// We've crashed; emit a log report.
@ -176,7 +175,7 @@ Future<int> _handleToolError(
// Print the stack trace on the bots - don't write a crash report.
globals.stdio.stderrWrite('$error\n');
globals.stdio.stderrWrite('$stackTrace\n');
return _exit(1, shutdownHooks: shutdownHooks);
return exitWithHooks(1, shutdownHooks: shutdownHooks);
}
// Report to both [Usage] and [CrashReportSender].
@ -217,7 +216,7 @@ Future<int> _handleToolError(
final File file = await _createLocalCrashReport(details);
await globals.crashReporter!.informUser(details, file);
return _exit(1, shutdownHooks: shutdownHooks);
return exitWithHooks(1, shutdownHooks: shutdownHooks);
// This catch catches all exceptions to ensure the message below is printed.
} catch (error, st) { // ignore: avoid_catches_without_on_clauses
globals.stdio.stderrWrite(
@ -283,76 +282,3 @@ Future<File> _createLocalCrashReport(CrashDetails details) async {
return crashFile;
}
Future<int> _exit(int code, {required ShutdownHooks shutdownHooks}) async {
// Need to get the boolean returned from `messenger.shouldDisplayLicenseTerms()`
// before invoking the print welcome method because the print welcome method
// will set `messenger.shouldDisplayLicenseTerms()` to false
final FirstRunMessenger messenger =
FirstRunMessenger(persistentToolState: globals.persistentToolState!);
final bool legacyAnalyticsMessageShown =
messenger.shouldDisplayLicenseTerms();
// Prints the welcome message if needed for legacy analytics.
globals.flutterUsage.printWelcome();
// Ensure that the consent message has been displayed for unified analytics
if (globals.analytics.shouldShowMessage) {
globals.logger.printStatus(globals.analytics.getConsentMessage);
if (!globals.flutterUsage.enabled) {
globals.printStatus(
'Please note that analytics reporting was already disabled, '
'and will continue to be disabled.\n');
}
// Because the legacy analytics may have also sent a message,
// the conditional below will print additional messaging informing
// users that the two consent messages they are receiving is not a
// bug
if (legacyAnalyticsMessageShown) {
globals.logger
.printStatus('You have received two consent messages because '
'the flutter tool is migrating to a new analytics system. '
'Disabling analytics collection will disable both the legacy '
'and new analytics collection systems. '
'You can disable analytics reporting by running `flutter --disable-analytics`\n');
}
// Invoking this will onboard the flutter tool onto
// the package on the developer's machine and will
// allow for events to be sent to Google Analytics
// on subsequent runs of the flutter tool (ie. no events
// will be sent on the first run to allow developers to
// opt out of collection)
globals.analytics.clientShowedMessage();
}
// Send any last analytics calls that are in progress without overly delaying
// the tool's exit (we wait a maximum of 250ms).
if (globals.flutterUsage.enabled) {
final Stopwatch stopwatch = Stopwatch()..start();
await globals.flutterUsage.ensureAnalyticsSent();
globals.printTrace('ensureAnalyticsSent: ${stopwatch.elapsedMilliseconds}ms');
}
// Run shutdown hooks before flushing logs
await shutdownHooks.runShutdownHooks(globals.logger);
final Completer<void> completer = Completer<void>();
// Give the task / timer queue one cycle through before we hard exit.
Timer.run(() {
try {
globals.printTrace('exiting with code $code');
exit(code);
completer.complete();
// This catches all exceptions because the error is propagated on the
// completer.
} catch (error, stackTrace) { // ignore: avoid_catches_without_on_clauses
completer.completeError(error, stackTrace);
}
});
await completer.future;
return code;
}

75
packages/flutter_tools/lib/src/base/process.dart
View File

@ -8,6 +8,8 @@ import 'package:meta/meta.dart';
import 'package:process/process.dart';
import '../convert.dart';
import '../globals.dart' as globals;
import '../reporting/first_run.dart';
import 'io.dart';
import 'logger.dart';
@ -564,3 +566,76 @@ class _DefaultProcessUtils implements ProcessUtils {
}
}
}
Future<int> exitWithHooks(int code, {required ShutdownHooks shutdownHooks}) async {
// Need to get the boolean returned from `messenger.shouldDisplayLicenseTerms()`
// before invoking the print welcome method because the print welcome method
// will set `messenger.shouldDisplayLicenseTerms()` to false
final FirstRunMessenger messenger =
FirstRunMessenger(persistentToolState: globals.persistentToolState!);
final bool legacyAnalyticsMessageShown =
messenger.shouldDisplayLicenseTerms();
// Prints the welcome message if needed for legacy analytics.
globals.flutterUsage.printWelcome();
// Ensure that the consent message has been displayed for unified analytics
if (globals.analytics.shouldShowMessage) {
globals.logger.printStatus(globals.analytics.getConsentMessage);
if (!globals.flutterUsage.enabled) {
globals.printStatus(
'Please note that analytics reporting was already disabled, '
'and will continue to be disabled.\n');
}
// Because the legacy analytics may have also sent a message,
// the conditional below will print additional messaging informing
// users that the two consent messages they are receiving is not a
// bug
if (legacyAnalyticsMessageShown) {
globals.logger
.printStatus('You have received two consent messages because '
'the flutter tool is migrating to a new analytics system. '
'Disabling analytics collection will disable both the legacy '
'and new analytics collection systems. '
'You can disable analytics reporting by running `flutter --disable-analytics`\n');
}
// Invoking this will onboard the flutter tool onto
// the package on the developer's machine and will
// allow for events to be sent to Google Analytics
// on subsequent runs of the flutter tool (ie. no events
// will be sent on the first run to allow developers to
// opt out of collection)
globals.analytics.clientShowedMessage();
}
// Send any last analytics calls that are in progress without overly delaying
// the tool's exit (we wait a maximum of 250ms).
if (globals.flutterUsage.enabled) {
final Stopwatch stopwatch = Stopwatch()..start();
await globals.flutterUsage.ensureAnalyticsSent();
globals.printTrace('ensureAnalyticsSent: ${stopwatch.elapsedMilliseconds}ms');
}
// Run shutdown hooks before flushing logs
await shutdownHooks.runShutdownHooks(globals.logger);
final Completer<void> completer = Completer<void>();
// Give the task / timer queue one cycle through before we hard exit.
Timer.run(() {
try {
globals.printTrace('exiting with code $code');
exit(code);
completer.complete();
// This catches all exceptions because the error is propagated on the
// completer.
} catch (error, stackTrace) { // ignore: avoid_catches_without_on_clauses
completer.completeError(error, stackTrace);
}
});
await completer.future;
return code;
}

13
packages/flutter_tools/lib/src/base/signals.dart
View File

@ -6,6 +6,8 @@ import 'dart:async';
import 'package:meta/meta.dart';
import '../base/process.dart';
import '../globals.dart' as globals;
import 'async_guard.dart';
import 'io.dart';
@ -18,7 +20,8 @@ abstract class Signals {
@visibleForTesting
factory Signals.test({
List<ProcessSignal> exitSignals = defaultExitSignals,
}) => LocalSignals._(exitSignals);
ShutdownHooks? shutdownHooks,
}) => LocalSignals._(exitSignals, shutdownHooks: shutdownHooks);
// The default list of signals that should cause the process to exit.
static const List<ProcessSignal> defaultExitSignals = <ProcessSignal>[
@ -50,13 +53,17 @@ abstract class Signals {
/// We use a singleton instance of this class to ensure that all handlers for
/// fatal signals run before this class calls exit().
class LocalSignals implements Signals {
LocalSignals._(this.exitSignals);
LocalSignals._(
this.exitSignals, {
ShutdownHooks? shutdownHooks,
}) : _shutdownHooks = shutdownHooks ?? globals.shutdownHooks;
static LocalSignals instance = LocalSignals._(
Signals.defaultExitSignals,
);
final List<ProcessSignal> exitSignals;
final ShutdownHooks _shutdownHooks;
// A table mapping (signal, token) -> signal handler.
final Map<ProcessSignal, Map<Object, SignalHandler>> _handlersTable =
@ -144,7 +151,7 @@ class LocalSignals implements Signals {
// If this was a signal that should cause the process to go down, then
// call exit();
if (_shouldExitFor(s)) {
exit(0);
await exitWithHooks(0, shutdownHooks: _shutdownHooks);
}
}

40
packages/flutter_tools/test/general.shard/base/signals_test.dart
View File

@ -6,19 +6,24 @@ import 'dart:async';
import 'dart:io' as io;
import 'package:flutter_tools/src/base/io.dart';
import 'package:flutter_tools/src/base/logger.dart';
import 'package:flutter_tools/src/base/process.dart';
import 'package:flutter_tools/src/base/signals.dart';
import 'package:test/fake.dart';
import '../../src/common.dart';
import '../../src/context.dart';
void main() {
group('Signals', () {
late Signals signals;
late FakeProcessSignal fakeSignal;
late ProcessSignal signalUnderTest;
late FakeShutdownHooks shutdownHooks;
setUp(() {
signals = Signals.test();
shutdownHooks = FakeShutdownHooks();
signals = Signals.test(shutdownHooks: shutdownHooks);
fakeSignal = FakeProcessSignal();
signalUnderTest = ProcessSignal(fakeSignal);
});
@ -168,9 +173,10 @@ void main() {
expect(errList, isEmpty);
});
testWithoutContext('all handlers for exiting signals are run before exit', () async {
testUsingContext('all handlers for exiting signals are run before exit', () async {
final Signals signals = Signals.test(
exitSignals: <ProcessSignal>[signalUnderTest],
shutdownHooks: shutdownHooks,
);
final Completer<void> completer = Completer<void>();
bool first = false;
@ -201,6 +207,27 @@ void main() {
fakeSignal.controller.add(fakeSignal);
await completer.future;
expect(shutdownHooks.ranShutdownHooks, isTrue);
});
testUsingContext('ShutdownHooks run before exiting', () async {
final Signals signals = Signals.test(
exitSignals: <ProcessSignal>[signalUnderTest],
shutdownHooks: shutdownHooks,
);
final Completer<void> completer = Completer<void>();
setExitFunctionForTests((int exitCode) {
expect(exitCode, 0);
restoreExitFunction();
completer.complete();
});
signals.addHandler(signalUnderTest, (ProcessSignal s) {});
fakeSignal.controller.add(fakeSignal);
await completer.future;
expect(shutdownHooks.ranShutdownHooks, isTrue);
});
});
}
@ -211,3 +238,12 @@ class FakeProcessSignal extends Fake implements io.ProcessSignal {
@override
Stream<io.ProcessSignal> watch() => controller.stream;
}
class FakeShutdownHooks extends Fake implements ShutdownHooks {
bool ranShutdownHooks = false;
@override
Future<void> runShutdownHooks(Logger logger) async {
ranShutdownHooks = true;
}
}