[flutter_tool] Add onError callback to asyncGuard. Use it in Doctor (#39445)

packages/flutter_tools/lib/src/base/async_guard.dart

@@ -11,14 +11,19 @@ import 'dart:async';
 /// returned by [asyncGuard] is completed with that value if it has not already
 /// been completed with an error.
 ///
-/// If the execution of [fn] throws a synchronous exception, then the [Future]
-/// returned by [asyncGuard] is completed with an error whose object and
-/// stack trace are given by the synchronous exception.
+/// If the execution of [fn] throws a synchronous exception, and no [onError]
+/// callback is provided, then the [Future] returned by [asyncGuard] is
+/// completed with an error whose object and stack trace are given by the
+/// synchronous exception. If an [onError] callback is provided, then the
+/// [Future] returned by [asyncGuard] is completed with its result when passed
+/// the error object and stack trace.
 ///
 /// If the execution of [fn] results in an asynchronous exception that would
-/// otherwise be unhandled, then the [Future] returned by [asyncGuard] is
-/// completed with an error whose object and stack trace are given by the
-/// asynchronous exception.
+/// otherwise be unhandled, and no [onError] callback is provided, then the
+/// [Future] returned by [asyncGuard] is completed with an error whose object
+/// and stack trace are given by the asynchronous exception. If an [onError]
+/// callback is provided, then the [Future] returned by [asyncGuard] is
+/// completed with its result when passed the error object and stack trace.
 ///
 /// After the returned [Future] is completed, whether it be with a value or an
 /// error, all further errors resulting from the execution of [fn] both
@@ -64,9 +69,43 @@ import 'dart:async';
 /// Without the [asyncGuard] the error 'Error' would be propagated to the
 /// error handler of the containing [Zone]. With the [asyncGuard], the error
 /// 'Error' is instead caught by the `catch`.
-Future<T> asyncGuard<T>(Future<T> Function() fn) {
+///
+/// [asyncGuard] also accepts an [onError] callback for situations in which
+/// completing the returned [Future] with an error is not appropriate.
+/// For example, it is not always possible to immediately await the returned
+/// [Future]. In these cases, an [onError] callback is needed to prevent an
+/// error from propagating to the containing [Zone].
+///
+/// [onError] must have type `FutureOr<T> Function(Object error)` or
+/// `FutureOr<T> Function(Object error, StackTrace stackTrace)` otherwise an
+/// [ArgumentError] will be thrown synchronously.
+Future<T> asyncGuard<T>(Future<T> Function() fn, {
+    Function onError,
+  }) {
+  if (onError != null &&
+      onError is! _UnaryOnError<T> &&
+      onError is! _BinaryOnError<T>) {
+    throw ArgumentError('onError must be a unary function accepting an Object, '
+                        'or a binary function accepting an Object and '
+                        'StackTrace. onError must return a T');
+  }
   final Completer<T> completer = Completer<T>();
 
+  void handleError(Object e, StackTrace s) {
+    if (completer.isCompleted) {
+      return;
+    }
+    if (onError == null) {
+      completer.completeError(e, s);
+      return;
+    }
+    if (onError is _UnaryOnError) {
+      completer.complete(onError(e));
+    } else if (onError is _BinaryOnError) {
+      completer.complete(onError(e, s));
+    }
+  }
+
   runZoned<void>(() async {
     try {
       final T result = await fn();
@@ -74,15 +113,14 @@ Future<T> asyncGuard<T>(Future<T> Function() fn) {
         completer.complete(result);
       }
     } catch (e, s) {
-      if (!completer.isCompleted) {
-        completer.completeError(e, s);
-      }
-    }
-  }, onError: (dynamic e, StackTrace s) {
-    if (!completer.isCompleted) {
-      completer.completeError(e, s);
+      handleError(e, s);
     }
+  }, onError: (Object e, StackTrace s) {
+    handleError(e, s);
   });
 
   return completer.future;
 }
+
+typedef _UnaryOnError<T> = FutureOr<T> Function(Object error);
+typedef _BinaryOnError<T> = FutureOr<T> Function(Object error, StackTrace stackTrace);

packages/flutter_tools/lib/src/doctor.dart

@@ -143,8 +143,15 @@ class Doctor {
   List<ValidatorTask> startValidatorTasks() {
     final List<ValidatorTask> tasks = <ValidatorTask>[];
     for (DoctorValidator validator in validators) {
+      // We use an asyncGuard() here to be absolutely certain that
+      // DoctorValidators do not result in an uncaught exception. Since the
+      // Future returned by the asyncGuard() is not awaited, we pass an
+      // onError callback to it and translate errors into ValidationResults.
       final Future<ValidationResult> result =
-          asyncGuard<ValidationResult>(() => validator.validate());
+          asyncGuard<ValidationResult>(validator.validate,
+            onError: (Object exception, StackTrace stackTrace) {
+              return ValidationResult.crash(exception, stackTrace);
+            });
       tasks.add(ValidatorTask(validator, result));
     }
     return tasks;

packages/flutter_tools/test/general.shard/base/async_guard_test.dart

@@ -5,6 +5,8 @@
 import 'dart:async';
 
 import 'package:flutter_tools/src/base/async_guard.dart';
+import 'package:flutter_tools/src/base/common.dart';
+import 'package:quiver/testing/async.dart';
 
 import '../../src/common.dart';
 
@@ -25,6 +27,17 @@ Future<void> syncAndAsyncError() {
   throw 'Sync Doom';
 }
 
+Future<void> delayedThrow(FakeAsync time) {
+  final Future<void> result =
+      Future<void>.delayed(const Duration(milliseconds: 10))
+      .then((_) {
+    throw 'Delayed Doom';
+  });
+  time.elapse(const Duration(seconds: 1));
+  time.flushMicrotasks();
+  return result;
+}
+
 void main() {
   Completer<void> caughtInZone;
   bool caughtByZone = false;
@@ -131,4 +144,76 @@ void main() {
     expect(caughtByZone, false);
     expect(caughtByHandler, true);
   });
+
+  test('asyncError is missed when catchError is attached too late', () async {
+    bool caughtByZone = false;
+    bool caughtByHandler = false;
+    bool caughtByCatchError = false;
+
+    final Completer<void> completer = Completer<void>();
+    await FakeAsync().run((FakeAsync time) {
+      unawaited(runZoned(() async {
+        final Future<void> f = asyncGuard<void>(() => delayedThrow(time))
+            .catchError((Object e, StackTrace s) {
+          caughtByCatchError = true;
+        });
+        try {
+          await f;
+        } on String {
+          caughtByHandler = true;
+        }
+        if (!completer.isCompleted) {
+          completer.complete(null);
+        }
+      }, onError: (Object e, StackTrace s) {
+        caughtByZone = true;
+        if (!completer.isCompleted) {
+          completer.complete(null);
+        }
+      }));
+      time.elapse(const Duration(seconds: 1));
+      time.flushMicrotasks();
+      return completer.future;
+    });
+
+    expect(caughtByZone, true);
+    expect(caughtByHandler, false);
+    expect(caughtByCatchError, true);
+  });
+
+  test('asyncError is propagated correctly with onError callback', () async {
+    bool caughtByZone = false;
+    bool caughtByHandler = false;
+    bool caughtByOnError = false;
+
+    final Completer<void> completer = Completer<void>();
+    await FakeAsync().run((FakeAsync time) {
+      unawaited(runZoned(() async {
+        final Future<void> f = asyncGuard<void>(() => delayedThrow(time),
+            onError: (Object e, StackTrace s) {
+          caughtByOnError = true;
+        });
+        try {
+          await f;
+        } catch (e) {
+          caughtByHandler = true;
+        }
+        if (!completer.isCompleted) {
+          completer.complete(null);
+        }
+      }, onError: (Object e, StackTrace s) {
+        caughtByZone = true;
+        if (!completer.isCompleted) {
+          completer.complete(null);
+        }
+      }));
+      time.elapse(const Duration(seconds: 1));
+      time.flushMicrotasks();
+      return completer.future;
+    });
+
+    expect(caughtByZone, false);
+    expect(caughtByHandler, false);
+    expect(caughtByOnError, true);
+  });
 }

packages/flutter_tools/test/general.shard/commands/doctor_test.dart

@@ -4,24 +4,25 @@
 
 import 'dart:async';
 
-import 'package:flutter_tools/src/base/process_manager.dart';
-import 'package:flutter_tools/src/features.dart';
-import 'package:flutter_tools/src/web/workflow.dart';
-import 'package:mockito/mockito.dart';
-import 'package:process/process.dart';
-
 import 'package:flutter_tools/src/artifacts.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/platform.dart';
+import 'package:flutter_tools/src/base/process_manager.dart';
 import 'package:flutter_tools/src/base/terminal.dart';
 import 'package:flutter_tools/src/base/user_messages.dart';
 import 'package:flutter_tools/src/doctor.dart';
+import 'package:flutter_tools/src/features.dart';
 import 'package:flutter_tools/src/globals.dart';
 import 'package:flutter_tools/src/proxy_validator.dart';
 import 'package:flutter_tools/src/reporting/reporting.dart';
 import 'package:flutter_tools/src/vscode/vscode.dart';
 import 'package:flutter_tools/src/vscode/vscode_validator.dart';
+import 'package:flutter_tools/src/web/workflow.dart';
+import 'package:mockito/mockito.dart';
+import 'package:process/process.dart';
+import 'package:quiver/testing/async.dart';
 
 import '../../src/common.dart';
 import '../../src/context.dart';
@@ -334,6 +335,34 @@ void main() {
       expect(testLogger.statusText, contains('#0      CrashingValidator.validate'));
     }, overrides: noColorTerminalOverride);
 
+
+    testUsingContext('validate non-verbose output format for run with an async crash', () async {
+      final Completer<void> completer = Completer<void>();
+      await FakeAsync().run((FakeAsync time) {
+        unawaited(FakeAsyncCrashingDoctor(time).diagnose(verbose: false).then((bool r) {
+          expect(r, isFalse);
+          completer.complete(null);
+        }));
+        time.elapse(const Duration(seconds: 1));
+        time.flushMicrotasks();
+        return completer.future;
+      });
+      expect(testLogger.statusText, equals(
+              'Doctor summary (to see all details, run flutter doctor -v):\n'
+              '[✓] Passing Validator (with statusInfo)\n'
+              '[✓] Another Passing Validator (with statusInfo)\n'
+              '[☠] Async crashing validator (the doctor check crashed)\n'
+              '    ✗ Due to an error, the doctor check did not complete. If the error message below is not helpful, '
+              'please let us know about this issue at https://github.com/flutter/flutter/issues.\n'
+              '    ✗ fatal error\n'
+              '[✓] Validators are fun (with statusInfo)\n'
+              '[✓] Four score and seven validators ago (with statusInfo)\n'
+              '\n'
+              '! Doctor found issues in 1 category.\n'
+      ));
+    }, overrides: noColorTerminalOverride);
+
+
     testUsingContext('validate non-verbose output format when only one category fails', () async {
       expect(await FakeSinglePassingDoctor().diagnose(verbose: false), isTrue);
       expect(testLogger.statusText, equals(
@@ -691,6 +720,24 @@ class CrashingValidator extends DoctorValidator {
   }
 }
 
+class AsyncCrashingValidator extends DoctorValidator {
+  AsyncCrashingValidator(this._time) : super('Async crashing validator');
+
+  final FakeAsync _time;
+
+  @override
+  Future<ValidationResult> validate() {
+    const Duration delay = Duration(seconds: 1);
+    final Future<ValidationResult> result = Future<ValidationResult>
+        .delayed(delay).then((_) {
+      throw 'fatal error';
+    });
+    _time.elapse(const Duration(seconds: 1));
+    _time.flushMicrotasks();
+    return result;
+  }
+}
+
 /// A doctor that fails with a missing [ValidationResult].
 class FakeDoctor extends Doctor {
   List<DoctorValidator> _validators;
@@ -772,6 +819,27 @@ class FakeCrashingDoctor extends Doctor {
   }
 }
 
+/// A doctor with a validator that throws an exception.
+class FakeAsyncCrashingDoctor extends Doctor {
+  FakeAsyncCrashingDoctor(this._time);
+
+  final FakeAsync _time;
+
+  List<DoctorValidator> _validators;
+  @override
+  List<DoctorValidator> get validators {
+    if (_validators == null) {
+      _validators = <DoctorValidator>[];
+      _validators.add(PassingValidator('Passing Validator'));
+      _validators.add(PassingValidator('Another Passing Validator'));
+      _validators.add(AsyncCrashingValidator(_time));
+      _validators.add(PassingValidator('Validators are fun'));
+      _validators.add(PassingValidator('Four score and seven validators ago'));
+    }
+    return _validators;
+  }
+}
+
 /// A DoctorValidatorsProvider that overrides the default validators without
 /// overriding the doctor.
 class FakeDoctorValidatorsProvider implements DoctorValidatorsProvider {