Catch errors thrown into the Zone by json_rpc (#38486)

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'dart:async';
+
+/// Runs [fn] with special handling of asynchronous errors.
+///
+/// If the execution of [fn] does not throw a synchronous exception, and if the
+/// [Future] returned by [fn] is completed with a value, then the [Future]
+/// 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] 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.
+///
+/// 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
+/// synchronous and asynchronous are ignored.
+///
+/// Rationale:
+///
+/// Consider the following snippet:
+/// ```
+/// try {
+///   await foo();
+///   ...
+/// } catch (e) {
+///   ...
+/// }
+/// ```
+/// If the [Future] returned by `foo` is completed with an error, that error is
+/// handled by the catch block. However, if `foo` spawns an asynchronous
+/// operation whose errors are unhandled, those errors will not be caught by
+/// the catch block, and will instead propagate to the containing [Zone]. This
+/// behavior is non-intuitive to programmers expecting the `catch` to catch all
+/// the errors resulting from the code under the `try`.
+///
+/// As such, it would be convenient if the `try {} catch {}` here could handle
+/// not only errors completing the awaited [Future]s it contains, but also
+/// any otherwise unhandled asynchronous errors occuring as a result of awaited
+/// expressions. This is how `await` is often assumed to work, which leads to
+/// unexpected unhandled exceptions.
+///
+/// [asyncGuard] is intended to wrap awaited expressions occuring in a `try`
+/// block. The behavior described above gives the behavior that users
+/// intuitively expect from `await`. Consider the snippet:
+/// ```
+/// try {
+///   await asyncGuard(() async {
+///     var c = Completer();
+///     c.completeError('Error');
+///   });
+/// } catch (e) {
+///   // e is 'Error';
+/// }
+/// ```
+/// 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) {
+  final Completer<T> completer = Completer<T>();
+
+  runZoned<void>(() async {
+    try {
+      final T result = await fn();
+      if (!completer.isCompleted) {
+        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);
+    }
+  });
+
+  return completer.future;
+}

packages/flutter_tools/lib/src/vmservice.dart

@@ -13,6 +13,7 @@ import 'package:stream_channel/stream_channel.dart';
 import 'package:web_socket_channel/io.dart';
 import 'package:web_socket_channel/web_socket_channel.dart';
 
+import 'base/async_guard.dart';
 import 'base/common.dart';
 import 'base/context.dart';
 import 'base/file_system.dart';
@@ -313,8 +314,8 @@ class VMService {
     Map<String, dynamic> params,
   ) {
     return Future.any<Map<String, dynamic>>(<Future<Map<String, dynamic>>>[
-      _peer.sendRequest(method, params).then<Map<String, dynamic>>(castStringKeyedMap),
-      _connectionError.future,
+        _peer.sendRequest(method, params).then<Map<String, dynamic>>(castStringKeyedMap),
+        _connectionError.future,
     ]);
   }
 
@@ -358,7 +359,7 @@ class VMService {
   Future<void> _streamListen(String streamId) async {
     if (!_listeningFor.contains(streamId)) {
       _listeningFor.add(streamId);
-      await _sendRequest('streamListen', <String, dynamic>{'streamId': streamId});
+      await asyncGuard(() => _sendRequest('streamListen', <String, dynamic>{'streamId': streamId}));
     }
   }
 

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'dart:async';
+
+import 'package:flutter_tools/src/base/async_guard.dart';
+
+import '../../src/common.dart';
+
+Future<void> asyncError() {
+  final Completer<void> completer = Completer<void>();
+  final Completer<void> errorCompleter = Completer<void>();
+  errorCompleter.completeError('Async Doom', StackTrace.current);
+  return completer.future;
+}
+
+Future<void> syncError() {
+  throw 'Sync Doom';
+}
+
+Future<void> syncAndAsyncError() {
+  final Completer<void> errorCompleter = Completer<void>();
+  errorCompleter.completeError('Async Doom', StackTrace.current);
+  throw 'Sync Doom';
+}
+
+void main() {
+  Completer<void> caughtInZone;
+  bool caughtByZone = false;
+  bool caughtByHandler = false;
+  Zone zone;
+
+  setUp(() {
+    caughtInZone = Completer<void>();
+    caughtByZone = false;
+    caughtByHandler = false;
+    zone = Zone.current.fork(specification: ZoneSpecification(
+      handleUncaughtError: (
+          Zone self,
+          ZoneDelegate parent,
+          Zone zone,
+          Object error,
+          StackTrace stackTrace,
+          ) {
+        caughtByZone = true;
+        if (!caughtInZone.isCompleted) {
+          caughtInZone.complete();
+        }
+      },
+    ));
+  });
+
+  test('asyncError percolates through zone', () async {
+    await zone.run(() async {
+      try {
+        // Completer is required or else we timeout.
+        await Future.any(<Future<void>>[asyncError(), caughtInZone.future]);
+      } on String {
+        caughtByHandler = true;
+      }
+    });
+
+    expect(caughtByZone, true);
+    expect(caughtByHandler, false);
+  });
+
+  test('syncAndAsyncError percolates through zone', () async {
+    await zone.run(() async {
+      try {
+        // Completer is required or else we timeout.
+        await Future.any(<Future<void>>[syncAndAsyncError(), caughtInZone.future]);
+      } on String {
+        caughtByHandler = true;
+      }
+    });
+
+    expect(caughtByZone, true);
+    expect(caughtByHandler, true);
+  });
+
+  test('syncError percolates through zone', () async {
+    await zone.run(() async {
+      try {
+        await syncError();
+      } on String {
+        caughtByHandler = true;
+      }
+    });
+
+    expect(caughtByZone, false);
+    expect(caughtByHandler, true);
+  });
+
+  test('syncError is caught by asyncGuard', () async {
+    await zone.run(() async {
+      try {
+        await asyncGuard(syncError);
+      } on String {
+        caughtByHandler = true;
+      }
+    });
+
+    expect(caughtByZone, false);
+    expect(caughtByHandler, true);
+  });
+
+
+  test('asyncError is caught by asyncGuard', () async {
+    await zone.run(() async {
+      try {
+        await asyncGuard(asyncError);
+      } on String {
+        caughtByHandler = true;
+      }
+    });
+
+    expect(caughtByZone, false);
+    expect(caughtByHandler, true);
+  });
+
+  test('asyncAndSyncError is caught by asyncGuard', () async {
+    await zone.run(() async {
+      try {
+        await asyncGuard(syncAndAsyncError);
+      } on String {
+        caughtByHandler = true;
+      }
+    });
+
+    expect(caughtByZone, false);
+    expect(caughtByHandler, true);
+  });
+}