From e28765a9979eafcbb8b8a36e1078e46cb5ea3ce7 Mon Sep 17 00:00:00 2001 From: xster Date: Wed, 20 Sep 2017 17:24:43 -0700 Subject: [PATCH] Delinting future awaits round 3 (#10791) * round 3 * partially address comments * merge * review notes * review * review * review --- .../flutter_tools/bin/fuchsia_builder.dart | 2 +- .../flutter_tools/bin/fuchsia_tester.dart | 2 +- .../flutter_tools/lib/src/commands/logs.dart | 2 +- .../flutter_tools/lib/src/commands/run.dart | 3 ++- packages/flutter_tools/lib/src/devfs.dart | 3 ++- .../lib/src/ios/code_signing.dart | 7 ++++--- .../flutter_tools/lib/src/ios/simulators.dart | 8 ++++--- .../lib/src/resident_runner.dart | 16 +++++++++----- .../lib/src/test/coverage_collector.dart | 5 ++++- .../lib/src/test/flutter_platform.dart | 21 ++++++++++++------- 10 files changed, 45 insertions(+), 24 deletions(-) diff --git a/packages/flutter_tools/bin/fuchsia_builder.dart b/packages/flutter_tools/bin/fuchsia_builder.dart index e0d1ff8b6f..9c8d146658 100644 --- a/packages/flutter_tools/bin/fuchsia_builder.dart +++ b/packages/flutter_tools/bin/fuchsia_builder.dart @@ -42,7 +42,7 @@ const List _kRequiredOptions = const [ Future main(List args) async { final AppContext executableContext = new AppContext(); executableContext.setVariable(Logger, new StdoutLogger()); - executableContext.runInZone(() { + await executableContext.runInZone(() { // Initialize the context with some defaults. context.putIfAbsent(Platform, () => const LocalPlatform()); context.putIfAbsent(FileSystem, () => const LocalFileSystem()); diff --git a/packages/flutter_tools/bin/fuchsia_tester.dart b/packages/flutter_tools/bin/fuchsia_tester.dart index 8abd2c2652..d357834a33 100644 --- a/packages/flutter_tools/bin/fuchsia_tester.dart +++ b/packages/flutter_tools/bin/fuchsia_tester.dart @@ -38,7 +38,7 @@ const List _kRequiredOptions = const [ Future main(List args) async { final AppContext executableContext = new AppContext(); executableContext.setVariable(Logger, new StdoutLogger()); - executableContext.runInZone(() { + await executableContext.runInZone(() { // Initialize the context with some defaults. context.putIfAbsent(Platform, () => const LocalPlatform()); context.putIfAbsent(FileSystem, () => const LocalFileSystem()); diff --git a/packages/flutter_tools/lib/src/commands/logs.dart b/packages/flutter_tools/lib/src/commands/logs.dart index d10eec8fe4..dd52f2f00d 100644 --- a/packages/flutter_tools/lib/src/commands/logs.dart +++ b/packages/flutter_tools/lib/src/commands/logs.dart @@ -73,7 +73,7 @@ class LogsCommand extends FlutterCommand { // Wait for the log reader to be finished. final int result = await exitCompleter.future; - subscription.cancel(); + await subscription.cancel(); if (result != 0) throwToolExit('Error listening to $logReader logs.'); } diff --git a/packages/flutter_tools/lib/src/commands/run.dart b/packages/flutter_tools/lib/src/commands/run.dart index 17651d5f5a..0d7e8d71a8 100644 --- a/packages/flutter_tools/lib/src/commands/run.dart +++ b/packages/flutter_tools/lib/src/commands/run.dart @@ -340,7 +340,8 @@ class RunCommand extends RunCommandBase { // // Do not add more operations to the future. final Completer appStartedTimeRecorder = new Completer.sync(); - appStartedTimeRecorder.future.then( + // This callback can't throw. + appStartedTimeRecorder.future.then( // ignore: unawaited_futures (_) { appStartedTime = clock.now(); } ); diff --git a/packages/flutter_tools/lib/src/devfs.dart b/packages/flutter_tools/lib/src/devfs.dart index b95d1aea7c..938d5deffc 100644 --- a/packages/flutter_tools/lib/src/devfs.dart +++ b/packages/flutter_tools/lib/src/devfs.dart @@ -286,7 +286,8 @@ class _DevFSHttpWriter { } catch (e) { if (retry < kMaxRetries) { printTrace('Retrying writing "$deviceUri" to DevFS due to error: $e'); - _scheduleWrite(deviceUri, content, retry + 1); + // Synchronization is handled by the _completer below. + _scheduleWrite(deviceUri, content, retry + 1); // ignore: unawaited_futures return; } else { printError('Error writing "$deviceUri" to DevFS: $e'); diff --git a/packages/flutter_tools/lib/src/ios/code_signing.dart b/packages/flutter_tools/lib/src/ios/code_signing.dart index e45eda0aba..647015a3ef 100644 --- a/packages/flutter_tools/lib/src/ios/code_signing.dart +++ b/packages/flutter_tools/lib/src/ios/code_signing.dart @@ -142,11 +142,12 @@ Future getCodeSigningIdentityDevelopmentTeam({BuildableIOSApp iosApp, bo ..close(); final String opensslOutput = await UTF8.decodeStream(opensslProcess.stdout); - opensslProcess.stderr.drain(); + // Fire and forget discard of the stderr stream so we don't hold onto resources. + // Don't care about the result. + opensslProcess.stderr.drain(); // ignore: unawaited_futures - if (await opensslProcess.exitCode != 0) { + if (await opensslProcess.exitCode != 0) return null; - } return _certificateOrganizationalUnitExtractionPattern .firstMatch(opensslOutput) diff --git a/packages/flutter_tools/lib/src/ios/simulators.dart b/packages/flutter_tools/lib/src/ios/simulators.dart index be27c60d12..75b5ccb7a5 100644 --- a/packages/flutter_tools/lib/src/ios/simulators.dart +++ b/packages/flutter_tools/lib/src/ios/simulators.dart @@ -237,7 +237,7 @@ class IOSSimulator extends Device { Future installApp(ApplicationPackage app) async { try { final IOSApp iosApp = app; - SimControl.instance.install(id, iosApp.simulatorBundlePath); + await SimControl.instance.install(id, iosApp.simulatorBundlePath); return true; } catch (e) { return false; @@ -382,7 +382,7 @@ class IOSSimulator extends Device { printError('Error waiting for a debug connection: $error'); return new LaunchResult.failed(); } finally { - observatoryDiscovery.cancel(); + await observatoryDiscovery.cancel(); } } @@ -556,7 +556,9 @@ class _IOSSimulatorLogReader extends DeviceLogReader { _systemProcess.stderr.transform(UTF8.decoder).transform(const LineSplitter()).listen(_onSystemLine); } - _deviceProcess.exitCode.whenComplete(() { + // We don't want to wait for the process or its callback. Best effort + // cleanup in the callback. + _deviceProcess.exitCode.whenComplete(() { // ignore: unawaited_futures if (_linesController.hasListener) _linesController.close(); }); diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart index 3e1db2a6ee..2c5f64622e 100644 --- a/packages/flutter_tools/lib/src/resident_runner.dart +++ b/packages/flutter_tools/lib/src/resident_runner.dart @@ -99,8 +99,10 @@ class FlutterDevice { if (flutterViews == null || flutterViews.isEmpty) return; for (FlutterView view in flutterViews) { - if (view != null && view.uiIsolate != null) - view.uiIsolate.flutterExit(); + if (view != null && view.uiIsolate != null) { + // Manage waits specifically below. + view.uiIsolate.flutterExit(); // ignore: unawaited_futures + } } await new Future.delayed(const Duration(milliseconds: 100)); } @@ -563,8 +565,9 @@ abstract class ResidentRunner { } Future stopEchoingDeviceLog() async { - for (FlutterDevice device in flutterDevices) - device.stopEchoingDeviceLog(); + await Future.wait( + flutterDevices.map((FlutterDevice device) => device.stopEchoingDeviceLog()) + ); } /// If the [reloadSources] parameter is not null the 'reloadSources' service @@ -591,7 +594,10 @@ abstract class ResidentRunner { // Listen for service protocol connection to close. for (FlutterDevice device in flutterDevices) { for (VMService service in device.vmServices) { - service.done.then( + // This hooks up callbacks for when the connection stops in the future. + // We don't want to wait for them. We don't handle errors in those callbacks' + // futures either because they just print to logger and is not critical. + service.done.then( // ignore: unawaited_futures _serviceProtocolDone, onError: _serviceProtocolError ).whenComplete(_serviceDisconnected); diff --git a/packages/flutter_tools/lib/src/test/coverage_collector.dart b/packages/flutter_tools/lib/src/test/coverage_collector.dart index 21b80e0bef..1925ed5795 100644 --- a/packages/flutter_tools/lib/src/test/coverage_collector.dart +++ b/packages/flutter_tools/lib/src/test/coverage_collector.dart @@ -42,7 +42,10 @@ class CoverageCollector extends TestWatcher { final int pid = process.pid; int exitCode; - process.exitCode.then((int code) { + // Synchronization is enforced by the API contract. Error handling + // synchronization is done in the code below where `exitCode` is checked. + // Callback cannot throw. + process.exitCode.then((int code) { // ignore: unawaited_futures exitCode = code; }); if (exitCode != null) diff --git a/packages/flutter_tools/lib/src/test/flutter_platform.dart b/packages/flutter_tools/lib/src/test/flutter_platform.dart index e1bd527e57..2f26b6f5da 100644 --- a/packages/flutter_tools/lib/src/test/flutter_platform.dart +++ b/packages/flutter_tools/lib/src/test/flutter_platform.dart @@ -153,7 +153,8 @@ class _FlutterPlatform extends PlatformPlugin { bool subprocessActive = false; bool controllerSinkClosed = false; try { - controller.sink.done.whenComplete(() { controllerSinkClosed = true; }); + // Callback can't throw since it's just setting a variable. + controller.sink.done.whenComplete(() { controllerSinkClosed = true; }); // ignore: unawaited_futures // Prepare our WebSocket server to talk to the engine subproces. final HttpServer server = await HttpServer.bind(host, 0); @@ -272,7 +273,8 @@ class _FlutterPlatform extends PlatformPlugin { subprocessActive = false; final String message = _getErrorMessage(_getExitCodeMessage(exitCode, 'before connecting to test harness'), testPath, shellPath); controller.sink.addError(message); - controller.sink.close(); + // Awaited for with 'sink.done' below. + controller.sink.close(); // ignore: unawaited_futures printTrace('test $ourTestCount: waiting for controller sink to close'); await controller.sink.done; break; @@ -280,7 +282,8 @@ class _FlutterPlatform extends PlatformPlugin { printTrace('test $ourTestCount: timed out waiting for process with pid ${process.pid} to connect to test harness'); final String message = _getErrorMessage('Test never connected to test harness.', testPath, shellPath); controller.sink.addError(message); - controller.sink.close(); + // Awaited for with 'sink.done' below. + controller.sink.close(); // ignore: unawaited_futures printTrace('test $ourTestCount: waiting for controller sink to close'); await controller.sink.done; break; @@ -332,8 +335,10 @@ class _FlutterPlatform extends PlatformPlugin { testDone.future.then<_TestResult>((Null _) { return _TestResult.testBailed; }), ]); - harnessToTest.cancel(); - testToHarness.cancel(); + await Future.wait(>[ + harnessToTest.cancel(), + testToHarness.cancel(), + ]); switch (testResult) { case _TestResult.crashed: @@ -342,7 +347,8 @@ class _FlutterPlatform extends PlatformPlugin { subprocessActive = false; final String message = _getErrorMessage(_getExitCodeMessage(exitCode, 'before test harness closed its WebSocket'), testPath, shellPath); controller.sink.addError(message); - controller.sink.close(); + // Awaited for with 'sink.done' below. + controller.sink.close(); // ignore: unawaited_futures printTrace('test $ourTestCount: waiting for controller sink to close'); await controller.sink.done; break; @@ -384,7 +390,8 @@ class _FlutterPlatform extends PlatformPlugin { } } if (!controllerSinkClosed) { - controller.sink.close(); + // Waiting below with await. + controller.sink.close(); // ignore: unawaited_futures printTrace('test $ourTestCount: waiting for controller sink to close'); await controller.sink.done; }