Calling one async function from tail position in another escapes enclosing try blocks

[stereotype441]

The analysis server's PreviewSite.handleGetRequest function attempts to catch exceptions occuring while handling a get request, so that an error response may be returned instead:

  Future<void> handleGetRequest(HttpRequest request) async {
    ...
    try {
      ...
      return respond(request, DartFilePage(this, unitInfo)); // (1)
      ...
    } catch (exception, stackTrace) { // (2)
      try {
        await respond(
            request,
            createExceptionPageWithPath(path, '$exception', stackTrace),
            HttpStatus.internalServerError);
      } catch (exception, stackTrace) {
        ...
      }
    }
  }

Where the respond method is itself asynchronous:

  Future<void> respond(HttpRequest request, Page page,
      [int code = HttpStatus.ok]) async {
    HttpResponse response = request.response;
    response.statusCode = code;
    ...
    response.write(await page.generate(request.uri.queryParameters));
    response.close();
  }

The intended behavior here is that when an HttpRequest is received, a DartFilePage object is created and passed to respond. Then respond calls its generate method to generate HTML text. If an exception occurs during page.generate, then it should be caught by the catch block at (2), and so the HttpRequest should be completed with an error response. But that doesn't happen. What happens instead that the HttpRequest is never completed, so the analysis server appears to hang.

After a lot of single stepping in the debugger and scratching my head, I've figured out the reason the code isn't behaving as intended: since there is no await keyword at (1), as soon as a Future is received from the call to respond, that future is returned from handleGetRequest, and then the caller waits for it to complete. If respond's call to page.generate, encounters an exception, that occurs after handleGetRequest has already exited, so its try/catch is no longer in scope, and the exception propagates to the caller.

I've been able to get the intended behavior by changing (1) to use return await rather than just return. This ensures that handleGetRequest actually waits for the future returned by respond to complete, so that if an exception occurs during page.generate, the try/catch will still be in scope, and the intended exception page will be generated.

I think there's a bit of a user trap here. Usually return await and return are equivalent (because of the "implicit await" behavior of return statements inside of async functions) , so I've gotten into the habit of changing one into the other without really thinking carefully. But when the return statement is inside of a try block, the behavior is very different, and shortening return await to just return is almost certainly a big mistake.

I don't really have a concrete proposal for what to do about this--perhaps the best answer is just to have a lint that detects this problematic pattern (cc @pq). But I figured I would start by reaching out to the language folks to see if you have any other ideas.

[stereotype441]

Thinking about this a little bit from a language design perspective, I found my self wondering why we didn't just specify return inside of an async function to always implicitly await the returned expression. I think the reason is that by not awaiting an expression whose type is Future, we are essentially doing the equivalent of a tail call optimization. But tail call optimizations aren't safe inside of try blocks (because of scenarios like this one).

So I actually think I do have a concrete proposal for what to do about this: What if we change the language so that if a return appears in an async function inside of a try block, it always does an implicit await, even if the type of the expression being returned is already a subtype of Future? I suspect that the behavioral change for most programs would be very small (since it only affects return statements inside of try blocks in async functions, and the behavioral difference is only visible if an exception occurs), and I suspect that of those programs that are affected, in the vast majority of cases the effect would be beneficial (because the programmer's intention is probably for the exception to be caught).

[lrhn]

This is a known bug/missing fix: dart-lang/sdk#39204
And yes, a return in an async function should always await the future it returns (it's just only visible whether it does so inside a try/catch).

It's not about tail-calling, you get the same effect with:

Future<int> foo() async {
  try {
    return Future.error("Gotcha");
  } catch (e) {
    return -1;
  }
}

It's the returning of a future containing an error directly that bypasses the catch.

[MichaelRFairhurst]

Yeah, this looks very much looks like an unsafe optimization to me. The comparison to tail call optimization is a good one.

[lrhn]

The historical reason for the behavior is that the implementation used a completer and just completed it with the value of the return expression. That's why you could return both a future and a value at all - completer.complete accepted both, and since it was Dart 1, nobody checked the types in production mode anyway. So, eventually it became "the behavior" and was specified.

This issue has cropped up a few times, typically around synchronous functions that become asynchronous, which is why I changed the specification around the Dart 2.0 time to require the await for returns that return a Future<T>. That should ensure that a function wrapped in a try/catch won't actually throw asynchronously.

[lrhn]

This is a solved problem from the language perspective, we just need the implementations to implement the desired behavior. Marking this as a duplicate of dart-lang/sdk#39204.