dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.26k stars 1.58k forks source link

whenComplete(...).ignore() does not silence an error if the callback has an asynchronous gap #56806

Closed alexeyinkin closed 1 month ago

alexeyinkin commented 1 month ago

This example throws:

void main() async {
  final future = Future<void>.delayed(
    Duration(seconds: 1),
    () => throw Exception(),
  );

  future.whenComplete(() async {
    print('whenComplete()');
    await Future.delayed(Duration.zero); // This line prevents the silencing.
  }).ignore();
}
% dart main.dart
whenComplete()
Unhandled exception:
Exception
#0      main.<anonymous closure> (package:my_package/main.dart:4:11)
#1      new Future.delayed.<anonymous closure> (dart:async/future.dart:431:42)
#2      Timer._createTimer.<anonymous closure> (dart:async-patch/timer_patch.dart:18:15)
#3      _Timer._runTimers (dart:isolate-patch/timer_impl.dart:398:19)
#4      _Timer._handleMessage (dart:isolate-patch/timer_impl.dart:429:5)
#5      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

However, if the asynchronous gap is removed from the callback, the error is ignored as expected:

void main() async {
  final future = Future<void>.delayed(
    Duration(seconds: 1),
    () => throw Exception(),
  );

  future.whenComplete(() async {
    print('whenComplete()'); // No gap.
  }).ignore();

  await Future.delayed(Duration(seconds: 1));
}
% dart main.dart
whenComplete()

I consider this a bug because this behaviour contradicts the doc of whenComplete method which says it's equivalent to the following:

Future<T> whenComplete(action()) {
  return this.then((v) {
    var f2 = action();
    if (f2 is Future) return f2.then((_) => v);
    return v;
  }, onError: (e) {
    var f2 = action();
    if (f2 is Future) return f2.then((_) { throw e; });
    throw e;
  });
}

When using this equivalent, ignore() successfully silences the error even with an asynchronous gap in the callback:

void main() async {
  final future = Future<void>.delayed(
    Duration(seconds: 1),
    () => throw Exception(),
  );

  future.whenCompleteEquivalent(() async {
    print('whenComplete()');
    await Future.delayed(Duration.zero);
  }).ignore();
}

extension FutureExtension<T> on Future<T> {
  Future<T> whenCompleteEquivalent(action()) {
    return this.then((v) {
      var f2 = action();
      if (f2 is Future) return f2.then((_) => v);
      return v;
    }, onError: (e) {
      var f2 = action();
      if (f2 is Future) return f2.then((_) {
        throw e;
      });
      throw e;
    });
  }
}
% dart main.dart
whenComplete()

Also, it just feels wrong. When passing a callback to whenComplete(), it's impossible to tell if it has an asynchronous gap, and untestable things should not be such a strong factor in a deterministic API like this one.

I can reproduce this in both stable and dev.

dart info (3.5.3 stable)

``` If providing this information as part of reporting a bug, please review the information below to ensure it only contains things you're comfortable posting publicly. #### General info - Dart 3.5.3 (stable) (Wed Sep 11 16:22:47 2024 +0000) on "macos_arm64" - on macos / Version 13.6 (Build 22G120) - locale is es-GE #### Process info | Memory | CPU | Elapsed time | Command line | | -----: | ---: | -----------: | ------------------------------------------------------------------------------------------ | | 28 MB | 0.0% | 03-07:03:12 | dart devtools --machine --dtd-uri=ws:/XEjeVjO68XdEYq9P | | 28 MB | 0.0% | 03-05:45:11 | dart devtools --machine --dtd-uri=ws:/s7qnnoiffl3kKD12 | | 24 MB | 0.0% | 04-01:24:03 | dart devtools --machine --dtd-uri=ws:/tYaTy5CYayArivx6 | | 28 MB | 0.0% | 03-06:54:10 | dart devtools --machine --dtd-uri=ws:/x4l4KEhjBLqhKJCY | | 155 MB | 0.0% | 04-01:24:03 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 366 MB | 0.0% | 03-07:03:14 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 299 MB | 0.0% | 03-06:54:12 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 506 MB | 0.0% | 03-05:45:12 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 27 MB | 0.0% | 04-01:24:03 | dart tooling-daemon --machine | | 29 MB | 0.0% | 03-07:03:12 | dart tooling-daemon --machine | | 29 MB | 0.0% | 03-06:54:11 | dart tooling-daemon --machine | | 29 MB | 0.0% | 03-05:45:11 | dart tooling-daemon --machine | | 47 MB | 0.0% | 04-01:24:08 | flutter_tools.snapshot daemon | | 47 MB | 0.0% | 03-07:03:17 | flutter_tools.snapshot daemon | | 47 MB | 0.0% | 03-06:54:12 | flutter_tools.snapshot daemon | | 47 MB | 0.0% | 03-05:45:12 | flutter_tools.snapshot daemon | ```

dart info (3.6.0-277.0.dev)

``` If providing this information as part of reporting a bug, please review the information below to ensure it only contains things you're comfortable posting publicly. #### General info - Dart 3.6.0-277.0.dev (dev) (Tue Sep 24 21:06:37 2024 -0700) on "macos_arm64" - on macos / Version 13.6 (Build 22G120) - locale is es-GE #### Process info | Memory | CPU | Elapsed time | Command line | | -----: | ---: | -----------: | ------------------------------------------------------------------------------------------ | | 28 MB | 0.0% | 03-07:04:51 | dart devtools --machine --dtd-uri=ws:/XEjeVjO68XdEYq9P | | 28 MB | 0.0% | 03-05:46:50 | dart devtools --machine --dtd-uri=ws:/s7qnnoiffl3kKD12 | | 24 MB | 0.0% | 04-01:25:42 | dart devtools --machine --dtd-uri=ws:/tYaTy5CYayArivx6 | | 28 MB | 0.0% | 03-06:55:49 | dart devtools --machine --dtd-uri=ws:/x4l4KEhjBLqhKJCY | | 155 MB | 0.0% | 04-01:25:42 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 366 MB | 0.0% | 03-07:04:53 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 299 MB | 0.0% | 03-06:55:51 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 506 MB | 0.0% | 03-05:46:51 | dart language-server --client-id=Android-Studio --client-version=AI-241.15989.150 --protocol=analyzer | | 27 MB | 0.0% | 04-01:25:42 | dart tooling-daemon --machine | | 29 MB | 0.0% | 03-07:04:51 | dart tooling-daemon --machine | | 29 MB | 0.0% | 03-06:55:50 | dart tooling-daemon --machine | | 29 MB | 0.0% | 03-05:46:50 | dart tooling-daemon --machine | | 47 MB | 0.0% | 04-01:25:47 | flutter_tools.snapshot daemon | | 47 MB | 0.0% | 03-07:04:56 | flutter_tools.snapshot daemon | | 47 MB | 0.0% | 03-06:55:51 | flutter_tools.snapshot daemon | | 47 MB | 0.0% | 03-05:46:51 | flutter_tools.snapshot daemon | ```

dart-github-bot commented 1 month ago

Summary: The whenComplete() method does not silence an error if the callback has an asynchronous gap, contradicting the documentation and expected behavior. This inconsistency makes it difficult to predict the outcome of using whenComplete() and ignore().

lrhn commented 1 month ago

Does look like a bug, as if the original future is considered unhandled, rather than the future returned by whenComplete. If I change the code to

void main() async {
  final future = Future<void>.delayed(
    Duration(seconds: 1),
    () => throw Exception(),
  );

  future.ignore();
  future.whenComplete(() async {
    print('whenComplete()');
    await Future.delayed(Duration.zero); // This line prevents the silencing.
    print('/whenComplete()');
  });
}

the error goes away, which it shouldn't. The future returned by future.whenComplete still completes with an error, and it isn't handled.

Probably an optimization that ends up returning the original Future since it has the same result. Likely related to chaining.

When one future/completer is completed with a another not-yet-completed future, all listeners of the former are moved over to be listeners on the latter. Which means that the lack of listeners is also copied over. Ignoring a chained future should also ignore the future it's chained to. I think I fixed something related to that already, but there may be another case to handle around whenComplete, which does precisely complete the returned future with the original future if the inner computation doesn't throw, and when the inner computation is sync, it does so before you call ignore on it. If it's async, the ignore is called before we know that the futures will be chained.

That is, the equivalent function would be more like;

extension FutureExtension<T> on Future<T> {
  Future<T> whenCompleteEquivalent(action()) {
    return this.then((v) {
      var f2 = action();
      if (f2 is Future) return f2.then((_) => v);
      return v;
    }, onError: (e) {
      var f2 = action();
      if (f2 is Future) return f2.then((_) {
        return this; // CHANGE
      });
      return this; // CHANGE
    });
  }
}

The changed lines is what causes the chaining. It's a little dirty, I've never liked that code, but it preserves the error zone of the error, which is another complication that the code would otherwise need to address directly.