|
|
Created:
5 years, 4 months ago by nweiz Modified:
5 years, 3 months ago Reviewers:
Lasse Reichstein Nielsen CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/async.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd a CancelableFuture class.
This is useful for controlling processes that the user may want a value
out of or may want to cancel.
R=lrn@google.com
Committed: https://github.com/dart-lang/async/commit/ed025a1ca8f581c96d675a273ce476cb6a186556
Patch Set 1 #Patch Set 2 : Convert to CancelableOperation #
Total comments: 34
Patch Set 3 : Code review changes #Patch Set 4 : Code review changes #
Messages
Total messages: 14 (1 generated)
nweiz@google.com changed reviewers: + lrn@google.com
As a general principle,. I don't think a "cancelable future" is a good idea. It conflates a future (the result of a computation) with the computation itself. Since futures can otherwise be shared freely, and are reused in some cases (returning an existing future instead of creating a new one with the same result), putting something like "cancel" on a future doesn't mesh very well. If you share such an enhanced future, other code can see and access the cancel method. I'd much rather have something like: class Computation { Future get result; Future cancel({withResult}); // withResult can be Future. } that you use instead, so the future doesn't gain new features, and you can still pass the result future around freely without exposing the cancel method to everybody. The interaction between the Computation and the code that can be canceled is then the concern of the Computation class, not the Future class, and there is still only one type of Future. https://github.com/dart-lang/sdk/issues/22265 I know that both pub and the analyzer already have a cancelable future. If they are well-behaved, and not doing "is CancelableFuture" tests after passing around the value as a plain Future, then they should be able to use the Computation class instead with only minor rewrites. I would prefer to try that rather than adding a general cancelable future.
After a little more thought, a Computation class could look like: /// A cancelable computation. /// /// Runs a computation and captures its result in a [Future]. /// /// The computation is passed another future which is used to request that /// the computation aborts itself. class Computation<T> { /// The result of the computation. final Future<T> result; /// Completer for the `cancelFuture` passed to the computation. /// /// Complete this to request that the computation cancels itself. final Completer _cancelCompleter = new Completer(); /// Runs computation and completes [result] with its result. /// /// If [cancel] is called, the `cancelFuture` parameter to the computation /// will complete. The computation can then stop in any way it wants to. /// Its final result, if any, is still used to complete [result]. factory Computation(computation(Future cancelFuture)) : var cancelCompleter = new Completer(); var result = new Future.sync(() => computation(_cancelCompleter.future)) return new Computation<T>._(result, cancelCompleter); } Computation._(this.result, this._cancelCompleter); /// Cancel the computation. /// /// This completes the `cancelFuture` passed to the computation with /// [cancelValue] (defaults to `null`). The [cancelValue] may be a /// [Future], which may again contain an error. /// /// The computation must handle the future completing and cancel itself. /// The result of canceling will just be the future returned by void cancel([cancelValue]) { if (_cancelCompelter.isComplete) { // Or just return, but since there is a [cancelValue] throw new StateError("Computation canceled twice"); } _cancelCompleter.complete(cancelValue); } }
https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... File lib/src/cancelable_operation.dart (right): https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:11: /// An asynchronuos operation that can be cancelled. So basically, you have the result of an async operation (a future) and a function that you can call to cancel the operation. The goal of this class is to wrap that up and ensure some state management - if the result completes after cancel is called, the result is ignored, and the cancel function is called only once. Then there is the asStream operation which is just added on top of the rest. This does not require the cancelable completer class at all (it should be fairly simple), something like: class CancelableOperation<T> { final Completer<T> _completer = new Completer<T>.sync(); final Function _cancel; bool _cancelled = false; CancelableOperation(Future<T> result, Future cancel()) : _cancel = cancel { result.then(_completeValue, onError: _completeError); } Future<T> get result => _completer.future; void _completeError(e, s) { if (!_cancelled) { _completer.completeError(e, s); } } void _completeValue(T v) { if (!_cancelled) { _completer.complete(v); } } Future cancel() { if (_completer.isCompleted || _cancelled) { return new Future.value(); // never return null. } _cancelled = true; return new Future.sync(_cancel); } Stream asStream() {...} } So, the completer class is there as a convenience to someone who wants to create a cancelable operation. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:22: Future<T> get value => _completer._inner.future; I'd call this "result" since it may be an error. We usually distinguish between a future completing with a value or with an error. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:22: Future<T> get value => _completer._inner.future; Move getter below constructor. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:35: factory CancelableOperation.fromFuture(Future<T> inner, {onCancel()}) { Why is onCancel optional? If it's null, I don't see how this would do anything. Should be named "cancel". It's not something that is triggered by a cancel, but something which does a cancel. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:38: return completer.operation; You are passing a callback here, which is not going to be called in response to an async event. For that, I'd rather provide a future that completes when you call cancel, so you can manually add onCancel as a listener on that. That is more general than passing one onCancel function since you can add multiple listeners to such a future. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:45: Stream<T> asStream() { I prefer to make this a getter. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:70: class CancelableCompleter<T> { I don't think you need this class. Do you *want* it? https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:78: CancelableOperation<T> _operation; I find it very hard to see which fields the class have when it's interleaved like this. I'd prefer to move the getters down below the constructor. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:113: if (_isCanceled) return; If value is a future with an error, the error is not handled. It should probably be ignored since we have said we don't care about the operation's result. Maybe this line just shouldn't be here, since you check _isCanceled again below? https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:147: final _cancelMemo = new AsyncMemoizer(); Move field to top of class.
https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... File lib/src/cancelable_operation.dart (right): https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:27: CancelableOperation._(this._completer); I think one thing that makes it hard for me to wrap my head around these classes is the double dependency: operation's constructor creates the operation using a completer, completer creates an operation. It wasn't until I noticed this constructor that I realized why the recursion terminated :) It's possible to write them so that Operation stands by itself and Completer just creates one using its own future and cancel function. It's also slightly suspicious to me that there are two completers for each operation. One should be enough.
https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... File lib/src/cancelable_operation.dart (right): https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:27: CancelableOperation._(this._completer); On 2015/08/27 10:57:41, Lasse Reichstein Nielsen wrote: > It's also slightly suspicious to me that there are two completers for each > operation. One should be enough. And it is, the other one was the cancelable completer which I misread, so ignore this.
Code review changes
https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... File lib/src/cancelable_operation.dart (right): https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:11: /// An asynchronuos operation that can be cancelled. On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > So basically, you have the result of an async operation (a future) and a > function that you can call to cancel the operation. > > The goal of this class is to wrap that up and ensure some state management - if > the result completes after cancel is called, the result is ignored, and the > cancel function is called only once. > Then there is the asStream operation which is just added on top of the rest. > > This does not require the cancelable completer class at all (it should be fairly > simple), something like: > > class CancelableOperation<T> { > final Completer<T> _completer = new Completer<T>.sync(); > final Function _cancel; > bool _cancelled = false; > CancelableOperation(Future<T> result, Future cancel()) : _cancel = cancel { > result.then(_completeValue, onError: _completeError); > } > > Future<T> get result => _completer.future; > > void _completeError(e, s) { > if (!_cancelled) { > _completer.completeError(e, s); > } > } > > void _completeValue(T v) { > if (!_cancelled) { > _completer.complete(v); > } > } > > Future cancel() { > if (_completer.isCompleted || _cancelled) { > return new Future.value(); // never return null. > } > _cancelled = true; > return new Future.sync(_cancel); > } > > Stream asStream() {...} > } > > So, the completer class is there as a convenience to someone who wants to create > a cancelable operation. All this is true, although CancelableCompleter is intentionally forefronted because I think it's the most natural and common way to interact with a CancelableFuture. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:22: Future<T> get value => _completer._inner.future; On 2015/08/27 10:20:53, Lasse Reichstein Nielsen wrote: > I'd call this "result" since it may be an error. We usually distinguish between > a future completing with a value or with an error. "result" seems confusing when the package has a "Result" type that's unrelated. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:22: Future<T> get value => _completer._inner.future; On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > Move getter below constructor. Done, although this is contrary to the style most packages follow, which places getters near fields because they work the same way from an API standpoint. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:27: CancelableOperation._(this._completer); On 2015/08/27 10:57:41, Lasse Reichstein Nielsen wrote: > I think one thing that makes it hard for me to wrap my head around these classes > is the double dependency: operation's constructor creates the operation using a > completer, completer creates an operation. > > It wasn't until I noticed this constructor that I realized why the recursion > terminated :) > > It's possible to write them so that Operation stands by itself and Completer > just creates one using its own future and cancel function. That's true, but I expect exposing operations created by completers to be much more common than those created via [fromFuture]. The current version optimizes for that, both in terms of efficiency and in terms of the flow of the code. The Completer has all the real functionality; the Operation is just a shell on top to expose a specific API to the consumer. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:35: factory CancelableOperation.fromFuture(Future<T> inner, {onCancel()}) { On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > Why is onCancel optional? If it's null, I don't see how this would do anything. > > Should be named "cancel". It's not something that is triggered by a cancel, but > something which does a cancel. From the user's perspective, canceling an operation (like canceling a stream subscription) primarily means that the future won't fire. It may or may not make things more efficient down the line, but that's not their primary concern. The callback is optional because a user may want an operation that can be told not to complete even if there's no way to take advantage of that to avoid doing work. Along the same lines, the class uses "canceling" to mean "telling the operation not to complete the future". Taken that way, the callback's job isn't to cancel the operation; the class already does that. The callback is just there so that the code that produces the CancelableOperation can react to the cancellation if they have some useful way to do so. It's basically the same principle as StreamController's onCancel argument. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:38: return completer.operation; On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > You are passing a callback here, which is not going to be called in response to > an async event. > For that, I'd rather provide a future that completes when you call cancel, so > you can manually add onCancel as a listener on that. > > That is more general than passing one onCancel function since you can add > multiple listeners to such a future. I don't think it's very likely that multiple disparate pieces of code will want to register callbacks for handling cancellation, and the way it works now makes it possible to pipe the result of [onCancel] to the return value of [cancel]. It's also entirely parallel to StreamController, which is familiar to users and has proven itself quite usable. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:45: Stream<T> asStream() { On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > I prefer to make this a getter. I think it's more important for it to be consistent with [Future.asStream]. Also, a getter makes it seem like the Stream is the same "tier" of exposed value as the Future as opposed to being derived, which is confusing in a couple ways: it makes it less clear that this class is "cancel behavior like Stream/StreamController but for Futures", and it makes it less clear that calling [asStream] will stop any errors in [value] from being top-leveled. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:70: class CancelableCompleter<T> { On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > I don't think you need this class. > Do you *want* it? Yeah; see above comments. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:78: CancelableOperation<T> _operation; On 2015/08/27 10:20:53, Lasse Reichstein Nielsen wrote: > I find it very hard to see which fields the class have when it's interleaved > like this. I'd prefer to move the getters down below the constructor. Done, although see above. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:113: if (_isCanceled) return; On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > If value is a future with an error, the error is not handled. > It should probably be ignored since we have said we don't care about the > operation's result. Done. > Maybe this line just shouldn't be here, since you check _isCanceled again below? I don't want to complete the inner completer for non-Future values or keep around closures with references to this completer for longer than necessary, but done. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:147: final _cancelMemo = new AsyncMemoizer(); On 2015/08/27 10:20:54, Lasse Reichstein Nielsen wrote: > Move field to top of class. I will if you insist, but I think it's a lot clearer to have memoization fields right next to the corresponding methods/getters.
lgtm https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... File lib/src/cancelable_operation.dart (right): https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:22: Future<T> get value => _completer._inner.future; The order in the file is independent of how the API is presented in documenatation. I use it find the structure of the class, which is why I prefer to have the fields collected in one predictable place. I thought the style guide said something about the order of members, but I can't find it now. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:35: factory CancelableOperation.fromFuture(Future<T> inner, {onCancel()}) { Ack, makes sense. Cancel only cancels the "operation" object, the source can optiohally be notified if it wants to. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:38: return completer.operation; True. It has to be documented when the callback will be called (and be carefully chosen so as to not cause any reentrancy problems). https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:45: Stream<T> asStream() { Ok, that's fair. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:64: _canceled = true; Maybe drop the _canceled field in this class and just read _completer._isCanceled directly. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:92: bool _fired = false; I think _fired is equivalent to _inner.isCompleted, so you don't need an extra field for it. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:147: final _cancelMemo = new AsyncMemoizer(); I still prefer to have all the fields collected in one place. That makes it easier to see what the state of the object is. We should have static variables in instance functions, like in C. Then it's obvious that it's an instance field that's only visible to the current function.
https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... File lib/src/cancelable_operation.dart (right): https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:38: return completer.operation; On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > True. It has to be documented when the callback will be called (and be carefully > chosen so as to not cause any reentrancy problems). Done. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:64: _canceled = true; On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > Maybe drop the _canceled field in this class and just read > _completer._isCanceled directly. We don't actually use this field anyway, it turns out. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:92: bool _fired = false; On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > I think _fired is equivalent to _inner.isCompleted, so you don't need an extra > field for it. Done. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:147: final _cancelMemo = new AsyncMemoizer(); On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > I still prefer to have all the fields collected in one place. That makes it > easier to see what the state of the object is. > > We should have static variables in instance functions, like in C. Then it's > obvious that it's an instance field that's only visible to the current function. Done.
https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... File lib/src/cancelable_operation.dart (right): https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:38: return completer.operation; On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > True. It has to be documented when the callback will be called (and be carefully > chosen so as to not cause any reentrancy problems). Done. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:64: _canceled = true; On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > Maybe drop the _canceled field in this class and just read > _completer._isCanceled directly. We don't actually use this field anyway, it turns out. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:92: bool _fired = false; On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > I think _fired is equivalent to _inner.isCompleted, so you don't need an extra > field for it. Done. https://codereview.chromium.org/1266603005/diff/20001/lib/src/cancelable_oper... lib/src/cancelable_operation.dart:147: final _cancelMemo = new AsyncMemoizer(); On 2015/09/03 13:06:17, Lasse Reichstein Nielsen wrote: > I still prefer to have all the fields collected in one place. That makes it > easier to see what the state of the object is. > > We should have static variables in instance functions, like in C. Then it's > obvious that it's an instance field that's only visible to the current function. Done.
Code review changes
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as ed025a1ca8f581c96d675a273ce476cb6a186556 (presubmit successful). |