From 3d7cd3594a12dacf2ddf45f630bb4bcc401e6f17 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 13 Sep 2023 13:05:29 -0700 Subject: [PATCH] [flutter_tools] Run ShutdownHooks when handling signals (#134590) 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`. --- packages/flutter_tools/lib/runner.dart | 86 ++----------------- .../flutter_tools/lib/src/base/process.dart | 75 ++++++++++++++++ .../flutter_tools/lib/src/base/signals.dart | 13 ++- .../test/general.shard/base/signals_test.dart | 40 ++++++++- 4 files changed, 129 insertions(+), 85 deletions(-) diff --git a/packages/flutter_tools/lib/runner.dart b/packages/flutter_tools/lib/runner.dart index 9b33580af0..262eed5229 100644 --- a/packages/flutter_tools/lib/runner.dart +++ b/packages/flutter_tools/lib/runner.dart @@ -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 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 _handleToolError( globals.printError('${error.message}\n'); globals.printError("Run 'flutter -h' (or 'flutter -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 _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 _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 _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 _createLocalCrashReport(CrashDetails details) async { return crashFile; } - -Future _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 completer = Completer(); - - // 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; -} diff --git a/packages/flutter_tools/lib/src/base/process.dart b/packages/flutter_tools/lib/src/base/process.dart index 82a3c89b1d..6fb36b0bc0 100644 --- a/packages/flutter_tools/lib/src/base/process.dart +++ b/packages/flutter_tools/lib/src/base/process.dart @@ -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 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 completer = Completer(); + + // 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; +} diff --git a/packages/flutter_tools/lib/src/base/signals.dart b/packages/flutter_tools/lib/src/base/signals.dart index 9185762cdf..a83d85b622 100644 --- a/packages/flutter_tools/lib/src/base/signals.dart +++ b/packages/flutter_tools/lib/src/base/signals.dart @@ -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 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 defaultExitSignals = [ @@ -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 exitSignals; + final ShutdownHooks _shutdownHooks; // A table mapping (signal, token) -> signal handler. final Map> _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); } } diff --git a/packages/flutter_tools/test/general.shard/base/signals_test.dart b/packages/flutter_tools/test/general.shard/base/signals_test.dart index 0d8cae24c1..d2b321b102 100644 --- a/packages/flutter_tools/test/general.shard/base/signals_test.dart +++ b/packages/flutter_tools/test/general.shard/base/signals_test.dart @@ -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: [signalUnderTest], + shutdownHooks: shutdownHooks, ); final Completer completer = Completer(); 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: [signalUnderTest], + shutdownHooks: shutdownHooks, + ); + final Completer completer = Completer(); + + 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 watch() => controller.stream; } + +class FakeShutdownHooks extends Fake implements ShutdownHooks { + bool ranShutdownHooks = false; + + @override + Future runShutdownHooks(Logger logger) async { + ranShutdownHooks = true; + } +}