Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Future#cancel #1806

Closed
DartBot opened this issue Feb 22, 2012 · 15 comments
Closed

Add Future#cancel #1806

DartBot opened this issue Feb 22, 2012 · 15 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Feb 22, 2012

This issue was originally filed by @seaneagan


Future should allow canceling of callbacks added with Future#then. Might look like:

void cancel(void onComplete(T value));

example:

future.then(onComplete);
//...
future.cancel(onComplete);

@iposva-google
Copy link
Contributor

Siggi, this is your area.


cc @kasperl.
Set owner to @sigmundch.
Added Area-Library, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Feb 13, 2013

This comment was originally written by @seaneagan


A couple of use cases for this:

I believe the only reason for the Timer class to exist now is that a Future.delayed(...) callback cannot be canceled.

and the only reason for dart:html.requestAnimationFrame to exist is that a dart:html.animationFrame callback cannot be canceled.

However, I think the API for cancellation should look similar to how it does for Streams, where the callback subscription method returns an object which can be used to cancel the callback. It would probably look just like Future.then, except return such an object:

Canceler maybe(onValue(T value), {onError(AsyncError asyncError)});

where dart:async defines:

typedef void Canceler();

@sigmundch
Copy link
Member

+florian who's now actively maintaining this code.


cc @floitschG.
cc @blois.
Removed the owner.

@floitschG
Copy link
Contributor

cc @lrhn.

@nex3
Copy link
Member

nex3 commented Feb 28, 2013

I don't like this proposal, for a couple reasons. From a semantic standpoint, I think Futures work best when they're consistently thought of as a way of declaring a chain of computations, rather than registering callbacks on an emitter, which is more of a Stream thing. A cancelable callback goes against this idea. From a more pragmatic angle, I've found it to be useful in various cases to create classes that implement Future, and adding more methods to it makes these classes harder to write.

Why not just use future.asStream().listen and get all the benefits of the StreamSubscription API?

@DartBot
Copy link
Author

DartBot commented Mar 12, 2013

This comment was originally written by @seaneagan


It seems to make just as much sense to be able to cancel
callbacks on a Future as a Stream. In either case something can
happen before the completion of the Future/Stream which makes you no
longer want to take action. Consider a user clicking on one data row,
resulting in a fetch (Future), then clicking on another row before the
first fetch completes. Now you have a race condition, so you want to
be able to cancel the first fetch callback before starting the second. It's like an async version of "if"(/"else"?).

It seems like implementing extra methods on Future should be handled
pretty well by a mixin of some sort, just as with Iterable, Stream,
etc.

That workaround works, but it's not quite as convenient or discoverable:

future.asStream().listen(callback)

vs:

future.maybe(callback);

I can't think of any reason to "pause" / "resume" a Future subscription, but there probably are some out there.

@lrhn
Copy link
Member

lrhn commented Jun 4, 2013

Listeners on futures are not "subscribers".
They are computations that expect a result of a previous computation before they can begin. You can't cancel a computation, but you are free to write your computation to do nothing in some cases.

A utility function doing what you want would be:

class Canceler {
  bool _canceled = false;
  Canceler(this.result);
  final Future result;
  void cancel() { _canceled = true; }
}
Canceler maybe(Future future, continuation(var result), {onError(error)}) {
  Canceler canceler;
  Completer completer = new Completer();
  Future future = future.then((v) {
    if (!canceler._canceled) {
      return continuation(v);
    } else {
      return new Completer().future;  // Never completes.
    }
  }, onError: (e) {
    if (!canceler._canceled) {
      if (onError != null) return onError(e);
      throw e;
    }
    return new Completer().future;
  });
  canceler = new Canceler(future);
  return canceler;
}  

(Obviously not tested!)

I don't think we want subscriptions on futures.

@lrhn
Copy link
Member

lrhn commented Aug 16, 2013

We have no current plans to add canceling to futures, or to allow removing then-handlers from futures.


Removed Type-Defect label.
Added Type-Enhancement, NotPlanned labels.

@DartBot DartBot added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue labels Aug 16, 2013
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
@slightfoot
Copy link
Contributor

slightfoot commented Feb 27, 2019

@kwalrath This is the cancel future ticket.

@kwalrath
Copy link
Contributor

@lrhn in a chat with developers today, one said that having lots of "open" futures slows down apps and is hard to diagnose. Do we have any plans to revisit this, or are there tools devs can use to hunt down these futures and fix their code?

@kwalrath kwalrath reopened this Feb 27, 2019
@lrhn
Copy link
Member

lrhn commented Feb 28, 2019

There is no cost to having an "open" future that doesn't do computation. It's just an object, like any other object.
What costs is having computations running, and the ability to cancel that computation is not something that belongs on Future. It's something the computation itself must cooperate with.
There is no easy way to integrate cancelabilty with async functions, so most async functions would not be cancelable. The functions which are cancelable have to be written for that, and they cannot be async functions.

We can help you write such functions. I still recommend using CancelableOperation from package:async or something similar, perhaps building on that class.

import "dart:async";
import "package:async/async.dart";

CancelableOperation<T> runWithCancel<T>(
    Future<T> action(Future<void> onCancel)) {
  var cancel = Completer<void>();
  return CancelableOperation<T>.fromFuture(action(cancel.future), onCancel: () {
    cancel.complete();
    return null;
  });
}

main() {
  var running = runWithCancel<int>((cancelFuture) async {
    bool isCancelled = false;
    cancelFuture.then((_) {
      isCancelled = true;
    });
    int i = 0;
    while (true) {
      if (isCancelled) return -1;
      print(i++);
      await Future.delayed(Duration(milliseconds: 250));
    }
    return 42;
  });
  var timer = Timer(Duration(seconds: 2), running.cancel);
  running.value.then(print).whenComplete(timer.cancel);
}

We still have no current plans to add cancel to Future. It's not the right place for that functionality.

@lrhn lrhn closed this as completed Feb 28, 2019
@pinyin
Copy link

pinyin commented May 17, 2019

As a Dart user, I think there may be a situation where a cancelable future is very useful.

When using Stream in combination with the async* syntax, it's quite normal to break the flow with a Future at some lines:

Stream<int> load10Data(Future<int> loadAt(int index)) async* {
  for (var i = 0; i < 10; i++) {
    yield await loadAt(i);
  }
}

When a Stream from load10Data is listened to then canceled before it finishes, if await accepts something like CancelableFuture (and forward the cancellation down to the await stack), the pending loadAt would know it should do a graceful shutdown instead of hanging there.

Allowing such syntax will also make StreamTransformer.bind much easier to use, since one would be able to await on Stream#singleWhere in the passed bind function. The literal promise provided by Future may not be an obstruct here if we consider every CancelableFuture as an alternate future timeline, which is cool IMO :)

CancelableOperations will not help here because awaits only work with Futures. Of course, there's nothing stopping a programmer from replace the yield await Future to a yield* Stream call.

But Stream won't solve a bigger problem: since Future is always the first choice for library authors (lightweight and catchable), the lack of a CancelableFuture may accidentally make many cancelable actions to be non-cancelable across 3rd-parth libraries in the future, which is happening in the JavaScript community. IMHO the fetch API in browsers may be a good counterexample for weirdly making network requests non-cancelable, I really hope Dart's ecosystem would go the other way.

@nex3
Copy link
Member

nex3 commented May 17, 2019

@pinyin For what it's worth, you can write:

Stream<int> load10Data(CancelableOperation<int> loadAt(int index)) async* {
  for (var i = 0; i < 10; i++) {
    yield* loadAt(i).asStream();
  }
}

and it should have the behavior you want when you cancel the load10Data() stream. But that certainly is more cumbersome, and CancelableOperation living outside the core library means that many operations that are notionally cancelable don't "just work" in this way.

@lrhn
Copy link
Member

lrhn commented May 17, 2019

Your example of yield await something; where a cancel on the stream would somehow cancel the something operation is hard to define in a consistent way. It is currently completely equivalent to
var tmp = await something; yield tmp;. If cancelling the stream should affect them the same, then it means that cancelling the stream of an async* function should be able to interrupt any await in the body. Interrupting an expression is tricky, because expressions can only complete in two ways: with a value or with a throw. Neither seems right here.

If it completes with a value, then computation will continue with that value. If you have var x = await something; x.update(whatnot); yield x.whatever;, then completing with null will make the code fail badly. Completing with any other value is likely just as bad, because it STILL won't be the correct value that would allow the code to continue without issues. Throwing immediately is also unlikely to be what you want, then you would have to defensively wrap the entire body of any async* function with a catch to avoid it throwing on a cancel.

One alternative is to make it throw an AsyncCancelException and make async* function bodies implicitly catch that and complete the stream normally. That would allow cancelling an async* method at any await, not just at yields.
This was actually considered originally, but was rejected because it made it incredibly hard to reason about the code. Something as simple as:

var res = await allocateResource();
try {
 . ..
} finally {
  res.dispose();
}

(which is perfectly good async code, if allocation throws, nothing happens, if not, the allocated resource is definitely disposed) would not be safe for async* code. If the await cancel-throws after allocating the resource, but before entering the try/finally, then the resource is not disposed correctly.

If any await can throw (or perform any other control flow), independently of the awaited future, then it is practically impossible to write safe code.
That is why the model is that an async* method only checks for cancel at yield statements. A yield is documented as being able to bail out when the stream is cancelled, but it never happens in the middle of another expression, so expression evaluation stays rational and consistent. If you await a future, the result you get is the result of that future, not anything else injected from the side.

The only semantically reasonable option is to make a cancelled future never complete. Sadly, that's too dangerous to allow. If the future never completes, the method body is stuck at the await, and won't execute finally blocks. Example:

var res = allocateResource();
try {
  while (await res.fetchNextPage()) {
     for (var entry in res.currentPage) yield entry;
  }
} finally {
 await res.dispose();
}

Now, if this async* body was cancelled, and it cancelled the awaited future returned by res.fetchNextPage(), then execution would not continue. It would block at that await.
Which means that the resource is never disposed.
Blocking non-completing futures can already happen by accident, but when it does, it's usually a hard-to-debug error caused by some future accidentally never completing. Allowing it to happen for every await in an async* body is far too scary to contemplate.

This is all an aside from cancelable futures, saying why even if we had them, an async* stream cancel would not cancel an awaited future in the method body anyway.
It also describes some of the issues with canceling futures. If only some futures were cancelable (say, all futures had a cancel method to request a cancel, but no promise that it would do anything), then those futures would still have to figure out how to behave after cancel: complete with a default value/provided value/null (won't work when we have non-null types), throw or block.

All in all, we are not planing to make futures cancelable. The API is all wrong for that.
If we had designed futures differently from the beginning, maybe, and only maybe, would we have had a way to make them cancelable in a meaningful way.

@pinyin
Copy link

pinyin commented May 17, 2019

Thanks for the replies & clear explanations.
If making Future cancelable is not an option, would moving CancelableOperation to dart:async be possible? So the problems cannot be solved by cancelable promises, but the need of something that:

  • is lightweight
  • represents a cancelable asynchronous operation
  • resolves to one value
  • can block async* in some way and can be automatically canceled (as @nex3 demonstrated)
  • usable in core libraries, especially dart:io, to encourage 3rd-party usage

may still exist.
I'm not sure if it still belongs to this issue, but CancelableOperation seems to be covering most potential use cases for cancelable promsies, except not being in the core libraries.
This may be relevant to #33713.

copybara-service bot pushed a commit that referenced this issue Dec 5, 2022
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/dc502d0..4d7dc93):
  4d7dc93b  2022-12-05  dependabot[bot]  Bump github/codeql-action from 2.1.31 to 2.1.35 (#3263)
  bcf8b6e8  2022-12-05  Parker Lougheed  Weight enums the same as classes for searching (#3260)
  7d95578b  2022-12-04  Parker Lougheed  Update template descriptions (#3258)
  d558f043  2022-12-04  Parker Lougheed  Fix error when using base element href (#3256)
  c3663762  2022-12-04  Parker Lougheed  Add unnecessary override ignore to fix build (#3257)

http (https://github.com/dart-lang/http/compare/976bd56..46a7708):
  46a7708  2022-12-02  Brian Quinlan  Remove binary artifact (#833)

sync_http (https://github.com/dart-lang/sync_http/compare/f5c1f18..8622614):
  8622614  2022-12-02  Kevin Moore  blast_repo fixes (#32)

test (https://github.com/dart-lang/test/compare/f3d80a6..4dceb87):
  4dceb87c  2022-12-01  Nate Bosch  Ignore some usage of dperecated errors (#1807)

webdev (https://github.com/dart-lang/webdev/compare/91b8a19..e39506e):
  e39506e  2022-12-05  Anna Gringauze  Pre-warm expression compiler to speed up Flutter Inspector page loading. (#1786)
  9b19b3b  2022-12-02  Elliott Brooks (she/her)  Can save storage objects in both `session` and `local` storage (#1807)
  e75c45e  2022-12-02  Elliott Brooks (she/her)  Injected client adds `isFlutterApp` to global window object (#1806)
  ba5e3ec  2022-12-01  Elliott Brooks (she/her)  `DebugSession` listens to events instead of just sending events (#1804)

Change-Id: I881d02e966b763879df72b29653a9f241b71eb3d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/273826
Commit-Queue: Devon Carew <devoncarew@google.com>
Reviewed-by: Nate Bosch <nbosch@google.com>
copybara-service bot pushed a commit that referenced this issue Dec 9, 2022
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/4d7dc93..f2bb6e9):
  f2bb6e92  2022-12-07  Sam Rawlins  Bump to 6.1.4 (#3266)

fixnum (https://github.com/dart-lang/fixnum/compare/62916f2..e4f5e97):
  e4f5e97  2022-12-08  Michael Thomsen  Change IntegerDivisionByZeroException to UnsupportedError (#100)
  3bd726f  2022-12-08  Devon Carew  rev the sdk min to 2.19.0-0 (#99)

logging (https://github.com/dart-lang/logging/compare/f322480..0373ef8):
  0373ef8  2022-12-07  Mushaheed Syed  Add a check that throws if a logger name ends with '.' (#123)
  6d46d71  2022-12-06  Devon Carew  Create no-response.yml (#124)

protobuf (https://github.com/dart-lang/protobuf/compare/4f3e328..2706b53):
  2706b53  2022-12-07  Mahdi K. Fard  Add type to a method parameter (#782)
  a57c16a  2022-12-07  Mahdi K. Fard  Fix a typo (#781)

shelf (https://github.com/dart-lang/shelf/compare/1c21047..32e342d):
  32e342d  2022-12-08  István Soós  Prepare the release of shelf_router_generator (#316)
  06e2fe6  2022-12-08  Kevin Moore  shelf: drop non-null-safe tests (#317)
  98363fd  2022-12-06  Kevin Moore  lint: sort pub dependencies
  ad6af2a  2022-12-06  Kevin Moore  shelf_static: move RegExp creation out of every request

test (https://github.com/dart-lang/test/compare/4dceb87..73cd754):
  73cd7540  2022-12-07  Nate Bosch  Record the working directory for VM platform (#1804)
  e40274a6  2022-12-07  Nate Bosch  Restore mono_repo config (#1810)
  02d8764e  2022-12-07  Sigurd Meldgaard  Use Future for return type in runner `main()`. (#1809)
  3d6039b3  2022-12-05  Nate Bosch  Merge test selections by path (#1806)

webdev (https://github.com/dart-lang/webdev/compare/e39506e..3e2364e):
  3e2364e  2022-12-07  Elliott Brooks (she/her)  Add the Dart Debugger / Flutter Inspector panels in Chrome DevTools (#1812)
  c164231  2022-12-07  Elliott Brooks (she/her)  Small fix to the extension test (#1811)
  4bbb4d0  2022-12-06  Elliott Brooks (she/her)  Gracefully handle debugger disconnect events (#1808)
  d3892cf  2022-12-05  Elliott Brooks (she/her)  Refactor puppeteer tests to use `Worker` type (#1809)

Change-Id: I42033e849f40f209831cfb344247b24ad7731ff0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/274580
Reviewed-by: Nate Bosch <nbosch@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

10 participants