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

Fixes flutter#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`.

Author: christopherfujino

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<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,22 +150,22 @@ 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!);
     }
     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;
-}

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<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;
+}

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<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);
     }
   }
 

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: <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;
+  }
+}