Skip to content

Commit

Permalink
[flutter_tools] Instantiate shutdown hooks before localfilesystem (#1…
Browse files Browse the repository at this point in the history
…10693)
  • Loading branch information
christopherfujino authored Sep 2, 2022
1 parent 1560d0c commit 08d5d51
Show file tree
Hide file tree
Showing 11 changed files with 79 additions and 48 deletions.
1 change: 1 addition & 0 deletions packages/flutter_tools/lib/executable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ Future<void> main(List<String> args) async {
},
PreRunValidator: () => PreRunValidator(fileSystem: globals.fs),
},
shutdownHooks: globals.shutdownHooks,
);
}

Expand Down
22 changes: 12 additions & 10 deletions packages/flutter_tools/lib/runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Future<int> run(
bool? reportCrashes,
String? flutterVersion,
Map<Type, Generator>? overrides,
required ShutdownHooks shutdownHooks,
}) async {
if (muteCommandLogging) {
// Remove the verbose option; for help and doctor, users don't need to see
Expand Down Expand Up @@ -64,7 +65,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);
return await _exit(0, shutdownHooks: shutdownHooks);
}

// We already hit some error, so don't return success. The error path
Expand All @@ -74,15 +75,15 @@ Future<int> run(
// This catches all exceptions to send to crash logging, etc.
firstError = error;
firstStackTrace = stackTrace;
return _handleToolError(error, stackTrace, verbose, args, reportCrashes!, getVersion);
return _handleToolError(error, stackTrace, verbose, args, reportCrashes!, getVersion, shutdownHooks);
}
}, onError: (Object error, StackTrace stackTrace) async { // ignore: deprecated_member_use
// If sending a crash report throws an error into the zone, we don't want
// to re-try sending the crash report with *that* error. Rather, we want
// to send the original error that triggered the crash report.
firstError ??= error;
firstStackTrace ??= stackTrace;
await _handleToolError(firstError!, firstStackTrace, verbose, args, reportCrashes!, getVersion);
await _handleToolError(firstError!, firstStackTrace, verbose, args, reportCrashes!, getVersion, shutdownHooks);
});
}, overrides: overrides);
}
Expand All @@ -94,27 +95,28 @@ Future<int> _handleToolError(
List<String> args,
bool reportCrashes,
String Function() getFlutterVersion,
ShutdownHooks shutdownHooks,
) async {
if (error is UsageException) {
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);
return _exit(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);
return _exit(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);
return _exit(error.exitCode, shutdownHooks: shutdownHooks);
}
} else {
// We've crashed; emit a log report.
Expand All @@ -124,7 +126,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);
return _exit(1, shutdownHooks: shutdownHooks);
}

// Report to both [Usage] and [CrashReportSender].
Expand Down Expand Up @@ -165,7 +167,7 @@ Future<int> _handleToolError(
final File file = await _createLocalCrashReport(details);
await globals.crashReporter!.informUser(details, file);

return _exit(1);
return _exit(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(
Expand Down Expand Up @@ -228,7 +230,7 @@ Future<File> _createLocalCrashReport(CrashDetails details) async {
return crashFile;
}

Future<int> _exit(int code) async {
Future<int> _exit(int code, {required ShutdownHooks shutdownHooks}) async {
// Prints the welcome message if needed.
globals.flutterUsage.printWelcome();

Expand All @@ -241,7 +243,7 @@ Future<int> _exit(int code) async {
}

// Run shutdown hooks before flushing logs
await globals.shutdownHooks!.runShutdownHooks();
await shutdownHooks.runShutdownHooks(globals.logger);

final Completer<void> completer = Completer<void>();

Expand Down
11 changes: 6 additions & 5 deletions packages/flutter_tools/lib/src/base/file_system.dart
Original file line number Diff line number Diff line change
Expand Up @@ -177,17 +177,18 @@ File getUniqueFile(Directory dir, String baseName, String ext) {
/// directories and files that the tool creates under the system temporary
/// directory when the tool exits either normally or when killed by a signal.
class LocalFileSystem extends local_fs.LocalFileSystem {
LocalFileSystem(this._signals, this._fatalSignals, this._shutdownHooks);
LocalFileSystem(this._signals, this._fatalSignals, this.shutdownHooks);

@visibleForTesting
LocalFileSystem.test({
required Signals signals,
List<ProcessSignal> fatalSignals = Signals.defaultExitSignals,
}) : this(signals, fatalSignals, null);
}) : this(signals, fatalSignals, ShutdownHooks());

Directory? _systemTemp;
final Map<ProcessSignal, Object> _signalTokens = <ProcessSignal, Object>{};
final ShutdownHooks? _shutdownHooks;

final ShutdownHooks shutdownHooks;

Future<void> dispose() async {
_tryToDeleteTemp();
Expand All @@ -206,7 +207,7 @@ class LocalFileSystem extends local_fs.LocalFileSystem {
_systemTemp?.deleteSync(recursive: true);
}
} on FileSystemException {
// ignore.
// ignore
}
_systemTemp = null;
}
Expand Down Expand Up @@ -239,7 +240,7 @@ class LocalFileSystem extends local_fs.LocalFileSystem {
}
// Make sure that the temporary directory is cleaned up when the tool
// exits normally.
_shutdownHooks?.addShutdownHook(
shutdownHooks.addShutdownHook(
_tryToDeleteTemp,
);
}
Expand Down
37 changes: 20 additions & 17 deletions packages/flutter_tools/lib/src/base/process.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'dart:async';

import 'package:meta/meta.dart';
import 'package:process/process.dart';

import '../convert.dart';
Expand All @@ -13,7 +14,7 @@ import 'logger.dart';
typedef StringConverter = String? Function(String string);

/// A function that will be run before the VM exits.
typedef ShutdownHook = FutureOr<dynamic> Function();
typedef ShutdownHook = FutureOr<void> Function();

// TODO(ianh): We have way too many ways to run subprocesses in this project.
// Convert most of these into one or more lightweight wrappers around the
Expand All @@ -22,34 +23,34 @@ typedef ShutdownHook = FutureOr<dynamic> Function();
// for more details.

abstract class ShutdownHooks {
factory ShutdownHooks({
required Logger logger,
}) => _DefaultShutdownHooks(
logger: logger,
);
factory ShutdownHooks() => _DefaultShutdownHooks();

/// Registers a [ShutdownHook] to be executed before the VM exits.
void addShutdownHook(
ShutdownHook shutdownHook
);

@visibleForTesting
List<ShutdownHook> get registeredHooks;

/// Runs all registered shutdown hooks and returns a future that completes when
/// all such hooks have finished.
///
/// Shutdown hooks will be run in groups by their [ShutdownStage]. All shutdown
/// hooks within a given stage will be started in parallel and will be
/// guaranteed to run to completion before shutdown hooks in the next stage are
/// started.
Future<void> runShutdownHooks();
///
/// This class is constructed before the [Logger], so it cannot be direct
/// injected in the constructor.
Future<void> runShutdownHooks(Logger logger);
}

class _DefaultShutdownHooks implements ShutdownHooks {
_DefaultShutdownHooks({
required Logger logger,
}) : _logger = logger;
_DefaultShutdownHooks();

final Logger _logger;
final List<ShutdownHook> _shutdownHooks = <ShutdownHook>[];
@override
final List<ShutdownHook> registeredHooks = <ShutdownHook>[];

bool _shutdownHooksRunning = false;

Expand All @@ -58,16 +59,18 @@ class _DefaultShutdownHooks implements ShutdownHooks {
ShutdownHook shutdownHook
) {
assert(!_shutdownHooksRunning);
_shutdownHooks.add(shutdownHook);
registeredHooks.add(shutdownHook);
}

@override
Future<void> runShutdownHooks() async {
_logger.printTrace('Running shutdown hooks');
Future<void> runShutdownHooks(Logger logger) async {
logger.printTrace(
'Running ${registeredHooks.length} shutdown hook${registeredHooks.length == 1 ? '' : 's'}',
);
_shutdownHooksRunning = true;
try {
final List<Future<dynamic>> futures = <Future<dynamic>>[];
for (final ShutdownHook shutdownHook in _shutdownHooks) {
for (final ShutdownHook shutdownHook in registeredHooks) {
final FutureOr<dynamic> result = shutdownHook();
if (result is Future<dynamic>) {
futures.add(result);
Expand All @@ -77,7 +80,7 @@ class _DefaultShutdownHooks implements ShutdownHooks {
} finally {
_shutdownHooksRunning = false;
}
_logger.printTrace('Shutdown hooks complete');
logger.printTrace('Shutdown hooks complete');
}
}

Expand Down
1 change: 0 additions & 1 deletion packages/flutter_tools/lib/src/context_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,6 @@ Future<T> runInContext<T>(
platform: globals.platform,
usage: globals.flutterUsage,
),
ShutdownHooks: () => ShutdownHooks(logger: globals.logger),
Stdio: () => Stdio(),
SystemClock: () => const SystemClock(),
Usage: () => Usage(
Expand Down
6 changes: 5 additions & 1 deletion packages/flutter_tools/lib/src/globals.dart
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,11 @@ PlistParser? _plistInstance;
/// The global template renderer.
TemplateRenderer get templateRenderer => context.get<TemplateRenderer>()!;

ShutdownHooks? get shutdownHooks => context.get<ShutdownHooks>();
/// Global [ShutdownHooks] that should be run before the tool process exits.
///
/// This is depended on by [localFileSystem] which is called before any
/// [Context] is set up, and thus this cannot be a Context getter.
final ShutdownHooks shutdownHooks = ShutdownHooks();

// Unless we're in a test of this class's signal handling features, we must
// have only one instance created with the singleton LocalSignals instance
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import 'package:file_testing/file_testing.dart';
import 'package:flutter_tools/src/base/common.dart';
import 'package:flutter_tools/src/base/file_system.dart';
import 'package:flutter_tools/src/base/io.dart';
import 'package:flutter_tools/src/base/logger.dart';
import 'package:flutter_tools/src/base/platform.dart';
import 'package:flutter_tools/src/base/signals.dart';
import 'package:test/fake.dart';
Expand Down Expand Up @@ -162,6 +163,21 @@ void main() {
signalUnderTest = ProcessSignal(fakeSignal);
});

testWithoutContext('runs shutdown hooks', () async {
final Signals signals = Signals.test();
final LocalFileSystem localFileSystem = LocalFileSystem.test(
signals: signals,
);
final Directory temp = localFileSystem.systemTempDirectory;

expect(temp.existsSync(), isTrue);
expect(localFileSystem.shutdownHooks.registeredHooks, hasLength(1));
final BufferLogger logger = BufferLogger.test();
await localFileSystem.shutdownHooks.runShutdownHooks(logger);
expect(temp.existsSync(), isFalse);
expect(logger.traceText, contains('Running 1 shutdown hook'));
});

testWithoutContext('deletes system temp entry on a fatal signal', () async {
final Completer<void> completer = Completer<void>();
final Signals signals = Signals.test();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ void main() {
int i = 1;
int? cleanup;

final ShutdownHooks shutdownHooks = ShutdownHooks(logger: BufferLogger.test());
final ShutdownHooks shutdownHooks = ShutdownHooks();

shutdownHooks.addShutdownHook(() async {
cleanup = i++;
});

await shutdownHooks.runShutdownHooks();
await shutdownHooks.runShutdownHooks(BufferLogger.test());

expect(cleanup, 1);
});
Expand Down
24 changes: 14 additions & 10 deletions packages/flutter_tools/test/general.shard/runner/runner_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'package:flutter_tools/src/base/file_system.dart';
import 'package:flutter_tools/src/base/io.dart' as io;
import 'package:flutter_tools/src/base/net.dart';
import 'package:flutter_tools/src/base/platform.dart';
import 'package:flutter_tools/src/base/process.dart';
import 'package:flutter_tools/src/base/user_messages.dart';
import 'package:flutter_tools/src/cache.dart';
import 'package:flutter_tools/src/globals.dart' as globals;
Expand Down Expand Up @@ -68,6 +69,7 @@ void main() {
// This flutterVersion disables crash reporting.
flutterVersion: '[user-branch]/',
reportCrashes: true,
shutdownHooks: ShutdownHooks(),
));
return null;
},
Expand Down Expand Up @@ -120,6 +122,7 @@ void main() {
// This flutterVersion disables crash reporting.
flutterVersion: '[user-branch]/',
reportCrashes: true,
shutdownHooks: ShutdownHooks(),
));
return null;
},
Expand Down Expand Up @@ -159,16 +162,17 @@ void main() {
// catch it in a zone.
unawaited(runZoned<Future<void>>(
() {
unawaited(runner.run(
<String>['crash'],
() => <FlutterCommand>[
CrashingFlutterCommand(),
],
// This flutterVersion disables crash reporting.
flutterVersion: '[user-branch]/',
reportCrashes: true,
));
return null;
unawaited(runner.run(
<String>['crash'],
() => <FlutterCommand>[
CrashingFlutterCommand(),
],
// This flutterVersion disables crash reporting.
flutterVersion: '[user-branch]/',
reportCrashes: true,
shutdownHooks: ShutdownHooks(),
));
return null;
},
onError: (Object error, StackTrace stack) { // ignore: deprecated_member_use
expect(firstExitCode, isNotNull);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ void main() {
]);

// Check for message only printed in verbose mode.
expect(result.stdout, contains('Running shutdown hooks'));
expect(result.stdout, contains('Shutdown hooks complete'));
});

testWithoutContext('flutter config contains all features', () async {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ void main() {
return null;
}),
Barrier('Performing hot restart...'.padRight(progressMessageWidth)),
Multiple(<Pattern>[RegExp(r'^Restarted application in [0-9]+ms.$'), 'called main', 'called paint'], handler: (String line) {
// This could look like 'Restarted application in 1,237ms.'
Multiple(<Pattern>[RegExp(r'^Restarted application in .+m?s.$'), 'called main', 'called paint'], handler: (String line) {
return 'q';
}),
const Barrier('Application finished.'),
Expand Down

0 comments on commit 08d5d51

Please sign in to comment.