Fix bug in Isolate.run.

Failed to close the receive-port if `Isolate.spawn` threw
asynchronously, which would keep the isolate alive forever.

Fixes #48516

BUG= http://dartbug.com/48516

Change-Id: If53a868bcc6a11aef9123e95a7dfe47d25932c4c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/235603
Reviewed-by: Nate Bosch <nbosch@google.com>
Commit-Queue: Lasse Nielsen <lrn@google.com>

pkg/async_helper/lib/async_helper.dart

@@ -105,31 +105,38 @@ Future<void> asyncTest(f()) {
 /// failed test expectation, that error cannot be caught and accepted.
 Future<T> asyncExpectThrows<T extends Object>(Future<void> result,
     [String reason = ""]) {
-  // Handle null being passed in from legacy code while also avoiding producing
-  // an unnecessary null check warning here.
-  if ((reason as dynamic) == null) reason = "";
-
-  var type = "";
-  if (T != dynamic && T != Object) type = "<$T>";
-  var header = "asyncExpectThrows$type(${reason}):";
-
-  if ((result as dynamic) == null) {
-    Expect.testError("$header result Future must not be null.");
+  // Delay computing the header text until the test has failed.
+  // The header computation uses complicated language features,
+  // and language tests should avoid doing complicated things
+  // until after the actual test has had a chance to succeed.
+  String header() {
+    // Handle null being passed in from legacy code
+    // while also avoiding producing an unnecessary null check warning here.
+    if ((reason as dynamic) == null) reason = "";
+    // Only include the type in he message if it's not a top-type.
+    var type = Object() is! T ? "<$T>" : "";
+    return "asyncExpectThrows$type($reason):";
   }
+
+  // Unsound null-safety check.
+  if ((result as dynamic) == null) {
+    Expect.testError("${header()} result Future must not be null.");
+  }
+
   // TODO(rnystrom): It might useful to validate that T is not bound to
   // ExpectException since that won't work.
 
   asyncStart();
   return result.then<T>((_) {
-    throw ExpectException("$header Did not throw.");
+    throw ExpectException("${header()} Did not throw.");
   }, onError: (error, stack) {
-    // A test failure doesn't count as throwing.
+    // A test failure doesn't count as throwing. Rethrow it.
     if (error is ExpectException) throw error;
 
     if (error is! T) {
       // Throws something unexpected.
       throw ExpectException(
-          "$header Unexpected '${Error.safeToString(error)}'\n$stack");
+          "${header()} Unexpected '${Error.safeToString(error)}'\n$stack");
     }
 
     asyncEnd();

sdk/lib/isolate/isolate.dart

@@ -235,9 +235,16 @@ class Isolate {
               onExit: resultPort.sendPort,
               errorsAreFatal: true,
               debugName: debugName)
-          .then<void>((_) {}, onError: result.completeError);
+          .then<void>((_) {}, onError: (error, stack) {
+        // Sending the computation failed asynchronously.
+        // Do not expect a response, report the error asynchronously.
+        resultPort.close();
+        result.completeError(error, stack);
+      });
     } on Object {
-      // Sending the computation failed.
+      // Sending the computation failed synchronously.
+      // This is not expected to happen, but if it does,
+      // the synchronous error is respected and rethrown synchronously.
       resultPort.close();
       rethrow;
     }
@@ -938,7 +945,7 @@ class _RemoteRunner<R> {
       if (potentiallyAsyncResult is Future<R>) {
         result = await potentiallyAsyncResult;
       } else {
-        result = potentiallyAsyncResult as R;
+        result = potentiallyAsyncResult;
       }
     } catch (e, s) {
       // If sending fails, the error becomes an uncaught error.

tests/lib/isolate/isolate_run_test.dart

@@ -21,6 +21,8 @@ void main() async {
   await testIsolateHangs();
   await testIsolateKilled();
   await testIsolateExits();
+  // Failing to start.
+  await testInvalidMessage();
   asyncEnd();
 }
 
@@ -113,3 +115,11 @@ Future<void> testIsolateExits() async {
   Expect.equals("Computation ended without result", e.toString());
   Expect.equals(0, variable);
 }
+
+Future<void> testInvalidMessage() async {
+  // Regression test for http://dartbug.com/48516
+  var unsendable = RawReceivePort();
+  await asyncExpectThrows<Error>(Isolate.run<void>(() => unsendable));
+  unsendable.close();
+  // Test should not hang.
+}

tests/lib_2/isolate/isolate_run_test.dart

@@ -23,6 +23,8 @@ void main() async {
   await testIsolateHangs();
   await testIsolateKilled();
   await testIsolateExits();
+  // Failing to start.
+  await testInvalidMessage();
   asyncEnd();
 }
 
@@ -115,3 +117,11 @@ Future<void> testIsolateExits() async {
   Expect.equals("Computation ended without result", e.toString());
   Expect.equals(0, variable);
 }
+
+Future<void> testInvalidMessage() async {
+  // Regression test for http://dartbug.com/48516
+  var unsendable = RawReceivePort();
+  await asyncExpectThrows<Error>(Isolate.run<void>(() => unsendable));
+  unsendable.close();
+  // Test should not hang.
+}